Repository navigation
fix(tracing): flush buffered data after tracing is disabled - #4528
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c088462d29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the fix. Comparing v0.22.0, the base, and this branch confirms that disabling tracing already allows buffered data to be exported by the background worker and shutdown. Allowing an explicit flush to drain that data is therefore a useful correction.
Please retain _refresh_disabled_flag() and remove only the disabled early return. Removing the refresh call also changes when OPENAI_AGENTS_DISABLE_TRACING is first cached, which is unrelated to this fix. Please add a focused regression test preserving that initialization timing.
|
Fixed. I updated the existing disabled-provider force_flush() test to match the new lifecycle contract, so both the direct provider test and the public flush_traces() regression now verify that buffered processors still flush after tracing is disabled. |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks. The contradictory test is fixed, but my latest review request is still outstanding. Please retain _refresh_disabled_flag(), remove only the disabled early return, and add the regression test for the environment variable's initialization timing.
|
Done. I restored |
seratch
changed the title
Summary
Keep the public
flush_traces()operation effective after tracing has been disabled.DefaultTraceProvider.force_flush()currently returns early when the provider's disabled flag is set. That means an application can create buffered trace data, callset_tracing_disabled(True)to stop collecting new traces, and then callflush_traces()before a worker or request exits, but the explicit flush silently does nothing.Disabling tracing should control creation of new trace data. It should not prevent already-registered processors from flushing data they already own. This follows the same lifecycle distinction as processor shutdown.
Reproduction
On current
main,flush_traces()reachesDefaultTraceProvider.force_flush()and returns before calling any registered processor.Change
Remove the disabled-state early return from
DefaultTraceProvider.force_flush(). Trace and span creation remain disabled exactly as before.Test plan
Adds a regression test through the public
flush_traces()function and verifies that a registered processor receivesforce_flush()even after the provider is disabled.Issue number
N/A