record_min_max=False exports histogram min=+Infinity and max=-Infinity - #5571
Open
dwin-gharibi wants to merge 2 commits into
Open
record_min_max=False exports histogram min=+Infinity and max=-Infinity#5571dwin-gharibi wants to merge 2 commits into
dwin-gharibi wants to merge 2 commits into
Conversation
The aggregators seed min/max with +inf/-inf. Assert those sentinels never reach a data point when record_min_max is disabled: the SDK fields are None, the OTLP fields are absent, to_json stays valid JSON, and the delta-to-cumulative merge does not resurrect them. Both histogram aggregations and both temporalities are covered, along with the record_min_max=True path to guard against over-correcting. These tests fail against the current implementation.
Both histogram aggregations seed _min/_max with +inf/-inf and never consulted record_min_max in collect(). With min/max recording disabled the sentinels reached the data point, and the OTLP encoder set both optional fields as present -- so backends received a histogram with min=+Infinity and max=-Infinity, violating the min <= max invariant. to_json emitted the bare literals Infinity and -Infinity, which are not valid JSON. Report None instead, in both aggregations and in both delta-to-cumulative merge branches, and widen the data point fields to float | None. Protobuf treats None as "field not set", so no encoder change is required.
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-23 16:19 UTC Review the latest changes. Status above doesn't look right?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5570.
Description
Both histogram aggregations seed
_min/_maxwithmath.infand-math.infand never consultself._record_min_maxincollect(). With min/max recording disabled the sentinels are handed straight to the data point, and the OTLP encoder sets both optional fields as present - so a backend receives a histogram whose minimum is+Infinityand maximum is-Infinity.to_jsonhas the same problem and emits the bare literalsInfinityand-Infinity, which are not valid JSON.Root cause
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py._ExplicitBucketHistogramAggregation.collectreadsself._min/self._maxat lines 498-499 and_ExponentialBucketHistogramAggregation.collectat lines 767-768, neither guarded byself._record_min_max. The delta-to-cumulative merge at lines 542 and 927 then folds the sentinels into_previous_min/_previous_max, so they persist across collections.Approach
Read
Noneinstead of the sentinel whenrecord_min_maxis disabled, in both aggregations and in both cumulative merge branches. WidenHistogramDataPoint.min/maxandExponentialHistogramDataPoint.min/maxtofloat | None.No encoder change is needed: protobuf treats
Noneas "field not set", which is exactly the intended wire representation.Files changed
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.pyopentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/point.pyopentelemetry-sdk/tests/metrics/test_histogram_record_min_max.py.changelog/5566.fixedTesting
A dedicated module drives both aggregations through a real reader and asserts the SDK fields are
None, the OTLP fields reportHasField(...) == False, andto_jsonsurvives a strict RFC 8259 parser that rejectsInfinity.The delta-to-cumulative merge gets its own test across two collections, since that is where the sentinels were being carried forward, and DELTA temporality is covered separately.
A second class pins the
record_min_max=Truepath - values recorded, cumulative min/max spanning collections, OTLP fields present - so the fix cannot silently disable the default behaviour.Nine of the sixteen fail before the change.
Result: 863 passed in
opentelemetry-sdk, 20 passed inopentelemetry-exporter-otlp-proto-common.Risk / compatibility
HistogramDataPoint.min/maxmay now beNone. Anything reading them unconditionally would previously have receivedinf/-infin this configuration, which was already unusable, so no correct consumer regresses. The defaultrecord_min_max=Truepath is untouched.