chore(runtime): refresh runtime metric identity and collector state - #19822
litianningdatadog wants to merge 1 commit into
Conversation
litianningdatadog
added
changelog/no-changelog
Circular import analysis
|
Dependency direction analysis
|
Codeowners resolved asResolved from the full PR diff against |
litianningdatadog
changed the title
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: 8a7385c | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-10-01 18:48:52 Comparing candidate commit 8a7385c in PR branch Found 0 performance improvements and 4 performance regressions! Performance is the same for 365 metrics, 9 unstable metrics, 4 known flaky benchmarks, 4 flaky benchmarks without significant changes.
|
d59e112 to
16a5332
Compare
d6a1546 to
335f6ac
Compare
16a5332 to
8dd7e8e
Compare
335f6ac to
2e625cf
Compare
cde3045 to
a0e3c42
Compare
2e625cf to
7d36a71
Compare
There was a problem hiding this comment.
Pull request overview
This PR ensures runtime-metrics platform tags that include runtime identity (notably runtime-id) are refreshed when the runtime identity changes (e.g., after a MicroVM /run-triggered identity refresh), so runtime metrics emitted afterward carry the new identity.
Changes:
- Register a runtime-id change listener in
RuntimeWorkerto rebuild cached platform tags when identity changes. - Refactor platform-tag construction into a helper and add an identity-refresh callback method.
- Add a subprocess test asserting
runtime-id:tags update afterruntime.refresh_identity().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ddtrace/internal/runtime/runtime_metrics.py |
Builds platform tags via a helper and hooks runtime-id changes to refresh cached _platform_tags. |
tests/runtime/test_runtime_metrics_api.py |
Adds a subprocess test covering platform-tag refresh behavior on identity refresh. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f7876cb to
b84f21b
Compare
b009343 to
7b3934b
Compare
156be45 to
c8b45b6
Compare
7b3934b to
707de15
Compare
c8b45b6 to
c641a12
Compare
707de15 to
75336e0
Compare
5c23fca to
30e6c88
Compare
75336e0 to
4fed6d7
Compare
30e6c88 to
e484bf7
Compare
4fed6d7 to
c037394
Compare
e484bf7 to
fb28096
Compare
c037394 to
e7d5175
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: e7d51759d3
ℹ️ 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-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The two most critical issues are: (1) a retry-idempotency gap in the identity-refresh coordinator — when any subscriber fails, already-completed callbacks (tracer writer recreation and runtime metrics reset) re-execute for the same runtime ID, dropping buffered traces and discarding legitimate metric deltas; and (2) a race between reset() and flush() in the runtime metrics collector that can emit negative or cross-identity deltas when an identity refresh overlaps a periodic flush.
🤖 Bits Code Review · Commit 3d4a374 · @DataDog review to ask questions
emmettbutler
left a comment
emmettbutler
left a comment
There was a problem hiding this comment.
Deferring review since the base branch is not main
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd2f295d2c
ℹ️ 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".
| runtime_id = get_runtime_id() | ||
| self._platform_tags = self._collect_platform_tags() | ||
| runtime_metrics = list(self._runtime_metrics) | ||
| if runtime_id == get_runtime_id(): |
There was a problem hiding this comment.
When the MicroVM hook rotates the runtime ID but an identity-refresh callback raises before this worker's callback runs—or this worker's own collector reset fails partway—the coordinator leaves callbacks pending for a later request while the new ID is already visible. A periodic flush between those attempts reaches this block with the old collector baselines, recollects the new runtime-id, and sends a mixed pre/post-refresh interval under that new ID. The before/after comparison cannot detect this because both reads occur while holding the refresh lock; track the ID associated with the collector baselines and reset before collection when it differs.
Useful? React with 👍 / 👎.
| with self._identity_refresh_lock: | ||
| self._runtime_metrics.reset() |
There was a problem hiding this comment.
On every successful MicroVM refresh, the runtime ID is rotated before the callbacks are invoked, and those callbacks come from an unordered set. If the tracer or telemetry rebuild runs before this callback, its CPU, context switches, and GC activity occur under the new identity but are discarded here; if this callback happens first, the same activity is retained. Runtime metrics therefore vary with callback iteration order and can omit initialization work from the new runtime, so the collector reset needs a defined position immediately at the identity boundary rather than an unordered subscriber callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e16f6204f3
ℹ️ 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".
| monitor.reset() | ||
| self._reset_state() |
There was a problem hiding this comment.
When a collection starts after monitor.reset() but before gc.get_stats() is sampled by _reset_state()—which can itself occur while allocating the returned stats/list—the monitor records that pause while the new collections baseline already includes the collection. The next flush therefore reports a positive GC pause with zero corresponding collection delta. Reset the pause window and reseed the collection baseline under one synchronization boundary so activity during the identity transition is either retained or discarded consistently.
Useful? React with 👍 / 👎.
| self._identity_refresh_lock = get_runtime_identity_refresh_lock() if self._identity_refresh_enabled else None | ||
| if self._identity_refresh_enabled: | ||
| on_runtime_identity_refresh(self._on_identity_refresh) |
There was a problem hiding this comment.
When the public RuntimeMetrics.enable() is called concurrently with a MicroVM /run refresh, the collectors are constructed and seed their CPU/GC baselines before the callback is registered here, without holding the refresh lock. If identity rotation lands in that window, the new worker misses the transition entirely; its first flush recollects the new runtime-id tag but reports deltas whose baselines came from the previous identity. Construct and register the worker state under the shared refresh lock, or verify the identity after registration and reseed when it changed.
Useful? React with 👍 / 👎.
When runtime-ID tagging is enabled, recollect platform tags during the periodic runtime-metrics flush instead of keeping the pre-refresh list, so an AWS Lambda MicroVM identity refresh is reflected on the next flush. Platform tags stay cached when runtime-ID tagging is disabled. A MicroVM /run transition also rotates the runtime ID mid-process without a fork, which left the CPU time, context-switch, wall-clock, and GC pause collectors mid-interval: their next sample would mix pre- and post-refresh activity, or (for a fresh process) reports the whole process lifetime as one delta. Reset those collectors' baselines on the existing runtime identity refresh callback, registered only for MicroVM processes with runtime-ID tagging enabled. Non-MicroVM processes and MicroVMs without runtime-ID tagging register nothing and are unaffected. This does not add another identity-refresh callback beyond the existing one, and does not change trace, writer, or telemetry-worker lifecycle behavior.
Stacked PRs:
Description
After a MicroVM identity refresh, runtime metrics must use the new runtime identity and
must not mix a pre-refresh measurement interval with post-refresh process activity. This
PR covers both halves of that:
Platform tags. When runtime-ID tagging is enabled, runtime metrics recollect platform
tags during the periodic runtime-metrics flush instead of keeping the tag list built before
/run, so a MicroVM identity refresh is reflected on the next flush. When runtime-IDtagging is disabled, platform tags stay cached (no behavior change).
Collector state. A MicroVM
/runtransition rotates the runtime ID mid-process withouta fork. Left alone, the CPU time, context-switch, wall-clock, and GC pause/collections
collectors would report a delta spanning both the old and new identity on their next sample.
RuntimeWorkernow resets those collectors' baselines through the existingon_runtime_identity_refreshcallback, registered only when the process is a MicroVM andruntime-ID tagging is enabled; it's unregistered on
stop(). Non-MicroVM processes andMicroVMs without runtime-ID tagging register nothing and pay no extra cost.
Refresh synchronization. The worker shares the fork-safe MicroVM refresh lock from #19939.
It holds that lock across collector reset and the full runtime-metrics flush, including tag
collection, sampling, and sending, so identity rotation cannot overlap an in-flight export.
The lock is not used by non-MicroVM or runtime-ID-disabled paths.
This does not add a second identity-refresh callback/guard, and does not touch trace,
writer, or telemetry-worker lifecycle code.
Reference
Testing
Added focused subprocess/unit coverage for:
RuntimeMetrics.reset())RuntimeWorker.stop()unregistering the hookGCRuntimeMetricCollector/NativeProcessMetricCollector.reset()discarding pre-refreshpause/collections and CPU-time/context-switch/wall-clock state
The previous test that forced refreshes to interleave inside a flush was replaced because the
shared lock makes that interleaving impossible; the new regression test verifies the intended
serialization boundary directly.
Validation:
scripts/run-tests --venv 16cc321 -- -k 'test_runtime_metrics_microvm_flush_serializes_identity_rotation or test_runtime_metrics_refresh_identity_updates_runtime_id_tag or test_runtime_metrics_microvm_identity_refresh_resets_collectors or test_runtime_metrics_non_microvm_identity_refresh_does_not_reset_collectors' tests/runtime/test_runtime_metrics_api.py(4 passed)scripts/lint fmt -- ddtrace/internal/runtime/runtime_metrics.py tests/runtime/test_runtime_metrics_api.pyscripts/lint style -- ddtrace/internal/runtime/runtime_metrics.py tests/runtime/test_runtime_metrics_api.pyscripts/lint typing -- ddtrace/internal/runtime/runtime_metrics.py tests/runtime/test_runtime_metrics_api.pygit diff --checkRisks
Limited to runtime-metrics platform-tag collection and the two metric collectors' interval
state. The shared lock adds synchronization only for MicroVMs with runtime-ID tagging; the
runtime-ID-disabled and non-MicroVM paths are unchanged. A flush now waits for an in-flight
MicroVM identity transition, and a transition waits for an in-flight metrics flush, preventing
partial identity attribution.