fix(tracing): rebuild the default exporter and processor after shutdown - #4732
fix(tracing): rebuild the default exporter and processor after shutdown#4732rajarshidattapy wants to merge 7 commits into
Conversation
`BackendSpanExporter._shutdown_event` is the signal that abandons retry backoff, and it is never cleared. Because `default_exporter()` caches a module-level singleton, a processor shutdown left that signal set on an exporter the SDK would hand straight back out. Any batch exported through it afterwards gave up on the first 5xx instead of backing off, and was dropped with a warning blaming a shutdown that had long since finished. Make shutdown terminal instead of clearing the signal: `default_exporter()` and `default_processor()` discard the cached pair once either half has been shut down, so a later tracing initialization starts over with a fresh exporter and processor. The processor owns the exporter it was handed, so the two go down together -- no ref counting and no cross-processor cancellation protocol. Both classes expose `is_shut_down` so the terminal state is explicit rather than a private-attribute peek. Closes openai#4683
![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: 314e8aec59
ℹ️ 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".
Discarding the cached exporter/processor was only half the story. When tracing had been bootstrapped through `get_trace_provider()`, a timed `provider.shutdown()` left that provider registered with the original processor. The next `default_exporter()` or `set_tracing_export_api_key()` call built a fresh pair, but `get_trace_provider()` still handed back the old provider -- so default traces kept flowing through the shut-down exporter, and a newly configured API key landed on an exporter nothing exported through. Track the default processor wired into a provider we bootstrapped ourselves, and re-initialize the provider alongside the cache when that processor is shut down. Shutdown stays terminal for the whole default stack. A provider supplied through `set_trace_provider` is left alone: its lifecycle belongs to whoever set it. Guard the atexit path so a span closed during interpreter teardown cannot resurrect the stack and build a fresh HTTP client on the way out.
…-shutdown' into fix/tracing-fresh-defaults-after-shutdown
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba80bba6b4
ℹ️ 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".
…igured Rebuilding objects to recover from a shutdown meant every piece of caller-set configuration became something the recovery path had to remember to copy, and it was not copying any of it. Two structural fixes remove the whole class: Keep the provider. Only the one dead default processor is swapped, in place, via `SynchronousMultiTracingProcessor._replace_processor`. `set_tracing_disabled`, the cached env flag, and every processor added through `add_trace_processor` survive untouched, and registration order is preserved. A caller who dropped the default processor with `set_trace_processors` does not get it back. Carry the exporter's configuration. `_replacement_exporter` copies the API key set through `set_tracing_export_api_key` along with the organization, project, endpoint and retry schedule, so recovery restores the configured exporter rather than one derived from the environment. An application relying on a trace-only key no longer silently stops exporting after a shutdown. The exporter is only replaced when it is itself shut down. A shutdown with no timeout never signals it, so it is now reused rather than dropped, which also stops the recovery path leaking a live HTTP client.
get_trace_provider() is typed as the base TraceProvider, whose shutdown() takes no timeout, so pyright rejected the timeout kwarg. Cast at the two call sites, matching how the surrounding assertions already reach into the concrete provider.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8743a787c
ℹ️ 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".
| if _global_exporter is not None and _global_exporter.is_shut_down: | ||
| _global_exporter = _replacement_exporter(_global_exporter) | ||
| _global_processor = None |
There was a problem hiding this comment.
When a default provider is shut down with a timeout and then reinitialized, this replacement creates a new HTTP client but never closes the previous exporter's client after the stale processor is swapped out. Repeated shutdown/recovery cycles in a long-running worker therefore retain stale connection pools instead of deterministically releasing them; close the old exporter as part of replacing the cached pair.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
2 participants
Closes #4683.
The bug
BackendSpanExporter._shutdown_eventis the signal that abandons retry backoff, and it is never cleared:default_exporter()caches a module-level singleton, so aBatchTraceProcessor.shutdown(timeout=...)left that signal set on an exporter the SDK would hand straight back out on the next call. Every batch exported through it afterwards gave up on the first 5xx instead of backing off, and was dropped with a warning blaming a shutdown that had long since finished.The fix
This takes the terminal ownership model @seratch asked for in #4684, rather than clearing the shutdown signal:
default_exporter()anddefault_processor()discard the cached pair once either half has been shut down, so a later tracing initialization creates a fresh exporter and processor.BackendSpanExporter.is_shut_downandBatchTraceProcessor.is_shut_downmake the terminal state explicit instead of a private-attribute peek.Scoped deliberately to the second half of that model. The first half — the owner closes the default exporter — is #4712, and nothing here changes closing behaviour, so the two compose rather than competing.
The lock-free fast path in
default_exporter()/default_processor()is gone because the cached value now needs a liveness check. Both are cold-path calls (once per provider init, once perset_tracing_export_api_key), and the processor stays lazy: asking for the exporter still does not construct a processor or any threading primitives.Repro from the issue, before and after
Tests
Seven cases added to
tests/test_trace_processor.py:is_shut_downflags flip on shutdown, and a no-timeout shutdown leaves the exporter itself unsignalleddefault_exporter()does not build a processor (lazy-init regression guard)BackendSpanExporter._shutdown_eventis never cleared, permanently disabling retries after one shutdown #4683 regression itself: a directly reused shut-down exporter makes 1 attempt on a 504, while the fresh default still makes all 3uv run pytest tests/test_trace_processor.py tests/tracingpasses (127 passed; the one unrelated failure istest_tracing_atexit_cleanup_timeout_preserves_process_exit_code_on_504, a 10s subprocess timeout that flakes on Windows under load and passes on repeat runs onmainand on this branch alike).ruff format --check,ruff check, andmypyare clean.