Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenAI instrumentation docs fixes #2988

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Nov 9, 2024

],
)

Enabling message content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this snippet about configuring logs and events meant to be in here, too? open-telemetry/opentelemetry.io#5575 (comment)

*************************

When using the instrumentor, all clients will automatically trace OpenAI chat completion operations
and capture prompts and completions as events.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only happens when OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT is set as true, right? Can we highlight the parameter here as well?

Copy link

@dasiths dasiths Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worthwhile putting the example (json) format of the content shape that is emitted.

Also might be worth stating that if doing this you may have to configure your pipeline to redact any sensitive info contained in the content.

Comment on lines +26 to +27
When using the instrumentor, all clients will automatically trace OpenAI chat completion operations
and capture prompts and completions as events.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thought to @gyliu513 and also I don't think most people know what events are yet, specifically they are log not span. Maybe adding the word log helps?

Suggested change
When using the instrumentor, all clients will automatically trace OpenAI chat completion operations
and capture prompts and completions as events.
When using the instrumentor, all clients will automatically trace OpenAI chat completion operations.
You can also optionally capture prompts and completions as log events, discussed later.

*************************

Message content such as the contents of the prompt, completion, function arguments and return values
are not captured by default. To enable message content, set the environment variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
are not captured by default. To enable message content, set the environment variable
are not captured by default. To capture message content as log events, set the environment variable

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a doco or something related to setting up log events instrumentation that can be linked in place here? Most existing apps will not have log events provider setup.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karthikscale3 just confirming we are using log events and not span events for this even in the interim?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dasiths open-telemetry/opentelemetry-python#4269 has the patch for log event enabling, but that will only work if your backend/collector accepts logs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad sorry, i just realized this PR was merged - https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2925/files . Yes log events is what we are using now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dasiths can you confirm you can see the log events in tempo by clicking "logs for this span"? I've not tried tempo..

Copy link

@dasiths dasiths Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dasiths can you confirm you can see the log events in tempo by clicking "logs for this span"? I've not tried tempo..

Yes the logs do appear once OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT=true is set. https://github.com/jsburckhardt/ai-translation-bot-demo/blob/c3ddf39f65cba2eac742fdfbfb4dec4ebe19d7b3/app/services/observability.py#L150C6-L163C13

edit: I did however get a warning saying event logger provider is already set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I did however get a warning saying event logger provider is already set." me, too, but hopefully when "zero code" works that message goes away with it.

@lzchen lzchen added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 12, 2024

Message content such as the contents of the prompt, completion, function arguments and return values
are not captured by default. To enable message content, set the environment variable
`OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT` to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I found that if this is set to false, it currently crashes. Validation if that's only me requested here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants