[METRICS] Fix stale async attribute sets in cumulative exports (#4108) - #4484
vahle-at-psu wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4484 +/- ##
==========================================
+ Coverage 83.53% 83.53% +0.01%
==========================================
Files 522 522
Lines 20447 20452 +5
==========================================
+ Hits 17078 17083 +5
Misses 3369 3369
🚀 New features to boost your workflow:
|
…telemetry#4108) Async instruments (ObservableCounter, ObservableGauge, ObservableUpDownCounter) under cumulative temporality were emitting attribute sets indefinitely after the callback stopped reporting them, violating the OTel spec requirement: "The implementation SHOULD NOT produce aggregated metric data for a previously-observed attribute set which is not observed during a successful callback." Root cause: `TemporalMetricStorage::buildMetrics()` unconditionally carried every entry from `last_reported_metrics_` into the output even when it was absent from the current delta. Fix: - Add `is_async_` flag (default false) to `TemporalMetricStorage`. The cumulative merge now skips entries not present in the current delta for async instruments, while sync instruments retain the existing carry-forward behaviour. - Pass `is_async = true` when constructing `TemporalMetricStorage` from `AsyncMetricStorage`. - Do NOT prune `cumulative_hash_map_` in `AsyncMetricStorage::Collect()` so that the absolute-value baseline is preserved across absent cycles. This ensures correct delta computation (new - last_seen, not the full new value) when an attribute set reappears after a gap — consistent with the approach taken by opentelemetry-dotnet#6883. Tests added in async_metric_storage_test.cc: - StaleAttributeSetDroppedInCumulativeExport: verifies that an attribute set absent from the callback is not emitted in subsequent cumulative exports. - AttributeReappearanceAfterGapDeltaTemporality: verifies that an attribute set reappearing after an absent cycle emits only the increment since last observed (delta = 1, not 11), confirming the baseline is correctly preserved. Fixes open-telemetry#4108 Co-authored-by: pranitaurlam <227409059+pranitaurlam@users.noreply.github.com>
|
Built this on
Would you rather keep the carry-forward in |
good catch! I moved the is_async guard logic to the export callback, so the baseline state gets kept.
Hope everything is addressed now :-) |
|
rebuilt on also ran the two-collector cumulative case, since the new multi-collector test is delta and the one note, not a blocker. the filter tests looks right to me otherwise. |
…ttribute suppression The is_async_ cumulative export guard tested the shared delta_metrics map, which is only populated for whichever collector drains it first in a given cycle. Every other collector saw an empty map and had its already-observed attribute sets incorrectly suppressed for that cycle. Replace the check with a per-collector "observed this cycle" set, captured from each collector's own merged unreported deltas before the cumulative baseline is merged in. This preserves the open-telemetry#4108 stale-drop behavior while making it independent of collection order across collectors. Adds regression coverage for: - stale attribute suppression with two collectors - a collector lagging behind another by multiple cycles - start_ts/end_ts correctness for the multi-collector cumulative and delta paths
Again well analyzed - thank you! |
…ttribute suppression The is_async_ cumulative export guard tested the shared delta_metrics map, which is only populated for whichever collector drains it first in a given cycle. Every other collector saw an empty map and had its already-observed attribute sets incorrectly suppressed for that cycle. Replace the check with a per-collector "observed this cycle" set, captured from each collector's own merged unreported deltas before the cumulative baseline is merged in. This preserves the open-telemetry#4108 stale-drop behavior while making it independent of collection order across collectors. Adds regression coverage for: - stale attribute suppression with two collectors - a collector lagging behind another by multiple cycles - start_ts/end_ts correctness for the multi-collector cumulative and delta paths
Summary
Fixes #4108.
This PR continues the work from #4140 by @pranitaurlam. Since the original author has been unresponsive for several weeks and the fix is needed in production, I am taking over the PR, giving full credit to @pranitaurlam for the original analysis and fix skeleton.
Async instruments (
ObservableCounter,ObservableGauge,ObservableUpDownCounter) under cumulative temporality emitted attribute sets indefinitely after the callback stopped reporting them, violating the OTel spec:Root cause
TemporalMetricStorage::buildMetrics()unconditionally carried every entry fromlast_reported_metrics_into the cumulative output, even when the attribute set was absent from the current delta.Changes
temporal_metric_storage.h/.ccis_async_boolean (defaultfalse) toTemporalMetricStorage.else if (!is_async_)guard: sync instruments still carry forward all attribute sets (existing behaviour, spec-correct); async instruments skip entries not present in the current delta.async_metric_storage.his_async = truewhen constructingTemporalMetricStorage.cumulative_hash_map_inCollect()(deviation from [METRICS] Drop stale async attribute sets from cumulative exports #4140): the absolute-value baseline is preserved across absent cycles. This ensures correct incremental delta computation when an attribute set reappears after a gap (delta = new − last_seen, notnew), consistent with opentelemetry-dotnet#6883. Stale entries are suppressed at export time by theis_async_guard instead.Behaviour
Sister SDKs with equivalent fixes:
Tests
Two regression tests added in
async_metric_storage_test.cc, addressing lalitb's review request on #4140:StaleAttributeSetDroppedInCumulativeExport: attribute present incollection 1, absent in collection 2 → must not appear in collection
2's cumulative export.
AttributeReappearanceAfterGapDeltaTemporality:A=10 → missing → A=11under delta temporality → emitted delta must be1, not11,confirming the baseline is preserved across the absent cycle.
Co-authored-by: pranitaurlam 227409059+pranitaurlam@users.noreply.github.com