chore(tracer): tracer/writer identity refresh and stale-buffer reset - #19820
Conversation
litianningdatadog
added
changelog/no-changelog
litianningdatadog
changed the title
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 3cda934 | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-09-27 04:13:40 Comparing candidate commit 3cda934 in PR branch Found 0 performance improvements and 8 performance regressions! Performance is the same for 553 metrics, 10 unstable metrics, 7 known flaky benchmarks, 17 flaky benchmarks without significant changes.
|
d59e112 to
16a5332
Compare
3c0ea31 to
eabf5ec
Compare
16a5332 to
8dd7e8e
Compare
eabf5ec to
5853b4a
Compare
cde3045 to
a0e3c42
Compare
5853b4a to
7afb3c1
Compare
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
There was a problem hiding this comment.
Pull request overview
This PR updates the native trace writer so that when runtime identity is refreshed (e.g., AWS Lambda MicroVM /run refresh), the native exporter is rebuilt to pick up the new runtime id captured at exporter construction time.
Changes:
- Wire
NativeWriterto runtime-id change notifications and rebuild the exporter on identity refresh. - Add tests to verify the exporter is rebuilt (without recreating the writer/buffer) and that the wiring works end-to-end via
runtime.refresh_identity().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ddtrace/internal/writer/writer.py |
Subscribes to runtime-id change events and rebuilds the native exporter on identity refresh. |
tests/tracer/test_writer.py |
Adds unit + subprocess tests ensuring identity refresh rebuilds the exporter without replacing the writer’s client/buffer state. |
Suppressed comments (2)
ddtrace/internal/writer/writer.py:945
- _on_identity_refresh can raise if _create_exporter() fails (e.g., native builder/telemetry initialization error). Because runtime.refresh_identity() iterates callbacks without exception handling, a failure here will propagate out of refresh_identity() and can also prevent other runtime-id listeners from running. It’s safer to only swap the exporter after successfully creating the replacement, and to swallow/log failures from the rebuild path.
old_exporter = self._exporter
self._exporter = self._create_exporter()
try:
old_exporter.shutdown(3_000_000_000)
except Exception:
ddtrace/internal/writer/writer.py:944
- Exporter rebuild/swap is not synchronized with payload sending. NativeWriter.flush_queue() can run on the periodic thread and can also be called from other threads, while _on_identity_refresh swaps self._exporter and shuts down the old exporter. Without a lock, this introduces a race where a concurrent _exporter.send() can observe a half-transition or have its exporter shutdown mid-send.
Consider adding a dedicated lock (similar to HTTPWriter._conn_lck) and holding it in _send_payload(), _on_identity_refresh(), set_test_session_token(), and any other place that swaps/shuts down the exporter.
old_exporter = self._exporter
self._exporter = self._create_exporter()
try:
old_exporter.shutdown(3_000_000_000)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6332ba7 to
6231eed
Compare
2c58627 to
99e2b4e
Compare
6231eed to
6006fd6
Compare
676adf4 to
18c03c4
Compare
6006fd6 to
e1c70b6
Compare
18c03c4 to
a637bb2
Compare
e1c70b6 to
0d8f72e
Compare
a637bb2 to
0d53833
Compare
6e5799f to
75def6a
Compare
f94e6ae to
92a469d
Compare
litianningdatadog
removed
the
changelog/no-changelog
litianningdatadog
changed the title
litianningdatadog
added
the
changelog/no-changelog
92a469d to
5a48d93
Compare
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: 7f7d597465
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a8c91d0c5
ℹ️ 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".
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two of the most critical issues are: (1) on_span_finish in processor/__init__.py mutates trace aggregation state before checking the finishing span's generation, enabling stale spans to corrupt or prematurely export current-generation traces; and (2) start_span in tracer.py assigns the current generation to spans whose parent is a local Context (not a Span), allowing pre-refresh stale traces to pass generation checks and reach the replacement writer.
🤖 Bits Code Review · Commit 0ea9a2f · @DataDog review to ask questions
| # Capture the generation before processing so an identity refresh can invalidate this trace. | ||
| identity_generation = self._runtime_identity_generation | ||
| if identity_generation is not None and any( | ||
| finished_span._get_ctx_item(_RUNTIME_IDENTITY_GENERATION_KEY) != identity_generation |
There was a problem hiding this comment.
If an old span finishes after refresh and a new span shares its trace ID, on_span_finish increments the new trace's completion count before checking identity. With one unfinished new span buffered, this removes the trace and submits that unfinished span. The added check accepts it because it examines buffered spans rather than the stale finishing span. Check the finishing span's generation under the lock before modifying aggregation state.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| identity_generation = None | ||
| if self._span_aggregator._identity_refresh_enabled: | ||
| identity_generation = self._span_aggregator._capture_identity_generation() | ||
| if parent is not None: |
There was a problem hiding this comment.
Instrumented ThreadPoolExecutor work submitted before refresh carries a copied Context, not a Span. If it starts after refresh, parent is None here, so its spans receive the current generation while retaining the old trace and parent IDs. They pass the generation checks and reach the replacement writer despite belonging to the discarded trace. Preserve identity generations through local Context propagation while continuing to accept remote request contexts.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| for client in self._clients: | ||
| getattr(client.encoder, "flush")() | ||
| if not self._exporter_dropped: | ||
| self._drop_exporter() |
There was a problem hiding this comment.
During MicroVM refresh, TraceExporterPy.drop() discards its inner exporter without stopping workers registered with the shared runtime. NativeWriter's existing destructor documents that explicit shutdown is required, but setting _exporter_dropped bypasses the remaining cleanup paths. Old workers therefore remain registered alongside their replacements. Refresh needs a non-flushing worker cancellation path before discarding the exporter.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
An explicit AWS Lambda MicroVM identity refresh changes the runtime ID, but the tracer can still hold trace aggregation state and writer-buffered entities created under the previous identity. The native exporter also captures the runtime ID when it is built.
This change wires the global tracer to the explicit identity-refresh notification. On refresh, the tracer:
CIVisibilityWriterbefore installing its replacement during the transition, and drops the native exporter's pending stats through its non-flushingdrop()path instead of a flushing shutdown, so the transition neither leaks a periodic writer thread nor exports stale native metrics.encode()rather than exposingget().The refresh callback is registered only for explicit MicroVM identity refresh. The identity-transition lock is enabled only for aggregators initialized in MicroVM mode; normal configuration and post-fork recreation paths retain their existing behavior.
The stale-buffer reset previously proposed in #20089 is included here because it is part of the tracer and writer identity-refresh behavior.
Reference
Testing
Added focused coverage for:
Validation completed:
tests/tracer/runtime/test_runtime_id.pyandtests/tracer/test_writer.pyafter the latest round of fixes: 185 passed, 10 skipped, 1 xpassed, 0 failed.Risks
Pre-refresh trace data is intentionally discarded during explicit MicroVM identity refresh so it cannot be exported with a stale runtime ID. The transition-specific locking is limited to MicroVM identity-refresh state; standard non-MicroVM reset and post-fork paths remain unchanged.