Skip to content

fix(sdk/metric): keep distinct streams when overflow attribute exists - #8903

Open
lllakshit wants to merge 1 commit into
open-telemetry:mainfrom
lllakshit:fix/8776-expo-histogram-overflow-attr
Open

lllakshit wants to merge 1 commit into
open-telemetry:mainfrom
lllakshit:fix/8776-expo-histogram-overflow-attr

Conversation

@lllakshit

Copy link
Copy Markdown

Summary

Fixes #8776

Exponential histogram aggregation treated any existing otel.metric.overflow=true stream as proof that cardinality overflow had already occurred. After that stream was created, every later unseen attribute set was folded into it, even when AggregationLimit was 0 (unlimited).

This change only reuses the overflow stream when the aggregation limit is actually reached. With no limit, overflowSet remains a normal distinct stream and later attribute sets get their own data points.

Test plan

  • Added TestExpoHistogramOverflowAttributeBeforeLimit
  • Existing TestExpoHistogramOverflow still passes (limit enforcement unchanged)
  • go test ./sdk/metric/internal/aggregate -run 'TestExpoHistogramOverflow' -count=1

Exponential histogram aggregation reused the overflow attribute set for
every new measurement after that stream was created, even when the
cardinality limit had not been reached. Only route new attribute sets to
the overflow stream once the aggregation limit is actually exceeded.

Fixes open-telemetry#8776

Signed-off-by: lllakshit <llakshitmathur239@gmail.com>
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: lllakshit / name: lllakshit (85f8ecc)

@itssaharsh

Copy link
Copy Markdown
Member

Confirmed the fix on the expo histogram path — at 85f8ecc, recording overflowSet, {key="a"}, {key="b"} yields 3 distinct streams.

One scoping question: limitedSyncMap.LoadOrStoreAttr (atomic.go:243) does the same unconditional overflowSet lookup before the limit check, and sum.go / histogram.go / lastvalue.go all go through it. The same three measurements into newDeltaSum collapse to a single otel.metric.overflow=true data point — with limit = 0, and also with limit = 5 before the limit is reached same as #8776 describes for expo histogram.

Is fixing that in scope here, or better as a follow-up alongside #8077? Either way it'd be worth saying which in the PR description, since #8776 is written generally enough to read as covering all aggregations.

Small thing on the test: TestExpoHistogramOverflowAttributeBeforeLimit passes 0 as the limit, so it doesn't actually cover the before-limit case its name describes a positive limit would. And require.Len(t, e.values, 3) would match the file's prevailing testify style while keeping the fatal semantics of the current t.Fatalf.

@dashpole

Copy link
Copy Markdown
Contributor

@open-telemetry/go-approvers would folks prefer trying to get #8077 in (which fixes the issue as part of the refactor), or do we want to get this PR in to buy more time for review?

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.4%. Comparing base (1ad6638) to head (85f8ecc).
⚠️ Report is 71 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #8903     +/-   ##
=======================================
- Coverage   88.4%   88.4%   -0.1%     
=======================================
  Files        331     331             
  Lines      21001   21002      +1     
=======================================
  Hits       18572   18572             
- Misses      2429    2430      +1     
Files with missing lines Coverage Δ
...metric/internal/aggregate/exponential_histogram.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exponential histogram reuses overflow attribute before reaching cardinality limit

3 participants