[Metrics] Implement cumulative aggregation for async Long and Double counters in MetricPoint

[nabutabu]

Fixes #7215
Design discussion issue NA

Changes

Use InterlockedHelper.Add in Update calls and reset aggregation values in TakeSnapshot using Interlocked.Exchange(ref this.runningValue.AsLong, 0);

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (8b03fd9) to head (371171b).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/OpenTelemetry/Metrics/AggregatorStore.cs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7227      +/-   ##
==========================================
- Coverage   90.19%   90.17%   -0.03%     
==========================================
  Files         285      279       -6     
  Lines       15376    14864     -512     
==========================================
- Hits        13869    13404     -465     
+ Misses       1507     1460      -47     

Flag	Coverage Δ
unittests-PowerShellScripts	?
unittests-Project-Experimental	90.06% <95.45%> (-0.04%)	⬇️
unittests-Project-Stable	90.14% <95.45%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...c/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs	94.94% <100.00%> (+0.02%)	⬆️
src/OpenTelemetry/Metrics/AggregatorStore.cs	89.51% <94.44%> (-0.15%)	⬇️

... and 16 files with indirect coverage changes

[cijothomas]

This is not correct. If incoming is cumulative, then it is already the sum. We (i.e SDK) shouldn't add it.

[cijothomas]

See
https://github.com/open-telemetry/opentelemetry-dotnet/pull/7227/changes#r3168921622

Let me know if I misunderstood the fix.

[nabutabu]

Fix was incorrect and the issue was that I did not take into account a couple of things which the failing tests also pointed out. Changing Update/UpdateWithExemplar was incorrect because each reader has its own AggregatorStore and so the same measurement stream feeds both views. Two measurements with different raw tags but the same filtered tags return the same index from FindMetricAggregatorsCustomTag. Then UpdateLongMetricPoint is called twice on that same index which then means that the second Exchange call overwrites the first.

Basically if a MetricPoint is in state CollectPending and is IsAsynchronous then the value sent by UpdateLongCustomTags is the existingRunningValue + newValue instead of just newValue.

[cijothomas]

The idea of modifying the incoming value to achieve "spatial aggregation" is good - but it does create incorrect Exemplar values.

It is not that bad - as Exemplars have limited utility in Observable instruments anyway and Exemplars are disabled also by-default for Observable. We should see if the fix can keep Exemplars correct as well. (Exemplar should get the original unmodified value, but Metricpoint should get the updated one..)

[nabutabu]

Read more about Exemplars and this does complicate this issue a bit. Blurting out my understanding of what's happening for future me:

Exemplars are defined as: "access to the raw measurement value, time stamp when measurement was made, and trace context", which means each exemplar recording should be the raw reading instead of the cumulative value that the (non-exemplar) async cumulative instrument uses.

This is important because each exemplar metric recording is also associated with a trace. If exemplars stored aggregated values, we'd lose the connection to individual traces for each recording. The exemplar's purpose is to preserve the context of specific measurements that contribute to the aggregate, which this current change loses.

I'll come back when I have more on how we could fix this.

• First thought is to add a parameter to UpdateWithExemplar in MetricPoint -> "number" would be the cumulative value and the new parameter is the measured value.
• MetricPoint does already have access to its own running value, maybe we could just subtract that when calling UpdateExemplar, this would need to happen before Interlocked.Add is called on the running value. Also would this work for every kind of instrument?

[cijothomas]

I added just one test to prove that we don't do spatial-aggregation in this SDK. We need much more coverage when fixing the issue. Covering delta/cumulative, multiple cycles, various view combinations.

[cijothomas]

The current iteration looks very promising (thanks! This was indeed a hard problem!).
Requesting changes to address below comment
https://github.com/open-telemetry/opentelemetry-dotnet/pull/7227/changes#r3183554865

Also, we need to vastly improve test coverage when doing this fix- lot of potential edge cases. (I spotted only one about Exemplar value, but could be more)

[nabutabu]

@cijothomas I added as many tests as I could think of, but I am not sure if this is all that needs coverage. Would appreciate any guidance on any other nuances of the measurements that could potentially be affected!

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[Copilot]

Pull request overview

Implements spatial (cumulative-within-collection) aggregation for asynchronous ObservableCounter<T> streams when a view filters attributes (TagKeys) such that multiple measurements collapse into the same resulting attribute set, while ensuring exemplars continue to record the raw measurement value (not the spatially-aggregated sum). This aligns the SDK’s observable counter behavior with expected semantics and re-enables previously skipped coverage.

Changes:

• Add spatial-aggregation handling for async cumulative long/double sums under tag filtering, and plumb raw measurement values separately into exemplar updates.
• Unskip and expand regression tests for observable counter spatial aggregation (including multi-cycle + double).
• Add exemplar-focused tests validating exemplar collection under spatial aggregation and across cycles.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/OpenTelemetry/Metrics/AggregatorStore.cs	Adjusts async cumulative update behavior under tag filtering to spatially aggregate collapsed series and preserve raw exemplar values.
src/OpenTelemetry/Metrics/MetricPoint/MetricPoint.cs	Extends UpdateWithExemplar to accept a separate exemplar value and exposes running-value reads needed for spatial aggregation.
test/OpenTelemetry.Tests/Metrics/MetricApiTests.cs	Unskips and adds/extends spatial aggregation regression tests for observable counters (long/double, multi-cycle, partial filtering).
test/OpenTelemetry.Tests/Metrics/MetricExemplarTests.cs	Adds exemplar regression coverage under spatial aggregation (including multi-cycle behavior).
test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs	Adds a regression test guarding synchronous counter behavior under tag filtering.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Kielek]

Asked Codex/GPT5.5 to check this PR.
Both of them marked as P1. Could you please double check this feedback?

Review Notes and Unit Test Proposals for PR #7227

I think there are two correctness risks worth covering with targeted unit tests.

Issue 1: Delta temporality is not correct when filtered streams change

The current fix spatially aggregates async cumulative counter values by updating the already-filtered MetricPoint with:

existing running value + incoming measurement value

That works for simple cumulative export cases where the same underlying streams are reported every cycle. The problem is delta export. MetricPoint.TakeSnapshot(outputDelta: true) computes the delta from the collapsed aggregate:

current collapsed aggregate - previous collapsed aggregate

That is not equivalent to summing per-stream deltas if one original measurement stream disappears or reappears between callbacks.

Example:

Collection 1:
  A = 10
  B = 10
  Collapsed value = 20

Collection 2:
  A = 15
  B is absent
  Collapsed value = 15

If the SDK diffs collapsed values, delta export becomes:

15 - 20 = -5

The expected delta is:

(15 - 10) = 5

The absent stream should not subtract from the collapsed aggregate. Delta needs to be calculated per original measurement stream before spatial aggregation, or the implementation needs equivalent per-stream state.

Suggested test: long observable counter, one stream disappears

Add a regression test for ObservableCounter<long> with delta temporality and a view that drops all tags.

Scenario:

Collection 1:
  A = 10
  B = 10
  Expected exported delta = 20

Collection 2:
  A = 15
  B is absent
  Expected exported delta = 5

This should fail if the SDK diffs only the collapsed aggregate, because it would export -5.

Suggested test: long observable counter, unchanged stream and missing stream

This is the smallest version of the same bug.

Scenario:

Collection 1:
  A = 10
  B = 10
  Expected exported delta = 20

Collection 2:
  A = 10
  B is absent
  Expected exported delta = 0

An aggregate-diff implementation would export:

10 - 20 = -10

The expected result is zero because the only stream still reported did not change.

Suggested test: double observable counter, one stream disappears

Add the same coverage for ObservableCounter<double>.

Scenario:

Collection 1:
  A = 10.5
  B = 10.5
  Expected exported delta = 21.0

Collection 2:
  A = 15.5
  B is absent
  Expected exported delta = 5.0

This covers the UpdateDoubleCustomTags path, which has the same aggregation risk as the long path.

Suggested test: stream reappears after being absent

This verifies that per-stream state is preserved across multiple collection cycles.

Scenario:

Collection 1:
  A = 10
  B = 10
  Expected exported delta = 20

Collection 2:
  A = 15
  B is absent
  Expected exported delta = 5

Collection 3:
  A = 20
  B = 25
  Expected exported delta = 20

Collection 3 should be:

(20 - 15) + (25 - 10) = 20

This catches both disappearing and reappearing stream behavior under spatial aggregation.

Issue 2: Custom-tag overflow can throw before the overflow metric point is used

FindMetricAggregatorsCustomTag can return -1 when the metric point cardinality limit is exhausted. Before this PR, that -1 was handled by UpdateLongMetricPoint or UpdateDoubleMetricPoint, which routes the measurement to the overflow metric point.

The new custom-tag async branches read the metric point before that existing negative-index handling runs:

ref var metricPoint = ref this.metricPoints[index];

If index is -1, this throws before the overflow path can run. The outer AggregatorStore.Update catches the exception and records an SDK internal error/drop, so the existing overflow behavior is bypassed.

This applies to both:

UpdateLongCustomTags
UpdateDoubleCustomTags

Suggested test: long observable counter custom-tag overflow

Add a test that forces the custom-tag path and exhausts the cardinality limit.

Shape:

Instrument:
  ObservableCounter<long>

View:
  TagKeys = ["keep"]

Measurements:
  Use enough distinct "keep" values to exhaust the configured metric point cardinality limit.

Assertions:
  ForceFlush does not throw.
  The existing overflow metric point behavior is used.
  The measurement is not dropped as an SDK internal error.

This guards against indexing metricPoints[-1] before UpdateLongMetricPoint can route the measurement to overflow.

[nabutabu]

The delta temporality behavior under spatial aggregation is a known open question for the spec and @cijothomas has already filed open-telemetry/opentelemetry-specification#4861 to track it.

The core issue is what delta to emit when a stream disappears or reappears:

• Option 1 (delta from zero):
  Bounded memory, but incorrect if the underlying counter continued incrementing during the gap.
• Option 2 (delta from last known value):
  Correct, but requires tracking all ever-seen attribute sets, which conflicts with cardinality limits and the SDK's existing reclaim machinery.

I think we should skip the four failing delta tests I added and reference the issue mentioned above:

ObservableCounterSpatialAggregationDelta_UnchangedStreamAndMissingStream_DeltaIsZero
ObservableCounterSpatialAggregationDelta_StreamDisappears_DeltaIsCorrect
ObservableCounterSpatialAggregationDelta_StreamReappearsAfterGap_DeltaIsCorrect
ObservableCounterSpatialAggregationDelta_Double_StreamDisappears_DeltaIsCorrect

Seems like we added the fix to remove Skip for one test and ended up adding 4 more tests that we need to skip haha

[nabutabu]

@cijothomas Could I get another review here whenever possible? Thank you :)