Skip to content

Comments

feat: Allow configuring of min/max in histograms#8095

Open
MikeGoldsmith wants to merge 10 commits intoopen-telemetry:mainfrom
honeycombio:mike/disable-histogram-minmax
Open

feat: Allow configuring of min/max in histograms#8095
MikeGoldsmith wants to merge 10 commits intoopen-telemetry:mainfrom
honeycombio:mike/disable-histogram-minmax

Conversation

@MikeGoldsmith
Copy link
Member

@MikeGoldsmith MikeGoldsmith commented Feb 16, 2026 •
edited
Loading

Summary

Adds support for disabling min/max recording in histogram aggregations per the OpenTelemetry spec.

Fixes #7859

Motivation

Some users need the ability to disable min/max recording in histograms. The OpenTelemetry spec requires this parameter for both explicit and exponential histogram aggregations.

Changes

  • Added HistogramOptions builder to encapsulate histogram behavioral options (currently recordMinMax), providing an extensible place for future options without method overload explosion
  • Added Aggregation.explicitBucketHistogram(List<Double>, HistogramOptions) and Aggregation.base2ExponentialBucketHistogram(int, int, HistogramOptions)
  • Updated aggregator implementations to conditionally record min/max
  • Integrated with declarative config — passes record_min_max from YAML to SDK
  • Backward compatible — existing methods without HistogramOptions default to recordMinMax=true
  • Updated public API diff doc with new methods and types

Testing

  • Added DoubleExplicitBucketHistogramAggregatorTest.testRecordMinMaxDisabled — verifies min/max not tracked when disabled for explicit histograms
  • Added DoubleBase2ExponentialHistogramAggregatorTest.testRecordMinMaxDisabled — verifies min/max not tracked when disabled for exponential histograms
  • Updated existing tests to use new constructor signatures

@MikeGoldsmith MikeGoldsmith requested a review from a team as a code owner February 16, 2026 14:55
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 16, 2026 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@MikeGoldsmith
Add recordMinMax param to histogram aggregations per spec
c01ceab 
Supports disabling min/max recording to work around GCP min/max handling issues. Backward compatible - defaults to true.
@MikeGoldsmith
Add tests for recordMinMax=false behavior
8f364a5 
Verifies min/max not tracked when disabled.
@MikeGoldsmith MikeGoldsmith force-pushed the mike/disable-histogram-minmax branch from 9caaa69 to 8f364a5 Compare February 16, 2026 14:56
@MikeGoldsmith MikeGoldsmith mentioned this pull request Feb 16, 2026
Open
@MikeGoldsmith MikeGoldsmith changed the title feat: Add recordMinMax parameter to histogram aggregations feat: Allow configuring of min/max in histograms Feb 16, 2026
@codecov
Copy link

codecov bot commented Feb 16, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (284a64f) to head (ec038ac).

Files with missing lines Patch % Lines
...gator/DoubleExplicitBucketHistogramAggregator.java 81.81% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #8095      +/-   ##
============================================
- Coverage     90.32%   90.30%   -0.02%     
- Complexity     7607     7610       +3     
============================================
  Files           839      839              
  Lines         22888    22910      +22     
  Branches       2283     2295      +12     
============================================
+ Hits          20673    20690      +17     
- Misses         1506     1507       +1     
- Partials        709      713       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

jack-berg
jack-berg reviewed Feb 19, 2026
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple small comments, but looks good overall. Thanks!

@MikeGoldsmith
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…
3d34d6c 
…nto mike/disable-histogram-minmax
@MikeGoldsmith
Address review feedback
849cc50 
- Remove @SInCE annotations (added during release flow)
- Remove old create() overloads without recordMinMax, update callers
- Remove redundant // read-only comment on final primitive field

Claude claude-sonnet-4-6 assisted with this change.
@MikeGoldsmith MikeGoldsmith force-pushed the mike/disable-histogram-minmax branch from a681c40 to 849cc50 Compare February 20, 2026 15:15
@MikeGoldsmith
Include recordMinMax in Aggregation toString()
7aaab68 
Declarative config tests compare aggregation equality via toString(), so
recordMinMax must be included for the comparisons to be valid.

Claude claude-sonnet-4-6 assisted with this change.
@MikeGoldsmith
Apply spotless formatting
ec038ac
jack-berg
jack-berg approved these changes Feb 20, 2026
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Will leave open for a few days to see if any other @open-telemetry/java-approvers have feedback about the new public API surface area

@jkwatson
Copy link
Contributor

jkwatson commented Feb 24, 2026

Will leave open for a few days to see if any other @open-telemetry/java-approvers have feedback about the new public API surface area

I finally got a chance to look at this. I'm slightly concerned about adding a simple boolean option to this. If we end up with more of them in the future, the API surface could end up exploding in non-optimal ways. What would y'all think about creating an AggregationOptions type that would encapsulate this new option, and also provide a place to put future options, when/if they crop up?

@MikeGoldsmith
Copy link
Member Author

MikeGoldsmith commented Feb 24, 2026 •
edited
Loading

Will leave open for a few days to see if any other @open-telemetry/java-approvers have feedback about the new public API surface area

I finally got a chance to look at this. I'm slightly concerned about adding a simple boolean option to this. If we end up with more of them in the future, the API surface could end up exploding in non-optimal ways. What would y'all think about creating an AggregationOptions type that would encapsulate this new option, and also provide a place to put future options, when/if they crop up?

Yeah, a wrapper type so we don't keep adding more and more options is a good idea 👍🏻

Thought more on this, and I'm not sure. Other existing Histogram options (maxBuckets, maxScale, and bucketBoundaries) all use the same in-line design. Adding an Options class just for recordMinMax feels excessive, especially if we don't know of any other options that might come along?

@jkwatson is this something you feel strongly about adding?

@jkwatson
Copy link
Contributor

jkwatson commented Feb 24, 2026

Will leave open for a few days to see if any other @open-telemetry/java-approvers have feedback about the new public API surface area

I finally got a chance to look at this. I'm slightly concerned about adding a simple boolean option to this. If we end up with more of them in the future, the API surface could end up exploding in non-optimal ways. What would y'all think about creating an AggregationOptions type that would encapsulate this new option, and also provide a place to put future options, when/if they crop up?

Yeah, a wrapper type so we don't keep adding more and more options is a good idea 👍🏻

Thought more on this, and I'm not sure. Other existing Histogram options (maxBuckets, maxScale, and bucketBoundaries) all use the same in-line design. Adding an Options class just for recordMinMax feels excessive, especially if we don't know of any other options that might come along?

@jkwatson is this something you feel strongly about adding?

Maybe we should put all the current options into an HistogramAggregationOptions as well, rather than continue the sub-optimal combinatoric parameter approach?

@MikeGoldsmith
Copy link
Member Author

MikeGoldsmith commented Feb 24, 2026 •
edited
Loading

Will leave open for a few days to see if any other @open-telemetry/java-approvers have feedback about the new public API surface area

I finally got a chance to look at this. I'm slightly concerned about adding a simple boolean option to this. If we end up with more of them in the future, the API surface could end up exploding in non-optimal ways. What would y'all think about creating an AggregationOptions type that would encapsulate this new option, and also provide a place to put future options, when/if they crop up?

Yeah, a wrapper type so we don't keep adding more and more options is a good idea 👍🏻
Thought more on this, and I'm not sure. Other existing Histogram options (maxBuckets, maxScale, and bucketBoundaries) all use the same in-line design. Adding an Options class just for recordMinMax feels excessive, especially if we don't know of any other options that might come along?
@jkwatson is this something you feel strongly about adding?

Maybe we should put all the current options into an HistogramAggregationOptions as well, rather than continue the sub-optimal combinatoric parameter approach?

Sure, I could look at doing that.

@MikeGoldsmith
Add HistogramOptions builder for histogram behavioral options
94da36b 
Replace boolean recordMinMax params on Aggregation factory methods with a
HistogramOptions builder, providing a single extensible place for future
histogram options without requiring new method overloads.

Claude claude-sonnet-4-6 assisted with this change.
@MikeGoldsmith
Copy link
Member Author

MikeGoldsmith commented Feb 24, 2026 •
edited
Loading

@jkwatson I've updated to use an options builder for the recordMinMax optional parameter. The other params are required so don't really fit into the options builder. Let me know what you think 😄

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

Reviewers

@jack-berg jack-berg jack-berg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support RecordMinMax for histogram and exponential histogram aggregations

3 participants

@MikeGoldsmith @jkwatson @jack-berg