Skip to content

fix(tracer): isolate MicroVM identity refresh - #19820

Draft
litianningdatadog wants to merge 1 commit into
mainfrom
tianning.li/3-3-trace-writer-identity-refresh
Draft

litianningdatadog wants to merge 1 commit into
mainfrom
tianning.li/3-3-trace-writer-identity-refresh

Conversation

@litianningdatadog

@litianningdatadog litianningdatadog commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Stacked PRs:

Description

An explicit AWS Lambda MicroVM identity refresh changes the runtime ID, but the tracer's aggregation state, writer buffers, native exporter and active request context can still belong to the previous logical runtime.

This change subscribes the global tracer to MicroVM identity-refresh notifications. During a refresh it:

  • Recreates the span-aggregator writer, so the new native exporter is built with the new runtime ID.
  • Discards the replaced writer's buffered traces without touching its encoder, and resets aggregation state. The encoder's lock is a plain threading.RLock, so after a fork it can still be held by a thread that no longer exists; the old buffer is abandoned along with the old writer.
  • Releases the replaced writer's native exporter with TraceExporter.drop() instead of the flushing shutdown(). drop() leaves the exporter's native workers running; see Risks.
  • Keeps network I/O out of the locks that span start/finish and the refresh wait on. Flushes no longer hold the writer lock across send(), whose retries take about 21 s against an unreachable agent. The send gate is rechecked under the exporter lock instead, so no send starts after a refresh closes it. In sync mode (AWS Lambda with the agent extension), a span finish only buffers the trace under the writer and span-aggregator locks; Tracer._on_span_finish sends it on the same thread after releasing the span-aggregator lock.
  • Never waits on an in-flight send. If a send holds the exporter lock, the thread running it drops the exporter once it releases the lock. That is the writer's own thread for an async writer, or the span-finishing or tracer.flush() thread for a sync writer, which never starts a thread. recreate() doesn't join the writer thread or shut down an exporter whose drop is still pending. A discarded writer's on_shutdown() and __del__ drop the exporter instead of shutting it down.
  • Skips the response callback for a send that finishes after the refresh, so a late rate_by_service can't overwrite rates the replacement writer received. A late 404/415 doesn't rebuild the exporter, and if the refresh wins while _downgrade() is building a replacement, the replacement is dropped instead of installed.
  • Serializes every span-aggregator writer swap (Tracer.configure(), agentless writer swaps, post-fork writer recreation) with the refresh. Otherwise a concurrent swap could install a writer whose exporter captured the previous runtime ID.
  • Tags spans with an identity generation. The aggregator checks it before each trace processor and again under its lock before writing, so an invalidated trace can't reach the replacement writer. A root span rejected in start_span() records its context's generation, so child_of=span.context is rejected too.
  • Stops spans that were already invalidated before trace.span_finish listeners and registered span processors run.
  • Detaches the snapshot-inherited context active in the thread handling /run, including a remote context. The /run hook fires before distributed-tracing header extraction, so a remote context active at refresh time belongs to the snapshot; extraction then activates the new request's incoming context.
  • Serializes the refresh with shutdown() on _post_fork_lock. shutdown() sets _shutting_down under that lock before stopping anything, so it waits for a refresh in flight and stops the writer that refresh installs, and a refresh that starts after shutdown builds nothing. The lock is reentrant, because the exit-signal handler can run shutdown() on the thread that is refreshing; if that handler returns, the refresh sees _shutting_down after the rebuild and stops the writer it just installed, shutting down the exporter of a NativeWriter that never started.
  • In MicroVM mode, shutdown(timeout) waits at most timeout (clamped at 0) for _post_fork_lock, then sets _shutting_down either way. Without the bound, an exit signal could deadlock: a refresh on another thread holds _post_fork_lock while waiting for the span-aggregator lock or the old writer's _writer_lock, which the thread running the signal handler holds. A refresh still running after the wait stops the writer it installs, through the same _shutting_down check. shutdown() without a timeout still waits as before. The span-start retry of a failed refresh (_ensure_post_fork_writer()) does the same in a MicroVM: it builds nothing once _shutting_down is set, and stops a writer it installs after that wait. Outside a MicroVM the post-fork rebuild is unchanged.
  • Preserves pending post-fork writer recreation on failure so the next span start retries it. The tracer metadata is stored in a finally around the rebuild, so a failed rebuild still records the new runtime ID. A Rust panic from the rebuild is logged instead of re-raised, because the refresh dispatcher only contains Exception and the panic would otherwise abort the /run request. The span-start retry contains its own failures in MicroVM mode: an Exception or Rust panic from the rebuild is logged (rate-limited) and the retry stays pending, so it doesn't fail the app's span start. KeyboardInterrupt and SystemExit still propagate, and outside MicroVM mode a failed post-fork rebuild still raises, as on main.

The identity-transition locks and callbacks are enabled only in MicroVM mode. Outside MicroVM mode the one change is that shutdown() takes _post_fork_lock once, so a shutdown racing a post-fork writer rebuild on another thread waits for it and then stops the rebuilt writer instead of leaving it running. The shutdown timeout doesn't bound that wait outside MicroVM mode.

The stale-buffer reset previously proposed in #20089 is included because it is part of the tracer and writer identity-refresh behavior.

Reference

Testing

  • MicroVM coverage that an invalidated span can't reach a finish listener or span processor.
  • MicroVM coverage that a snapshot-inherited remote context is detached, and that distributed-tracing headers extracted afterwards parent the new request correctly.
  • MicroVM coverage that a Tracer.configure() racing with the refresh can't leave a writer built for the pre-refresh runtime ID.
  • test_tracer_microvm_identity_refresh_records_generation_of_root_rejected_by_start_span forces a refresh between the identity read and the generation check in start_span(), then checks that child_of=span.context is rejected.
  • test_tracer_microvm_identity_refresh_after_shutdown_does_not_recreate_writer checks that a refresh after Tracer.shutdown() builds no writer.
  • test_tracer_shutdown_waits_for_in_flight_identity_refresh, test_tracer_identity_refresh_during_shutdown_does_not_recreate_writer and test_tracer_shutdown_on_refreshing_thread_stops_replacement_writer cover shutdown during a refresh, a refresh during shutdown, and shutdown run on the refreshing thread (no deadlock; the replacement writer is stopped, or has its exporter shut down if it never started). test_tracer_shutdown_waits_for_in_flight_post_fork_writer_recreation covers the same wait for a post-fork rebuild. Each fails without the change.
  • test_tracer_signal_shutdown_does_not_deadlock_with_refresh_waiting_on_writer_lock runs shutdown from a thread that holds the writer lock a refresh is waiting on (the cross-thread exit-signal case); shutdown returns after its timeout and the refresh then stops the writer it installed. test_tracer_timed_out_shutdown_stops_writer_from_retried_refresh_rebuild covers a timed-out shutdown during the span-start retry of a failed refresh, and test_tracer_post_fork_rebuild_ignores_shutdown_flag_outside_microvm checks that the post-fork rebuild is unchanged outside MicroVM mode. test_tracer_microvm_shutdown_accepts_negative_timeout covers the clamp, and test_tracer_shutdown_timeout_does_not_bound_post_fork_wait_outside_microvm checks that the timeout doesn't cut the post-fork wait short outside MicroVM mode.
  • test_tracer_microvm_identity_refresh_schedules_rebuild_after_recreate_failure and test_tracer_identity_refresh_contains_native_panic_from_writer_rebuild: a failed rebuild is left pending and still stores the metadata; RuntimeError and KeyboardInterrupt propagate, while a Rust panic doesn't escape refresh_identity(). test_tracer_microvm_identity_refresh_retries_failed_writer_recreation_on_next_span also checks that the stored metadata carries the refreshed runtime ID. test_tracer_microvm_failed_rebuild_retry_is_contained_in_start_span checks that, in MicroVM mode, a span-start retry that fails again with a RuntimeError or Rust panic doesn't raise and stays pending, and test_tracer_post_fork_rebuild_failure_still_raises_outside_microvm that the failure still raises outside it.
  • Writer coverage that the MicroVM drop path never calls into the inherited encoder, while the non-MicroVM path still clears it.
  • Writer coverage that a send stuck in retries blocks neither write() nor the refresh, that the send gate is rechecked under the exporter lock, that a replaced writer's thread drops its exporter without calling shutdown(), that stop() skips the join only for a replaced MicroVM writer, and that _downgrade() is skipped after a refresh.
  • Writer coverage that recreate() on a stopped sync writer doesn't wait for an in-flight send, that a flush_queue() sender drops the replaced exporter once its send ends, and that the exporter is dropped only once when several paths race to drop it.
  • test_native_writer_send_finishing_after_refresh_skips_response_callback and test_native_writer_del_drops_exporter_when_refresh_lost_lock_to_non_sender cover the late response and the __del__ drop.
  • End-to-end MicroVM test: with the exporter stuck in send(), a background span finish, an unrelated span start and refresh_identity() each complete in under 1 s.
  • Sync-mode version of that test: while a span finish's own send is stuck, an unrelated span start and refresh_identity() each complete in under 1 s, and the finishing thread then drops the replaced exporter without calling shutdown().
  • Manually checked with the real libdatadog exporter against an agent that accepts connections and never answers: an unrelated span start and finish took 21.0 s before the writer change and 0.00 s after, the /run refresh took 0.00 s, and the agent received only the in-flight payload's 6 retry attempts.
  • scripts/run-tests -r --venv ed437ab -- tests/tracer/test_writer.py tests/tracer/runtime/test_runtime_id.py tests/tracer/test_processors.py tests/tracer/test_tracer.py: 393 passed, 13 skipped, 1 xpassed. Pass -r after rebasing onto a newer main; a stale native build fails 18 of these tests with 'Span' object has no attribute '_override_sampling_decision'. test_trace_top_level_span_processor_partial_flushing and test_tracer_shutdown_timeout need a trace agent on localhost:8126, which CI provides; run docker compose up -d ddagent first when testing locally.
  • Use a tracer venv, not tracer-python-optimize, for these files: PYTHONOPTIMIZE=1 strips the asserts inside @pytest.mark.subprocess tests, so failures there report as passes.
  • After the shutdown change: scripts/run-tests --venv 128b106 -- tests/tracer/test_tracer.py tests/tracer/test_writer.py tests/tracer/runtime/test_runtime_id.py: 351 passed, 13 skipped, 1 xpassed.
  • After the panic, metadata and bounded-shutdown changes: scripts/run-tests --venv 128b106 -- tests/tracer/runtime/test_runtime_id.py tests/tracer/test_tracer.py tests/tracer/test_writer.py -k "test_runtime_id or test_writer or shutdown or post_fork": 242 passed, 10 skipped, 1 xpassed.
  • scripts/lint fmt and scripts/lint typing passed on the changed files.

Risks

Pre-refresh trace data is intentionally discarded during a MicroVM identity refresh so it can't be exported under a stale runtime ID. The transition-specific locking is limited to MicroVM identity-refresh state.

Known gap: TraceExporter.drop() doesn't stop the workers the replaced exporter registered on the shared native runtime, and the libdatadog version this PR pins (v43.0.2) has no discard-only stop. After a refresh, the old exporter's client-side stats worker keeps running. It sends the pre-refresh stat buckets on its next flush interval (10 s by default) and the rest at process exit, under the snapshotted runtime ID, so every MicroVM restored from the same image reports the same buckets. The old /info fetcher also keeps polling the agent, as do the dogstatsd worker (health metrics) and the exporter-owned telemetry worker (only used without a shared telemetry worker) when enabled. There is at most one extra set of workers per process, because /run refreshes identity once. Client-side stats are on by default only on Python 3.14+ or in agentless mode; DD_TRACE_STATS_COMPUTATION_ENABLED=false avoids the duplicate stats, except for agentless stats and OTLP trace metrics, which always use the native concentrator. Closing the gap needs a libdatadog exporter shutdown that discards without flushing (DataDog/libdatadog#2478). Calling the normal shutdown() instead, even from a background thread, would send the same stale stats.

Contexts this process never tracked are treated as current. That includes a remote or hand-built Context that survived the snapshot without being activated, used as a parent, or copied into a context that was used as a parent, and a snapshot-inherited Context still active in a thread or asyncio task other than the one handling /run. Spans parented to one are still created after the refresh, so they carry the new runtime ID and go through the new writer. Rejecting untracked contexts instead would drop legitimate post-refresh traffic that never passes through activate_distributed_headers (SQS/Kafka/Pub/Sub consumers, OTel remote parents, manual propagation).

The finish-time generation check before trace.span_finish doesn't take a lock. A span invalidated by a refresh that races with its finish can still reach finish listeners and span processors; the aggregator re-checks under its lock, so its trace is still not exported.

In MicroVM mode, a writer swap outside a refresh (such as Tracer.configure()) holds the span-aggregator lock while the old writer flushes, so span start/finish wait for that flush, including its send retries if the agent is unreachable. These swaps are rare, and ordinary periodic flushes no longer block span start/finish. If such a swap overlaps the runtime-ID rotation, the published runtime ID can briefly be newer than the one its writer was built with, until the refresh callback replaces that writer.

LogWriter (MicroVM Lambda without the agent extension) writes to stdout under the span-aggregator lock in MicroVM mode, so the generation check and the write are atomic. A stalled stdout delays span finishes and /run while it lasts.

A payload already in send() when a refresh starts still runs its retries to completion on the thread that started it; nothing else waits on it, and no later payload from that writer is sent. For a sync writer, that thread is the span-finishing or tracer.flush() thread, so that one call still takes as long as the retries.

In the exit-signal deadlock case, a MicroVM shutdown(timeout) can take up to about 2 × timeout: the bounded wait for an in-flight refresh, then the normal flush. If it gives up waiting while the span-start retry of a failed refresh (_ensure_post_fork_writer()) holds the lock, that retry stops the writer it installs. A Rust panic from the native metadata write still reaches /run, as it does from __init__ and after a fork; the write runs after the rebuild, so it can't skip it.

In MicroVM sync mode, the deferred send is drained by Tracer._on_span_finish. Code that calls SpanAggregator.on_span_finish directly, without going through the tracer, has its trace sent on that thread's next traced span finish instead.

Unchanged from main, and not specific to the refresh: an exit signal that arrives while a writer rebuild is inside the old writer's stop() deadlocks on that writer's _service_lock, as it does for Tracer.configure() and the post-fork rebuild; and, outside MicroVM mode, a post-fork rebuild interrupted by a same-thread shutdown whose signal handler returns can still install a writer after shutdown.

@litianningdatadog litianningdatadog added changelog/no-changelog A changelog entry is not required for this PR. aws-microvm Work related to AWS MicroVM onboarding labels Aug 23, 2026
@litianningdatadog litianningdatadog changed the title fix(tracer): rebuild native writer exporter on identity refresh chore(tracer): rebuild native writer exporter on identity refresh Aug 23, 2026
@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Aug 23, 2026 •

Copy link
Copy Markdown
Contributor

Pipelines  Tests

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 2 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-py | debugging/debugger — 🔄 Retry may pass, looks flaky

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-py | download win_arm64 wheels

View more details · View in GitLab

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🚧 1 test that failed was ignored due to quarantine View in Datadog

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d88408b | Docs | View more details | Give us feedback!

@pr-commenter

pr-commenter Bot commented Aug 23, 2026 •

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-10-16 14:28:30

Comparing candidate commit d88408b in PR branch tianning.li/3-3-trace-writer-identity-refresh with baseline commit 2ad3da7 in branch main.

📊 Benchmarking dashboard

Found 0 performance improvements and 4 performance regressions! Performance is the same for 352 metrics, 9 unstable metrics, 4 known flaky benchmarks, 4 flaky benchmarks without significant changes.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:httppropagationextract-tracecontext_headers

  • 🟥 execution_time [+2.709µs; +2.976µs] or [+8.603%; +9.452%]

scenario:msgpackencoderscenario-simple_one_span

  • 🟥 execution_time [+533.046ns; +601.464ns] or [+13.809%; +15.581%]

scenario:otelspan-start

  • 🟥 execution_time [+2.768ms; +3.602ms] or [+10.695%; +13.919%]

scenario:recursivecomputation-shallow

  • 🟥 execution_time [+63.120µs; +66.798µs] or [+9.058%; +9.586%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:coreapiscenario-context_with_data_listeners

  • unstable execution_time [-738.486ns; +734.146ns] or [-7.073%; +7.031%]

scenario:coreapiscenario-core_dispatch_1_listener

  • unstable execution_time [-38.111ns; +40.318ns] or [-5.753%; +6.086%]

scenario:coreapiscenario-core_dispatch_50_listeners

  • unstable execution_time [-2119.702ns; +1736.158ns] or [-10.566%; +8.654%]

scenario:coreapiscenario-core_dispatch_exception_listeners

  • unstable execution_time [-2009.368ns; +1640.628ns] or [-10.474%; +8.552%]

scenario:coreapiscenario-core_dispatch_listeners

  • unstable execution_time [-372.163ns; +386.444ns] or [-8.755%; +9.091%]

scenario:coreapiscenario-core_dispatch_no_args_listeners

  • unstable execution_time [-219.792ns; +236.541ns] or [-8.208%; +8.834%]

scenario:coreapiscenario-core_dispatch_with_results_1_listener

  • unstable execution_time [-109.765ns; +76.683ns] or [-8.008%; +5.595%]

scenario:coreapiscenario-core_dispatch_with_results_50_listeners

  • unstable execution_time [-4132.741ns; +5231.222ns] or [-8.583%; +10.864%]

scenario:coreapiscenario-core_dispatch_with_results_listeners

  • unstable execution_time [-891.526ns; +1000.859ns] or [-8.731%; +9.801%]

Known flaky benchmarks

These benchmarks are marked as flaky and will not trigger a failure. Modify FLAKY_BENCHMARKS_REGEX to control which benchmarks are marked as flaky.

scenario:httppropagationinject-ids_only

  • 🟥 execution_time [+2.872µs; +2.972µs] or [+21.539%; +22.289%]

scenario:span-start

  • 🟥 execution_time [+1.507ms; +1.799ms] or [+13.646%; +16.282%]

scenario:telemetryaddmetric-1-count-metric-1-times

  • 🟥 execution_time [+161.633ns; +194.376ns] or [+8.245%; +9.916%]

scenario:tracer-small

  • 🟥 execution_time [+48.989µs; +50.320µs] or [+20.207%; +20.755%]

Known flaky benchmarks without significant changes:

  • scenario:errortrackingflasksqli-baseline
  • scenario:flasksimple-iast-get
  • scenario:sethttpmeta-all-enabled
  • scenario:telemetryaddmetric-record-100-metrics

@litianningdatadog
litianningdatadog force-pushed the tianning.li/2-flask-web-request-starting-event branch from d59e112 to 16a5332 Compare August 24, 2026 02:23
@litianningdatadog
litianningdatadog force-pushed the tianning.li/3-3-trace-writer-identity-refresh branch from 3c0ea31 to eabf5ec Compare August 24, 2026 02:24
@litianningdatadog
litianningdatadog force-pushed the tianning.li/2-flask-web-request-starting-event branch from 16a5332 to 8dd7e8e Compare August 24, 2026 02:31
@litianningdatadog
litianningdatadog force-pushed the tianning.li/3-3-trace-writer-identity-refresh branch from eabf5ec to 5853b4a Compare August 24, 2026 02:31
@litianningdatadog
litianningdatadog force-pushed the tianning.li/2-flask-web-request-starting-event branch 5 times, most recently from cde3045 to a0e3c42 Compare August 24, 2026 23:56
@litianningdatadog
litianningdatadog force-pushed the tianning.li/3-3-trace-writer-identity-refresh branch from 5853b4a to 7afb3c1 Compare August 25, 2026 13:23
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Codeowners resolved as

Resolved from the full PR diff against main using the target branch CODEOWNERS file.
CODEOWNERS team requests not listed below are not required by the current file set.

ddtrace/_trace/context.py                                               @DataDog/apm-sdk-capabilities
ddtrace/_trace/processor/__init__.py                                    @DataDog/apm-sdk-capabilities
ddtrace/_trace/span.py                                                  @DataDog/apm-sdk-capabilities @DataDog/apm-core-python
ddtrace/_trace/tracer.py                                                @DataDog/apm-sdk-capabilities
ddtrace/internal/writer/writer.py                                       @DataDog/apm-core-python
ddtrace/trace/__init__.py                                               @DataDog/apm-sdk-capabilities
releasenotes/notes/fix-aws-lambda-microvm-stale-traces-a26d7bf374c1b2c2.yaml  @DataDog/apm-python
tests/tracer/runtime/test_runtime_id.py                                 @DataDog/apm-sdk-capabilities
tests/tracer/test_tracer.py                                             @DataDog/apm-sdk-capabilities
tests/tracer/test_writer.py                                             @DataDog/apm-sdk-capabilities

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Circular import analysis

⚠️ Existing circular imports

There are 1 circular imports that already exist on the base branch and have not been changed by this PR.

ddtrace.errortracking._handled_exceptions.bytecode_injector -> ddtrace.errortracking._handled_exceptions.callbacks -> ddtrace.errortracking._handled_exceptions.collector -> ddtrace.errortracking._handled_exceptions.bytecode_reporting -> ddtrace.errortracking._handled_exceptions.bytecode_injector

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

Dependency direction analysis

⚠️ Existing dependency direction violations

There are 163 dependency direction violations that already exist on the base branch and have not been changed by this PR.

Show existing violations (showing 5 of 163 highest severity)
ddtrace.internal.tracemethods -×-> ddtrace.trace  (internal-core -> product:tracing, score=121)
ddtrace.llmobs._telemetry -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=119)
ddtrace.appsec._listeners -×-> ddtrace.trace  (product:appsec -> product:tracing, score=119)
ddtrace.llmobs._integrations.vertexai -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=119)
ddtrace.llmobs._integrations.bedrock -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=119)

To see all violations, download the layers-base.json and layers-pr.json artifacts from this CI job and run:

uv run --script scripts/import-analysis/layers.py compare layers-base.json layers-pr.json

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 NativeWriter to 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.

Comment thread ddtrace/internal/writer/writer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/_trace/tracer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/internal/writer/writer.py Outdated
Comment thread ddtrace/_trace/context.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/tracer.py Outdated
Comment thread ddtrace/_trace/processor/__init__.py Outdated
Comment thread ddtrace/_trace/tracer.py
Comment thread ddtrace/_trace/tracer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/trace/__init__.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/tracer.py Outdated
Comment thread ddtrace/internal/writer/writer.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/internal/writer/writer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/processor/__init__.py Outdated
Comment thread ddtrace/internal/writer/writer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread ddtrace/_trace/tracer.py Outdated

@datadog-datadog-prod-us1-2 datadog-datadog-prod-us1-2 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Synchronous NativeWriter sends can still hold or reacquire identity-transition locks for the full network retry duration, allowing an unreachable agent to stall MicroVM refresh and unrelated span operations.

Open Bits AI session

🤖 Bits Code Review · Commit 38c8ce2 · @DataDog review to ask questions

Comment thread ddtrace/internal/writer/writer.py
Comment thread ddtrace/internal/writer/writer.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20964893b3

ℹ️ 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".

Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/internal/writer/writer.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 40629b6022

ℹ️ 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".

Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/_trace/tracer.py
Comment thread ddtrace/_trace/processor/__init__.py Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f79af32e93

ℹ️ 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".

Comment thread ddtrace/_trace/tracer.py
Comment thread ddtrace/_trace/processor/__init__.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1717155a9a

ℹ️ 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".

Comment thread ddtrace/internal/writer/writer.py
Comment thread ddtrace/internal/writer/writer.py

@datadog-datadog-prod-us1-2 datadog-datadog-prod-us1-2 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Two of the most critical issues are resource leaks during refresh: the replaced CIVisibilityWriter's periodic service is never stopped (retaining its thread, encoder, and connection), and discarded TraceExporter native background workers are neither cancelled nor unregistered. Additionally, several generation-check race conditions exist in tracer.py where span identity is validated once but not re-checked across finish processors, at root-span return time, or at remote-context activation, allowing stale spans or contexts to corrupt post-refresh state.

Open Bits AI session

🤖 Bits Code Review · Commit 63940cd

Comment thread ddtrace/_trace/tracer.py
Comment thread ddtrace/_trace/processor/__init__.py
Comment thread ddtrace/internal/writer/writer.py
Comment thread ddtrace/_trace/tracer.py
Comment thread ddtrace/_trace/tracer.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbb36dab92

ℹ️ 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".

Comment thread ddtrace/trace/__init__.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fdd54edfe2

ℹ️ 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".

Comment thread ddtrace/internal/writer/writer.py
Comment thread ddtrace/_trace/tracer.py

@datadog-datadog-prod-us1-2 datadog-datadog-prod-us1-2 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The two most critical issues are a race condition in writer.py where a discarded writer's response callback can overwrite the replacement writer's sampling rates (findings 1 & 3), and a deadlock risk where discarded writers remain subscribed to telemetry worker-change callbacks, blocking span operations while holding the aggregator lock. Additional distinct issues include missing writer-recreation retry logic on MicroVM refresh failure and stale parent-context generation tracking across remote.copy() owners.

Open Bits AI session

🤖 Bits Code Review · Commit 9c5b471

Comment thread ddtrace/_trace/tracer.py Outdated
Comment thread ddtrace/internal/writer/writer.py
Comment thread ddtrace/_trace/context.py
Comment thread ddtrace/internal/writer/writer.py
@litianningdatadog

Copy link
Copy Markdown
Contributor Author

/code blockers

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-10-13 18:58:55 UTC ℹ️ Start processing command /code blockers


2026-10-13 18:58:56 UTC ℹ️ Devflow:

Checking merge blockers for #19820...


2026-10-13 18:58:57 UTC ℹ️ Devflow: /code blockers

Detected 8 merge blocker(s) to address:

🔴 Blocking

  • Required check dd-gitlab/default-pipeline has not reported
    Hint: Ensure the workflow that produces this check is triggered.
  • Required check system-tests finished has not reported
    Hint: Ensure the workflow that produces this check is triggered.
  • This pull request has unresolved conversations.
    5 conversations are not resolved in the changes view. Outdated conversation ddtrace/_trace/tracer.py is not resolved.
    Hint: Resolve every conversation before merging.

🟠 Pending

  • Required check Validate changelog is running

  • Required check Validate changelog is running

  • Merge gate rule reviewers-approval is running: 2 approvals missing
    All required reviewers must approve this pull request before it can be merged. Learn more in our FAQ.

    Hint: Questions about this check? Reach out in #dx-source-code-management on Slack.

  • Merge gate rule reviewers-approval is running: 2 approvals missing
    All required reviewers must approve this pull request before it can be merged. Learn more in our FAQ.

    Hint: Questions about this check? Reach out in #dx-source-code-management on Slack.

  • Required check pr_name_lint is running

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 436c374e51

ℹ️ 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".

Comment thread ddtrace/_trace/tracer.py Outdated
Comment on lines +461 to +465
except BaseException:
# /run is not repeated, so a failed rebuild would leave the discarded writer installed.
# Retry it on the next span start, like a post-fork rebuild.
self._post_fork_writer_pending = True
raise

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Isolate native rebuild panics from the /run request

When exporter reconstruction raises a PyO3 panic (a BaseException, which this handler explicitly anticipates), re-raising it bypasses the Exception-only guard in _notify_runtime_identity_refresh_callbacks; the event hub also always propagates BaseException. Because the callback runs inline during WSGI/ASGI request-start dispatch, the MicroVM /run invocation is aborted instead of proceeding to the first span, where the newly set pending flag could retry reconstruction. Keep the pending marker, but isolate recognized native panics on the normal request-hook path while preserving explicit raise_on_error behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e97992f. The rebuild still marks itself pending first, but a Rust panic (is_panic_exception) is now logged instead of re-raised, so it no longer gets past the refresh dispatcher or the event hub into /run, and the first span retries the rebuild. KeyboardInterrupt, SystemExit and ordinary exceptions still propagate. test_tracer_identity_refresh_contains_native_panic_from_writer_rebuild drives this through refresh_identity().

One difference from your suggestion: the panic is contained inside the tracer callback, so a direct refresh_identity(raise_on_error=True) caller won't see it either. It's still logged and the rebuild is still retried. Nothing on the /run path calls with raise_on_error=True, so I kept the callback simple instead of threading that flag through.

@litianningdatadog

Copy link
Copy Markdown
Contributor Author

/code blockers

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-10-13 19:11:13 UTC ℹ️ Start processing command /code blockers


2026-10-13 19:11:14 UTC ℹ️ Devflow:

Checking merge blockers for #19820...


2026-10-13 19:11:15 UTC ℹ️ Devflow: /code blockers

Detected 5 merge blocker(s) to address:

🔴 Blocking

  • Required check system-tests finished has not reported
    Hint: Ensure the workflow that produces this check is triggered.
  • This pull request has unresolved conversations.
    1 conversation is not resolved in the changes view.
    Hint: Resolve every conversation before merging.

🟠 Pending

  • Required check dd-gitlab/default-pipeline is running

  • Merge gate rule reviewers-approval is running: 2 approvals missing
    All required reviewers must approve this pull request before it can be merged. Learn more in our FAQ.

    Hint: Questions about this check? Reach out in #dx-source-code-management on Slack.

  • Merge gate rule reviewers-approval is running: 2 approvals missing
    All required reviewers must approve this pull request before it can be merged. Learn more in our FAQ.

    Hint: Questions about this check? Reach out in #dx-source-code-management on Slack.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bc81a06965

ℹ️ 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".

Comment thread ddtrace/_trace/tracer.py
identity_generation = parent_generation
else:
_set_runtime_identity_generation(child_of, identity_generation)
span._set_ctx_item(_RUNTIME_IDENTITY_GENERATION_KEY, identity_generation)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Record contexts created while tracing is disabled

When a MicroVM snapshot contains a span created while tracer.enabled is false, this stamps only the span: the conditional processor dispatch below skips SpanAggregator.on_span_start(), so the span's Context never enters the generation side table. If tracing is enabled after /run and the application reuses span.context as child_of, _get_runtime_identity_generation() returns None, line 686 promotes it to the current generation, and a child of the pre-snapshot trace can be exported. Fresh evidence beyond the earlier untracked-context discussion is that this is a tracer-created context that becomes untracked specifically on the disabled-tracing path; record its generation here regardless of whether processors run.

Useful? React with 👍 / 👎.

On a MicroVM identity refresh, drop buffered traces and the old native
exporter without flushing them, and rebuild the writer for the new
runtime ID, so traces from before the refresh are never sent or used as
parents afterwards.

- SpanAggregator publishes one (generation, runtime ID) value. Span start
  and finish read it without a lock; the write re-checks it under the
  aggregator lock as the single commit point.
- Root spans take runtime-id from that value, so a span started after the
  ID rotated but before the tracer refreshed never carries the new ID to
  the old writer.
- SpanAggregator uses forksafe.RLock only in MicroVM; others keep RLock.
- drop() avoids shutdown()'s final stats flush but does not stop the
  exporter's native workers, so the replaced exporter's stats worker can
  still send pre-refresh stats later.
- The refresh and shutdown() serialize on _post_fork_lock, so a writer
  built by a refresh is always the one shutdown stops, and a refresh that
  starts after shutdown builds nothing. The lock is reentrant because the
  exit-signal handler can run shutdown() on the refreshing thread; if that
  handler returns, the refresh stops the writer it just installed.
- In a MicroVM, shutdown(timeout) waits at most timeout for that lock,
  because a refresh on another thread can hold it while waiting for an
  aggregator or writer lock held by the thread the exit signal
  interrupted. A refresh, or the span-start retry of a failed one, still
  running after the wait stops the writer it installs.
- The refresh stores the tracer metadata whether or not the writer
  rebuild succeeded, and a Rust panic from the rebuild is logged instead
  of aborting the /run request; the rebuild is retried on the next span.
  That retry also logs an exception or Rust panic from the rebuild in
  MicroVM mode and keeps the retry pending, instead of raising it from the
  app's span start.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws-microvm Work related to AWS MicroVM onboarding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants