fix(tracer): isolate MicroVM identity refresh - #19820
litianningdatadog wants to merge 1 commit into
Conversation
litianningdatadog
added
changelog/no-changelog
litianningdatadog
changed the title
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: a23e56e | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-10-01 18:50:36 Comparing candidate commit a23e56e in PR branch Found 0 performance improvements and 8 performance regressions! Performance is the same for 577 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.
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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b29531ab07
ℹ️ 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: eb487b59fa
ℹ️ 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: b585e1b387
ℹ️ 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: 94d7001195
ℹ️ 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 identity_generation is not None and not self._identity_generation_is_current(identity_generation): | ||
| return | ||
| try: | ||
| result = tp.process_trace(spans) or [] |
There was a problem hiding this comment.
If identity refresh begins after this generation check but while tp.process_trace() is executing, a side-effecting processor can still publish stale data; for example, LLMObsProcessor routes events during the call and CIVisibilitySpanForwarder writes immediately. Fresh evidence beyond the earlier comment is the remaining unlocked gap between the check and the entire processor invocation, so checking only before each processor does not prevent the processor currently running from exporting after the generation changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I narrowed this to the trace-writer invariant for this PR: stale APM traces must not be exported after refresh begins. The LLMObs/CI examples use product-specific processors outside the RFC MVP scope for #19820, so I did not serialize arbitrary product/custom processors here.
| if in_aws_lambda_microvm(): | ||
| from ddtrace.internal._runtime_id import on_runtime_identity_refresh | ||
|
|
||
| on_runtime_identity_refresh(tracer._refresh_runtime_identity) |
There was a problem hiding this comment.
In a MicroVM application that creates a public Tracer() instance instead of using the singleton, this registers only the module-level tracer for refresh. The /run hook therefore rotates the process-wide runtime ID while the custom tracer retains its inherited trace buffer, generation, and identity-bound native exporter; subsequent spans can carry the new runtime tag through an exporter configured with the snapshot identity. Use lifecycle-safe registration that covers every live tracer rather than only this singleton.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this scoped to the module-level tracer. Multiple manually-created public Tracer() instances are already unsupported (Tracer.__init__ logs an error after the singleton exists), and the MicroVM MVP wiring in this stack attaches the refresh callback to the global tracer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701fe64544
ℹ️ 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: 3a74cd2e33
ℹ️ 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".
| # Only explicit MicroVM refreshes use discard-and-recreate semantics. | ||
| identity_refresh = drop_buffered_traces and not flush_writer and self._identity_refresh_enabled | ||
| if identity_refresh: | ||
| with self._lock: |
There was a problem hiding this comment.
When a MicroVM forks while another application thread owns SpanAggregator._lock, the child inherits this raw ddtrace.internal.threads.RLock in the locked state. The first /run refresh then blocks forever on this newly added with self._lock, because _child_after_fork() resets the trace buffer but never replaces the lock. Fresh evidence beyond the earlier generation-table issue is that the aggregator's separate lock remains non-fork-safe; use forksafe.RLock() or recreate it in the child hook.
AGENTS.md reference: AGENTS.md:L135-L135
Useful? React with 👍 / 👎.
| if self._writer_lock is not None and not self._accepting_writes: | ||
| return | ||
| try: | ||
| self._send_payload(encoded, n_traces, client) |
There was a problem hiding this comment.
When a periodic flush passes this _accepting_writes check just before refresh sets the flag to false, it still calls _send_payload() with the stale payload. drop_buffered_traces() changes the flag outside _writer_lock and then waits for that lock, so the active flush can send—and potentially retry—after runtime-ID rotation before releasing the lock. Fresh evidence beyond the earlier active-flush thread is this remaining check-to-send gap; the transition must invalidate the payload atomically with the send rather than relying on this pre-send check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b50c3b38
ℹ️ 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".
| identity_refresh = drop_buffered_traces and not flush_writer and self._identity_refresh_enabled | ||
| if identity_refresh: | ||
| with self._lock: |
There was a problem hiding this comment.
When an AppSec remote-config callback calls Tracer.configure() concurrently with the first MicroVM /run, its normal reset path at lines 667-674 does not acquire this lock. It can begin recreating the old writer before the runtime ID rotates, then assign that old-identity writer after this branch has installed the refreshed writer; subsequent current-generation traces are consequently exported with stale runtime metadata. The transition lock must cover normal writer swaps as well as this identity-refresh branch.
Useful? React with 👍 / 👎.
| with writer_lock: | ||
| for client in self._clients: | ||
| getattr(client.encoder, "flush")() |
There was a problem hiding this comment.
In a MicroVM that forks while another application thread is inside encoder.put() or encoder.flush(), the writer's new fork-safe lock is reset in the child but the msgpack encoder's own raw _thread.RLock is not. The first /run refresh therefore reaches this flush() and blocks forever on a lock owned by a vanished parent thread. Recreate/reset the encoder lock in the child, or discard the inherited encoder without acquiring it.
AGENTS.md reference: AGENTS.md:L135-L135
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a8535645
ℹ️ 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".
| else: | ||
| _set_runtime_identity_generation(child_of, cast(int, identity_generation)) |
There was a problem hiding this comment.
When a Context was created before the snapshot but was never active or used to start a span, it has no side-table entry; after refresh, passing that retained context as child_of reaches this fallback and labels it with the current generation, so the span is accepted and exported as a child of the stale pre-refresh trace. Fresh evidence beyond the earlier retained-context comment is this explicit promotion of every untracked context to the current generation; creation/extraction needs to record the generation so pre-refresh and newly extracted contexts can be distinguished.
Useful? React with 👍 / 👎.
Stacked PRs:
Description
An explicit AWS Lambda MicroVM identity refresh changes the runtime ID, but tracer aggregation state, writer buffers, native exporter state, and active request context may still belong to the prior logical runtime.
This change subscribes the global tracer to explicit MicroVM identity-refresh notifications. During refresh it:
trace.span_finishlisteners and registered span processors run./runhook fires before distributed-tracing header extraction, so any active remote context at refresh time belongs to the snapshot; extraction then activates the new request's real incoming context.The identity-transition lock and callbacks are enabled only in MicroVM mode. Normal non-MicroVM and post-fork behavior remains unchanged.
The stale-buffer reset previously proposed in #20089 is included because it is part of the tracer and writer identity-refresh behavior.
Reference
Testing
scripts/run-tests -s --venv 4fcf978 -- tests/tracer/runtime/test_runtime_id.py— 46 passed.scripts/lint fmt,scripts/lint typing, andscripts/lint checkspassed.Risks
Pre-refresh trace and telemetry data is intentionally discarded during an explicit MicroVM identity refresh so it cannot be exported under a stale runtime ID. The transition-specific locking is limited to MicroVM identity-refresh state.