Repository navigation
fix(id): reset OpenTelemetry and runtime metric state - #10343
Conversation
Overall package sizeSelf size: 8.97 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tianning.li/microvm-buffer-reset-04-otel-runtime-baseline #10343 +/- ##
============================================================================================
Coverage ? 87.50%
============================================================================================
Files ? 1015
Lines ? 156250
Branches ? 74
============================================================================================
Hits ? 136728
Misses ? 19522
Partials ? 0
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
Unflushed aggregation state can cross identity refresh, and shutdown may still invoke observable callbacks.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Completes MicroVM identity-refresh resets for OpenTelemetry and runtime metrics.
Changes:
- Resets metric reader state and observable baselines.
- Drains native and GC runtime state.
- Adds reset and cancellation coverage.
File summaries
| File | Summary |
|---|---|
packages/dd-trace/test/runtime_metrics.spec.js |
Tests runtime metric reset behavior. |
packages/dd-trace/test/opentelemetry/metrics.spec.js |
Tests OpenTelemetry metric reset behavior. |
packages/dd-trace/src/runtime_metrics/runtime_metrics.js |
Moderate (2 votes): aggregation client state is not reset, allowing unflushed metrics to cross identity refresh. |
packages/dd-trace/src/runtime_metrics/otlp_runtime_metrics.js |
Moderate (2 votes): OTLP aggregation client state is not reset, allowing unflushed metrics to cross identity refresh. |
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js |
Moderate (1 vote): shutdown retains observable instruments and the refresh subscription, potentially invoking callbacks after shutdown. |
packages/dd-trace/src/opentelemetry/metrics/instruments.js |
Bounds observable metric collection. |
Review details
Suppressed comments (1)
packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js:270
- After
shutdown(),#isShutdownremains true but the reader still retainsobservableInstruments, and the identity-refresh subscription inmetrics/index.jsis not removed. A refresh after shutdown will therefore execute every user observable callback through this new baseline collection even though the provider is shut down, potentially performing arbitrary user work; guardresetPendingState()for shutdown (or detach the subscription during shutdown).
this.#collectObservableMeasurements(),
this.#lastExportedState,
this.#pendingObservableCounterBaselines
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Discards any GC entries recorded but not yet delivered to the observer's callback, so a GC | ||
| // pause from just before the snapshot isn't attributed to the clone's first post-resume sample. | ||
| gcObserver?.takeRecords() |
| // Drains the addon's own CPU/event-loop/GC accumulators, which stats() otherwise reports as a | ||
| // delta since the last call - without this, the first post-resume capture would include | ||
| // pre-snapshot activity. | ||
| nativeMetrics?.stats() |
BenchmarksBenchmark execution time: 2026-09-18 19:06:21 Comparing candidate commit d185b6b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2306 metrics, 7 unstable metrics.
|
3331733 to
82d6409
Compare
![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: 82d6409ed0
ℹ️ 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".
| resetPendingState () { | ||
| this.#measurements = [] |
There was a problem hiding this comment.
When an identity refresh occurs while exporter.export() still has an HTTP request in flight, this method clears only reader-owned state, so every resumed clone can complete the pre-snapshot metrics request. OtlpHttpExporterBase.resetPendingState() was added to cancel exactly these requests, and the log processor invokes it, but the metric reader never does; call the exporter reset here as well to prevent duplicated pre-refresh metrics.
Useful? React with 👍 / 👎.
| for (const instrument of this.observableInstruments) { | ||
| const remainingCapacity = maxMeasurements - measurements.length | ||
| const observableMeasurements = instrument.collect(remainingCapacity, onDrop) |
There was a problem hiding this comment.
When gauges or observable up/down counters emit enough data points to fill the 65,536-measurement reset budget, this loop consumes all capacity on readings that resetObservableCounterBaselines() immediately ignores, so later ObservableCounters are never sampled at the refresh boundary. If those earlier instruments emit fewer points at the next periodic collection, a previously known counter loses that entire first post-resume interval via the pending-baseline path, while a newly appearing counter exports its process-lifetime absolute value under the clone identity; collect or prioritize only delta ObservableCounter readings here.
Useful? React with 👍 / 👎.
| if (type !== METRIC_TYPES.OBSERVABLECOUNTER || this.#getTemporality(type) !== TEMPORALITY.DELTA) { | ||
| continue |
There was a problem hiding this comment.
When OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE is CUMULATIVE or LOWMEMORY, this condition skips every ObservableCounter, so its next export still reports the callback's full pre-snapshot cumulative value under each clone's refreshed runtime ID. This directly affects the built-in nodejs.eventloop.time ObservableCounter as well as user counters; retain a refresh-time offset for cumulative counters and export values relative to it, analogous to the synchronous cumulative state reset.
Useful? React with 👍 / 👎.
8d3615b to
8118609
Compare
52bc0b1 to
f2fc0ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2fc0ab560
ℹ️ 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".
| const baseline = observableCounterBaselines.get(stateKey) | ||
| if (baseline !== undefined) dataPoint.value -= baseline |
There was a problem hiding this comment.
When temporality is CUMULATIVE or LOWMEMORY and an ObservableCounter attribute series was exported before the snapshot but does not emit during resetPendingState(), the current revision's new baseline map has no entry for it; this condition therefore leaves its first returned process-lifetime value unchanged and reports pre-snapshot counts under the clone identity. Unlike the DELTA branch, cumulative counters have no deferred-baseline path, so previously seen cumulative series need to be tracked and their first post-refresh observation discarded or used as the new offset when no refresh sample was available.
Useful? React with 👍 / 👎.
8118609 to
48c72fc
Compare
f2fc0ab to
60c0679
Compare
60c0679 to
d185b6b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d185b6bd65
ℹ️ 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 (baseline !== undefined) { | ||
| dataPoint.value -= baseline | ||
| // Keep the latest absolute reading so a later refresh can preserve absent-series state. | ||
| observableCounterBaselines.set(stateKey, currentValue) |
There was a problem hiding this comment.
When an ObservableCounter uses CUMULATIVE or LOWMEMORY temporality after an identity refresh, updating the stored baseline after every export converts the series into interval deltas while it is still marked cumulative. For example, a refresh baseline of 23 followed by callback values 25 and 27 exports 2 and 2, whereas cumulative clone-local values must be 2 and 4; retain the refresh-time baseline until the next refresh. The current tests stop after one post-refresh export and therefore miss this repeated-collection sibling case.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
| const observe = maxMeasurements === Infinity | ||
| ? (instrument, value, attributes) => { | ||
| if (instruments.has(instrument) && (!filter || filter(instrument))) { |
There was a problem hiding this comment.
During an identity refresh, the new filter suppresses observations from non-ObservableCounter instruments but still invokes every batch callback. A batch callback registered only for gauges or ObservableUpDownCounters can drain or reset its backing accumulator on each invocation; this hidden collection discards that result, so the next real export loses the accumulated measurement. Check whether the callback's instrument set contains a matching instrument before invoking it.
Useful? React with 👍 / 👎.
| if (metric.type === METRIC_TYPES.OBSERVABLECOUNTER && observableCounterStateKeys) { | ||
| scopeKey = this.#getScopeKey(metric.instrumentationScope) | ||
| for (const dataPoint of metric.dataPointMap.values()) { | ||
| const stateKey = this.#getStateKey(scopeKey, metric.name, metric.type, dataPoint.attrKey) | ||
| observableCounterStateKeys.add(stateKey) |
There was a problem hiding this comment.
In a MicroVM using CUMULATIVE or LOWMEMORY temporality, every periodic collection now permanently adds each ObservableCounter attribute series to #observableCounterStateKeys. The sole production identity-refresh publisher is the one-shot /run hook, so after that refresh this set is never consumed again; counters with rotating attributes therefore grow retained heap for the clone's entire lifetime. Disable or clear this tracking after the refresh state has been captured.
AGENTS.md reference: AGENTS.md:L107-L112
Useful? React with 👍 / 👎.
|
Going to move this PR to draft since there has been no activity for 3 weeks, please reopen once ready |
What does this PR do?
Complete the OpenTelemetry and runtime-metrics reset path for MicroVM identity refresh.
Motivation
This is a reviewable slice from the breakdown of monolithic PR #9814, tracked by SVLS-9768. A MicroVM identity refresh must not carry buffered, baseline, or pending exporter state into the next identity.
Changes
Dependency tree
origin/master01-common-exporter-agent-spanstats-reset-and-cancel02-agentless-exporter-identity06-datastream-reset-and-cancel03-dogstatsd-buffer-reset04-otel-runtime-baseline05-otel-runtime-metric-resets← current PRThis PR is intentionally draft and is part of the stacked review sequence above.