Skip to content

[METRICS SDK] Enforce a runtime minimum scale for Base2ExponentialHistogramAggregation - #4353

Open
ThomsonTan wants to merge 11 commits into
open-telemetry:mainfrom
ThomsonTan:runtime_scale
Open

[METRICS SDK] Enforce a runtime minimum scale for Base2ExponentialHistogramAggregation#4353
ThomsonTan wants to merge 11 commits into
open-telemetry:mainfrom
ThomsonTan:runtime_scale

Conversation

@ThomsonTan

Copy link
Copy Markdown
Contributor

Fixes #4325

Changes

This change enforces a runtime minimum scale of -10 for base2 exponential histograms, guarantees sufficient bucket capacity at that floor to preserve recorded counts, centralizes scale reduction across record, merge, and diff paths, guards the indexer against oversized shifts, and adds regression tests plus changelog documentation for the new behavior.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@ThomsonTan
ThomsonTan requested a review from a team as a code owner August 4, 2026 02:12
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.88764% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.48%. Comparing base (3893576) to head (a5af8bd).

Files with missing lines Patch % Lines
...egation/base2_exponential_histogram_aggregation.cc 89.29% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4353      +/-   ##
==========================================
- Coverage   82.49%   82.48%   -0.00%     
==========================================
  Files         502      502              
  Lines       19899    19934      +35     
==========================================
+ Hits        16414    16441      +27     
- Misses       3485     3493       +8     
Files with missing lines Coverage Δ
...metry/sdk/metrics/aggregation/aggregation_config.h 100.00% <ø> (ø)
...aggregation/base2_exponential_histogram_indexer.cc 100.00% <100.00%> (ø)
...egation/base2_exponential_histogram_aggregation.cc 94.78% <89.29%> (-1.84%) ⬇️

... 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.

// max_size_ == 2 configuration, and the extra slot is only reachable at the floor.
size_t BucketCapacity(size_t max_buckets) noexcept
{
return (std::max)(max_buckets, kMinBucketsAtFloor);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can create 3 buckets when the user configured max_size = 2. Since max_size is the maximum number of buckets, can we use -11 as the runtime floor instead? At -11, the full double range fits in 2 buckets, so we do not need this exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks. Changed the runtime floor to -11 and dropped kMinBucketsAtFloor, so max_size is now a hard cap. At -11 every finite double maps to an index in [-1, 0], and the max_size = 2 repro from #4325 now ends at scale -11 with a two-bucket span and capacity 2.

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.

Define and enforce a runtime minimum scale for Base2ExponentialHistogramAggregation

2 participants