Skip to content

fix: raise ValueError for unsorted ExplicitBucketHistogramAggregation boundaries#5340

Open
RudraDudhat2509 wants to merge 11 commits into
open-telemetry:mainfrom
RudraDudhat2509:fix/explicit-bucket-histogram-unsorted-boundaries
Open

fix: raise ValueError for unsorted ExplicitBucketHistogramAggregation boundaries#5340
RudraDudhat2509 wants to merge 11 commits into
open-telemetry:mainfrom
RudraDudhat2509:fix/explicit-bucket-histogram-unsorted-boundaries

Conversation

@RudraDudhat2509

@RudraDudhat2509 RudraDudhat2509 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #5338

What's wrong

ExplicitBucketHistogramAggregation accepts unsorted boundary lists without any validation. bisect_left (used in aggregate()) requires sorted input with unsorted boundaries, measurements are silently placed in the wrong buckets with no error or warning.

What this does

Adds a validation loop in _ExplicitBucketHistogramAggregation.__init__ that raises ValueError if boundaries are not strictly increasing. This matches the behavior of the Java SDK (IllegalArgumentException) and the Go SDK (non-monotonic boundaries error).

Tests

Added test_unsorted_boundaries_raise and test_duplicate_boundaries_raise to TestExplicitBucketHistogramAggregation.

@RudraDudhat2509 RudraDudhat2509 requested a review from a team as a code owner June 22, 2026 13:52
RudraDudhat2509 added a commit to RudraDudhat2509/opentelemetry-python that referenced this pull request Jun 22, 2026
@RudraDudhat2509

Copy link
Copy Markdown
Contributor Author
image

tests pass on local

Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
@xrmx xrmx moved this to Reviewed PRs that need fixes in Python PR digest Jun 22, 2026
@RudraDudhat2509

RudraDudhat2509 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

good call added finite checks for NaN, +inf, and -inf, matching Java's full validation. tests added for all three cases

Comment thread .changelog/5340.fixed Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
@RudraDudhat2509

RudraDudhat2509 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

good catch, updated to match that pattern, also improved the error messages to show the actual violating values, and the validation check is shifted to the top so there is no half object intialization

Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
@RudraDudhat2509

Copy link
Copy Markdown
Contributor Author

Pushed the update for simplification, lmk if anything else comes up

@herin049

Copy link
Copy Markdown
Contributor

Pushed the update for simplification, lmk if anything else comes up

You might need to fix up the imports, otherwise LGTM!

Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
RudraDudhat2509 added a commit to RudraDudhat2509/opentelemetry-python that referenced this pull request Jun 25, 2026
@RudraDudhat2509 RudraDudhat2509 force-pushed the fix/explicit-bucket-histogram-unsorted-boundaries branch from 9c4ab66 to 4f119c9 Compare June 25, 2026 18:51
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
Comment thread opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Outdated
RudraDudhat2509 and others added 11 commits June 26, 2026 11:36
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
…egation.py


accidentally changed this because i used inf without importing math previously so i replaced all instances of 'inf' with math.inf

Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
@RudraDudhat2509 RudraDudhat2509 force-pushed the fix/explicit-bucket-histogram-unsorted-boundaries branch from 7df0782 to 295a35c Compare June 26, 2026 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

ExplicitBucketHistogramAggregation silently accepts unsorted boundaries, producing wrong bucket assignments

3 participants