[METRICS] Fix stale async attribute sets in cumulative exports (#4108)

[vahle-at-psu]

Summary

Fixes #4108. Asynchronous instruments using cumulative aggregation temporality no longer export attribute sets that were absent from the current reader callback. Their cumulative baselines are retained so values remain correct when observations reappear.

Root Cause

TemporalMetricStorage::buildMetrics() carries cumulative aggregations forward between collections. This state is needed to preserve cumulative values, but stale attribute sets must not be included in every export.

Cardinality overflow also needs special handling: a current observation can be merged into otel.metric.overflow, even though its original attribute set is not present under that key.

Changes

• Preserve cumulative state while filtering exported points by the current reader callback.
• Track observations routed to the overflow point so their contributions are still exported.
• Cover ObservableCounter, ObservableGauge, and ObservableUpDownCounter behavior.
• Add storage-level and SDK-level regression tests for stale attributes, gaps, multiple readers, and overflow.

Behavior

Scenario	Result
Async cumulative attribute is absent from the current callback	Point is omitted; cumulative baseline is retained
Async cumulative attribute reappears	Full cumulative value is exported
Async delta attribute reappears after a gap	Existing delta behavior is unchanged
Current observation is routed to overflow	Overflow contribution is exported
Sync cumulative attribute is absent this cycle	Existing carry-forward behavior is unchanged

With multiple readers, cumulative async points are exported only when observed in that reader's current callback round. A lagging reader may omit a point at an intermediate collection; the cumulative baseline is retained and exported after the attribute is observed again.

Tests

The following tests run with both cumulative and delta temporality:

• StaleAttributeSetDropped
• AttributeReappearanceAfterGap
• NonMonotonicInstrumentsDropStaleObservations
• StaleAttributeSetDroppedMultiCollector
• MultiCollectorLaggingCollector
• MultiCollector

Additional tests:

• WritableMetricStorageCardinalityLimit.AsyncCumulativeOverflowIsExportedForLaggingReader
• MeterTest.AsyncCumulativeObservationsAreReaderScoped

Validated with cmake --build build --target async_metric_storage_test -j2 and build/sdk/test/metrics/async_metric_storage_test --gtest_brief=1 (26 tests passed).

---

Co-authored-by: pranitaurlam 227409059+pranitaurlam@users.noreply.github.com

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: pranitaurlam / name: pranitaurlam (e9cc6f3)
• ✅ login: vahle-at-psu / name: Patrick Summerer (e9cc6f3)

[codecov]

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.82%. Comparing base (adebdf0) to head (5781472).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...dk/include/opentelemetry/sdk/metrics/instruments.h	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4484      +/-   ##
==========================================
+ Coverage   86.79%   86.82%   +0.03%     
==========================================
  Files         525      525              
  Lines       20729    20753      +24     
==========================================
+ Hits        17989    18016      +27     
+ Misses       2740     2737       -3     

Files with missing lines	Coverage Δ
sdk/src/metrics/state/temporal_metric_storage.cc	100.00% <100.00%> (ø)
...dk/include/opentelemetry/sdk/metrics/instruments.h	75.56% <75.00%> (-0.05%)	⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mateenali66]

Built this on 731099b, the reported case is fixed. One problem with suppressing at export time.

merged_metrics is both the running cumulative state and the exported set (temporal_metric_storage.cc:179-190), so the new else if (!is_async_) at :163 does not just omit the point, it drops the baseline. On reappearance Record supplies delta = new - last_seen with nothing left to merge onto, so the cumulative point is the increment. A=10, absent, A=30, one collector:

cycle	main	this PR	expected
A=10	10	10	10
absent	10 (stale, #4108)	nothing	nothing
A=30	30	20	30

AttributeReappearanceAfterGapDeltaTemporality misses it because one collector under delta returns on the fast path at :67, before the changed branch. Happy to post the cumulative version of that test.

Would you rather keep the carry-forward in last_reported_metrics_ and filter when building the MetricData, so the baseline survives the gap?

[vahle-at-psu]

Built this on 731099b, the reported case is fixed. One problem with suppressing at export time.

merged_metrics is both the running cumulative state and the exported set (temporal_metric_storage.cc:179-190), so the new else if (!is_async_) at :163 does not just omit the point, it drops the baseline. On reappearance Record supplies delta = new - last_seen with nothing left to merge onto, so the cumulative point is the increment. A=10, absent, A=30, one collector:
cycle main this PR expected
A=10 10 10 10
absent 10 (stale, #4108) nothing nothing
A=30 30 20 30

AttributeReappearanceAfterGapDeltaTemporality misses it because one collector under delta returns on the fast path at :67, before the changed branch. Happy to post the cumulative version of that test.

Would you rather keep the carry-forward in last_reported_metrics_ and filter when building the MetricData, so the baseline survives the gap?

good catch!

I moved the is_async guard logic to the export callback, so the baseline state gets kept.
2 tests have been added:

• AttributeReappearanceAfterGapCumulativeTemporality
• MultiCollectorDeltaTemporality to test the slow path

Hope everything is addressed now :-)

[mateenali66]

rebuilt on 70a6832e. the baseline survives now. A=10, absent, A=30 on one collector gives 10, nothing, 30, and AttributeReappearanceAfterGapCumulativeTemporality pins exactly that. 226 metric tests pass.

also ran the two-collector cumulative case, since the new multi-collector test is delta and the is_async_ filter only fires on cumulative. second collector's interval spanning a cycle it did not itself collect in: c2 reports 10, exports zero points at the gap, then reports 30 on reappearance rather than 20 or 50. the carry-forward holds on the slow path too.

one note, not a blocker. the filter tests delta_metrics, which is the current call's map rather than the union of that collector's unreported deltas. with two collectors, c2 can skip an export for an attribute that another collector's cycle observed inside c2's own interval. the cumulative value is still right when it reappears, so this is a cadence difference rather than lost data, but worth confirming it is deliberate.

looks right to me otherwise.

[vahle-at-psu]

rebuilt on 70a6832e. the baseline survives now. A=10, absent, A=30 on one collector gives 10, nothing, 30, and AttributeReappearanceAfterGapCumulativeTemporality pins exactly that. 226 metric tests pass.

also ran the two-collector cumulative case, since the new multi-collector test is delta and the is_async_ filter only fires on cumulative. second collector's interval spanning a cycle it did not itself collect in: c2 reports 10, exports zero points at the gap, then reports 30 on reappearance rather than 20 or 50. the carry-forward holds on the slow path too.

one note, not a blocker. the filter tests delta_metrics, which is the current call's map rather than the union of that collector's unreported deltas. with two collectors, c2 can skip an export for an attribute that another collector's cycle observed inside c2's own interval. the cumulative value is still right when it reappears, so this is a cadence difference rather than lost data, but worth confirming it is deliberate.

looks right to me otherwise.

Again well analyzed - thank you!
I thought it would be better if I again change the introduced tests to run as fixture with both Delta and Cumulative temporality like the existing tests. Now they cover more cases inherently.
The cadence in the multi-collector case could be mitigated by remembering the individual collections and then filter on this set.

[lalitb]

In PR desc, Behavior section:

Sync cumulative, attribute not measured this cycle	Carried forward (correct)	Unchanged

Please update the description's delta-reappearance row to say "unchanged." Main already preserves the baseline across gaps, and this PR does not change that delta behavior.

[vahle-at-psu]

641ce25 fixed format and clang-tidy issues

[mateenali66]

re-checked at 641ce251. the cumulative baseline point is fixed, and lalitb's two-reader case now exports {GET=12} for counter, gauge and up-down.

one regression at the cardinality limit. two readers, default limit, no view. reader 1's round observes 2000 sets, then reader 2 (cumulative) observes only x=7. reader 2 exports 0 points at head, main exports OVERFLOW=7. x goes to the overflow key when reader 2's queue is merged, and the filter at temporal_metric_storage.cc:206 looks the overflow key up in delta_metrics, where it never is. the spec says measurements MUST NOT be dropped during an overflow.

smaller thing, every new test uses ObservableCounter, but #4108 reports gauge and up-down, which go through BuildDeltaFromRound since #4635.

[vahle-at-psu]

re-checked at 641ce251. the cumulative baseline point is fixed, and lalitb's two-reader case now exports {GET=12} for counter, gauge and up-down.

one regression at the cardinality limit. two readers, default limit, no view. reader 1's round observes 2000 sets, then reader 2 (cumulative) observes only x=7. reader 2 exports 0 points at head, main exports OVERFLOW=7. x goes to the overflow key when reader 2's queue is merged, and the filter at temporal_metric_storage.cc:206 looks the overflow key up in delta_metrics, where it never is. the spec says measurements MUST NOT be dropped during an overflow.

smaller thing, every new test uses ObservableCounter, but #4108 reports gauge and up-down, which go through BuildDeltaFromRound since #4635.

Thx! 5781472 incorporates your feedback
It further adds a test for non-monotonic instruments ObservableGauge and ObservableUpDownCounter

[mateenali66]

re-checked at d6f95afc. the overflow drop is fixed at both limits, and the new test covers gauge and up-down.

one regression from the new merge order. temporal_metric_storage.cc:144-157 admits the previous cumulative state before the current round for async instruments too, and stale async sets stay there, so once a reader has held limit sets every later set exports only as overflow. one cumulative reader, gauge, 2000 sets once, then only x=7. head exports {OVERFLOW=7}, main and 641ce251 export x=7. the next round, {x=8, y=3}, gives {OVERFLOW=8}. the spec has async instruments prefer the callback's first-observed attributes.

keeping that order for sync and merging the reader's own round first for async cumulative fixed both locally. reader 2 then exports x=7, and only AsyncCumulativeOverflowIsExportedForLaggingReader changes.

the sync side changed too, limit 2 now keeps a and b plus OVERFLOW=12, but the description says sync is unchanged and no test pins it.

[vahle-at-psu]

re-checked at d6f95afc. the overflow drop is fixed at both limits, and the new test covers gauge and up-down.

one regression from the new merge order. temporal_metric_storage.cc:144-157 admits the previous cumulative state before the current round for async instruments too, and stale async sets stay there, so once a reader has held limit sets every later set exports only as overflow. one cumulative reader, gauge, 2000 sets once, then only x=7. head exports {OVERFLOW=7}, main and 641ce251 export x=7. the next round, {x=8, y=3}, gives {OVERFLOW=8}. the spec has async instruments prefer the callback's first-observed attributes.

keeping that order for sync and merging the reader's own round first for async cumulative fixed both locally. reader 2 then exports x=7, and only AsyncCumulativeOverflowIsExportedForLaggingReader changes.

the sync side changed too, limit 2 now keeps a and b plus OVERFLOW=12, but the description says sync is unchanged and no test pins it.

sorry for always reintroducing regressions - I am missing the big picture and dependencies within the code base.
The topic is getting quite complex and the changes are growing...
Hopefully continuously adding tests improves the situation and show up regressions faster in future.

With cdf2aff I instructed Copilot to fulfill all relevant metric specs, address feedback of all reviewers in this PR while keeping the changes minimal (except in tests)

Do you agree with continuing this change/review iteration in this way or does it make sense to revisit the whole topic again from beginning?