Repository navigation
fix(id): reset OpenTelemetry and runtime state on refresh - #10324
litianningdatadog wants to merge 1 commit into
Conversation
Overall package sizeSelf size: 8.96 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.5.1 | 127.66 kB | 513.61 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🔄 Datadog retried 1 test - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 741937c | Docs | View more details | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tianning.li/1-dogstatsd-exporter-refresh-resets #10324 +/- ##
================================================================================
Coverage 98.71% 98.71%
================================================================================
Files 1025 1025
Lines 160673 160770 +97
Branches 74 74
================================================================================
+ Hits 158610 158707 +97
Misses 2063 2063
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues remain around in-flight telemetry and runtime-client state, with additional shutdown cleanup gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds identity-refresh resets for OpenTelemetry queues and runtime metric baselines after MicroVM refreshes.
Changes:
- Resets queued logs, metrics, and runtime sampler baselines.
- Subscribes telemetry components to identity refresh events.
- Adds regression tests for refresh behavior.
File summaries
| File | Reviewed changes and final notes |
|---|---|
packages/dd-trace/test/runtime_metrics.spec.js |
Adds runtime refresh coverage. |
packages/dd-trace/test/opentelemetry/metrics.spec.js |
Adds metric queue reset coverage. |
packages/dd-trace/test/opentelemetry/logs.spec.js |
Adds log queue reset coverage. |
packages/dd-trace/src/runtime_metrics/runtime_metrics.js |
Resets runtime samplers. Moderate (1 vote): native sampler state is not cleared on refresh. |
packages/dd-trace/src/runtime_metrics/otlp_runtime_metrics.js |
Resets OTLP runtime sampler state. |
packages/dd-trace/src/runtime_metrics/client.js |
Adds refresh handling. Critical (1 vote): pending aggregation and the wrapped DogStatsD buffer may survive refresh when runtime tags are unchanged. |
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js |
Clears pending metric state. Critical (1 vote each): in-flight batches can export pre-refresh data, noted at lines 241 and 244. Nits (1 vote each): clear stale dropped-count state, and strengthen the baseline-reset test. |
packages/dd-trace/src/opentelemetry/metrics/index.js |
Subscribes metrics to refreshes. Moderate (1 vote): shutdown does not unsubscribe the refresh listener. |
packages/dd-trace/src/opentelemetry/logs/index.js |
Subscribes logs to refreshes. Moderate (1 vote): shutdown does not unsubscribe the refresh listener. |
packages/dd-trace/src/opentelemetry/logs/batch_log_processor.js |
Clears pending log records. Critical (1 vote): in-flight exports can complete with pre-refresh data. |
Review details
Suppressed comments (6)
packages/dd-trace/src/opentelemetry/logs/index.js:79
- This listener is only removed when
initializeOpenTelemetryLogsruns again. After the publicLoggerProvider.shutdown()or global logs disable path, the channel still strongly retains the processor/exporter through this closure and invokes it on later refreshes, so a stopped pipeline cannot be collected. Unsubscribe it as part of provider shutdown rather than only on re-init.
// A clone resume shouldn't export log records queued before the snapshot under its own identity.
unsubscribeLogsPendingStateReset?.()
const onIdentityRefresh = () => processor.resetPendingState()
identityRefreshChannel.subscribe(onIdentityRefresh)
unsubscribeLogsPendingStateReset = () => identityRefreshChannel.unsubscribe(onIdentityRefresh)
packages/dd-trace/src/opentelemetry/metrics/index.js:84
- This listener is only removed when
initializeOpenTelemetryMetricsruns again. If the publicMeterProvider.shutdown()or global metrics disable path is used without reinitialization, the channel retains this closure—and therefore the reader, exporter, and instrument state—for the rest of the process, and future refreshes still call the shut-down reader. Tie the unsubscribe to provider/reader shutdown instead of only replacing it on re-init.
// A clone resume shouldn't export measurements queued before the snapshot under its own identity.
unsubscribeMetricsPendingStateReset?.()
const onIdentityRefresh = () => reader.resetPendingState()
identityRefreshChannel.subscribe(onIdentityRefresh)
unsubscribeMetricsPendingStateReset = () => identityRefreshChannel.unsubscribe(onIdentityRefresh)
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js:247
- Clearing
#cumulativeStaterestarts the accumulated value, butMetricAggregator.#startTimeis initialized once and is not rebased here. With CUMULATIVE temporality (and always-cumulative UpDownCounters), the first post-refresh point therefore carries a startTimeUnixNano from before the snapshot while its value starts from the clone-local zero; reset the aggregator's cumulative start time together with this state.
for (const key of this.#cumulativeState.keys()) {
this.#lastExportedState.delete(key)
}
this.#cumulativeState.clear()
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js:247
- A queue overflow before the refresh leaves
#droppedCountintact. If the refresh occurs before the next collection, the next export logs that it dropped pre-refresh measurements, which is stale and misleading; clear this counter when discarding the pending state.
resetPendingState () {
this.#measurements = []
for (const key of this.#cumulativeState.keys()) {
this.#lastExportedState.delete(key)
}
this.#cumulativeState.clear()
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js:247
- The new test refreshes before the first collection, so
#cumulativeStateis empty and this loop never executes. It would still pass if#lastExportedStatewere not deleted; export a synchronous counter before the refresh, then record 7 afterward and assert the post-refresh delta is 7 (not 2) to cover the baseline-reset behavior introduced here.
for (const key of this.#cumulativeState.keys()) {
this.#lastExportedState.delete(key)
}
this.#cumulativeState.clear()
packages/dd-trace/src/runtime_metrics/runtime_metrics.js:196
- When the native-metrics branch is active,
eventLoopDelayObserveris never created (the branch above usesnativeMetrics.stats()instead), so this refresh callback does not clear any state accumulated by the native sampler. Samples from the pre-snapshot interval can therefore be returned by the first post-refreshstats()call and emitted with the clone's tags. Reset or consume the native sampler state at the refresh boundary as well.
if (eventLoopDelayObserver) {
eventLoopDelayObserver.disable()
eventLoopDelayObserver.reset()
eventLoopDelayObserver.enable()
}
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resetPendingState () { | ||
| this.#logRecords = [] | ||
| this.#clearTimer() |
| resetPendingState () { | ||
| this.#measurements = [] |
| const onIdentityRefresh = () => { | ||
| client.updateTags(buildClientConfig(config).tags) | ||
| onRefresh?.() |
![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: 741937c1cd
ℹ️ 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".
| function resetSamplerBaselines () { | ||
| lastTime = performance.now() |
There was a problem hiding this comment.
When runtime metrics use the default native path on Node versions without samplePerIteration, this callback resets only the JavaScript baselines. @datadog/native-metrics retains the interval consumed by nativeMetrics.stats(), so after cloning its CPU, event-loop, and GC samples still include pre-snapshot work; resetting lastTime alone can additionally divide that old CPU delta by only the short post-resume interval. The first flush from every clone can therefore duplicate or spike these metrics, so drain or restart the native sampler here and cover the currently skipped native test variant.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
| for (const key of this.#cumulativeState.keys()) { | ||
| this.#lastExportedState.delete(key) | ||
| } | ||
| this.#cumulativeState.clear() |
There was a problem hiding this comment.
When cumulative temporality is selected, or for an UpDownCounter that is always cumulative, clearing #cumulativeState restarts the exported value at zero but leaves MetricAggregator.#startTime at reader construction. Every subsequent point then advertises a pre-refresh start time while containing only post-refresh samples, producing a semantically inconsistent cumulative series and potentially undercounting backend rate conversions. Reset the aggregator start time with this state and add cumulative-temporality coverage rather than testing only the default delta counter path.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
BenchmarksBenchmark execution time: 2026-09-14 20:31:08 Comparing candidate commit 741937c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2304 metrics, 9 unstable metrics.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741937c1cd
ℹ️ 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".
| * since resume. | ||
| * @returns {void} | ||
| */ | ||
| function resetSamplerBaselines () { |
There was a problem hiding this comment.
When a GC entry is pending in the PerformanceObserver at snapshot time, this refresh callback resets the CPU and event-loop baselines but leaves that entry queued, so its callback runs after resume and reports the pre-snapshot pause through the clone's refreshed metrics client. Drain gcObserver.takeRecords() during refresh; the sibling resetSamplerBaselines() in otlp_runtime_metrics.js has the same omission and can likewise enqueue old GC durations after the metric reader has already been cleared.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
| if (lastElu !== null) { | ||
| lastElu = performance.eventLoopUtilization() |
There was a problem hiding this comment.
When a clone is restored before the first periodic metric export, the reader has no nodejs.eventloop.time entry in #lastExportedState; assigning lastElu here only rebases the utilization gauge, while the counter callback still observes the absolute idle and active totals from process startup. Its first delta export therefore includes the entire pre-snapshot initialization interval in every clone. Establish a refresh-time baseline for this observable counter, including the corresponding reader baseline, so only post-resume event-loop time is exported.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
| // A clone resume shouldn't export measurements queued before the snapshot under its own identity. | ||
| unsubscribeMetricsPendingStateReset?.() | ||
| const onIdentityRefresh = () => reader.resetPendingState() | ||
| identityRefreshChannel.subscribe(onIdentityRefresh) |
There was a problem hiding this comment.
When this initializer is called again without first disabling the OpenTelemetry global API—the re-init case the new comment claims to handle—setGlobalMeterProvider() does not replace the already registered provider, so applications continue recording through the first reader. These lines nevertheless unsubscribe that reader's reset callback and subscribe the newly constructed but unused reader; a later identity refresh therefore leaves the active reader's queued and cumulative state intact. Only swap the subscription when provider registration succeeds, or explicitly tear down the old provider first; the logs initializer has the same sibling failure.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
Stacked PRs
tianning.li/1-dogstatsd-exporter-refresh-resetstianning.li/2-otel-runtime-refresh-resets— current PRtianning.li/3-exporter-otel-identity-gapstianning.li/4-runtime-datastream-identity-gapstianning.li/5-exporter-buffer-statetianning.li/6-otel-datastream-counter-statetianning.li/7-spanstats-otel-retry-statetianning.li/8-agent-otel-counter-baselinestianning.li/9-datastream-dogstatsd-exporter-requestsOrigin
This change set was split from the monolithic PR #9814 to keep the identity-refresh fixes smaller and easier to review.
What does this PR do?
Reset OpenTelemetry log/metric queues and runtime metric baselines on identity refresh.
Motivation
OpenTelemetry and runtime metrics can retain measurements, observers, and baselines captured before a MicroVM snapshot. Resetting them ensures post-resume telemetry starts from the clone-local identity.
Logical change
This is the 2-otel-runtime-refresh-resets layer of the stacked MicroVM identity-refresh fix. It is based on
tianning.li/1-dogstatsd-exporter-refresh-resetsand keeps the subsystem-specific reset behavior isolated for review.Jira
SVLS-9768
Tests
Additional Notes
This PR is intentionally opened as a draft and is part of a linear stack.