Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3ad8ca3da2
ℹ️ 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".
3ad8ca3 to
9ac6e49
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac6e493b8
ℹ️ 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".
| return False | ||
| provider._set_processors(processors) | ||
| default_processor = _DEFAULT_PROCESSOR | ||
| if default_processor is not None and default_processor not in processors: |
There was a problem hiding this comment.
When a custom TracingProcessor implements value equality that returns true for the SDK processor, default_processor not in processors is false even though the default instance was removed from the provider. The provider then contains only the custom processor, while _DEFAULT_PROCESSOR and _global_processor still identify the detached default as active; its exporter is never retired, and atexit also skips closing it because the cached processor remains marked as attached. Use an identity check for this ownership decision.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
Co-authored-by: abhay-codes07 <[email protected]> Co-authored-by: rajarshidattapy <[email protected]>
9ac6e49 to
7bda67a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bda67a3b8
ℹ️ 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".
| _DEFAULT_PROCESSOR = None | ||
| _SDK_DEFAULT_PROCESSOR = None |
There was a problem hiding this comment.
When callers save the current processor list, temporarily call set_trace_processors([custom]), and later restore the saved list, these assignments discard both ownership pointers while retirement makes the saved default processor terminal. Restoring that list then follows the _DEFAULT_PROCESSOR is None path and installs the shut-down instance unchanged, so every subsequent default trace or span is silently dropped by _enqueue; the repository already uses this save/restore pattern in integration_tests/openai/test_tracing.py:74-95. Recognize the retired SDK default during restoration and replace it with the live recovered processor.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
|
After reassessing the practical impact, I don't think we should merge this lifecycle expansion. Replacing processors does not create a new default exporter or HTTP client each time; the current implementation retains one module-owned instance. Applications that construct their own exporters can close those exporters explicitly, and a fresh exporter is the existing alternative to reusing one after shutdown. Closing the process-lifetime default client does not justify automatic recovery, configuration restoration, processor retirement, and additional shutdown coordination. I'm closing this PR and the associated lifecycle proposals in #4681 and #4683 as not planned. |
This pull request supersedes #4712 and replaces the overlapping #4732 lifecycle work with one coherent default tracing shutdown model.
The module-owned default
BackendSpanExporternow closes exactly once after its final export, including timed shutdowns with a surviving worker. Caller-injected exporters remain caller-owned. Terminal default processors and exporters recover through the existing provider pipeline while preserving exporter configuration, disabled state, custom processors, and provider identity.The change also makes repeated shutdown safe, retires defaults removed through either top-level or direct provider processor replacement, serializes API-key updates with recovery, and prevents atexit callbacks from resurrecting a fresh default tracing stack.
This pull request resolves #4681.
This pull request resolves #4683.