-
Notifications
You must be signed in to change notification settings - Fork 79
improve tracing for openai agents #205
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
Conversation
Note that we appear to have a tracing bug where some spans don't get closed. We will address this as a separate issue. |
trace_id = gen_trace_id() | ||
with trace("Orchestrator evaluator", trace_id=trace_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does trace ID have to be explicitly generated here? It is the default correct? (this calls our deterministic UUID generator, and have it on our TODO to stop using the RNG shared with the user so we don't affect their RNG use with our own)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed that the default behavior is correct (get uuid from workflow) and removed the explicit trace_id generation.
Added non-shared RNG as a TODO.
instructions="You only respond in haikus.", | ||
) | ||
trace_id = gen_trace_id() | ||
with trace("Hello World", trace_id=trace_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression samples like these were intentionally meant to look like https://github.com/openai/openai-agents-python/blob/main/examples/basic/hello_world.py for comparison. OpenAI chooses not to demonstrate tracing in their hello world, is it important that Temporal demonstrate OpenAI tracing in the same sample? Should we upstream a PR to OpenAI samples if we feel demonstrating tracing in hello world is important? I think it may be clearer if we are wanting to demonstrate something beyond just showing OpenAI sample equivalents, we do it in a separate sample.
EDIT: This was removed from hello_world sample but still remains on tool sample. Do we want to upstream a PR to add a trace
around https://github.com/openai/openai-agents-python/blob/b09d37d8326c5d21760aaf10e9ff2eb8a01ebdaa/examples/basic/tools.py#L20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made our sample match.
We can stick with matching the samples as closely as possible and can add additional samples/examples where we want to show more.
@cretz thanks for your review. I have addressed the issues you pointed out. Please have another look. |
Thanks! I still have the concerns about deviating from OpenAI samples at #205 (comment) (can discuss on that thread). Also would like @dandavison or @tconley1428 to review if possible (I am doing less Python things these days). |
What was changed
Added tracing to all of the OpenAI Agents examples.
Why?
Some examples had incomplete tracing.
Checklist
Closes
How was this tested:
Manual validation via OpenAI platform UI.
Any docs updates needed?
N/A