fix(encoding): use +Inf as the implicit histogram overflow bucket bound - #343
Open
gheorghitamutu wants to merge 1 commit into
Open
fix(encoding): use +Inf as the implicit histogram overflow bucket bound#343gheorghitamutu wants to merge 1 commit into
gheorghitamutu wants to merge 1 commit into
Conversation
Signed-off-by: Gheorghita MUTU <gheorghitamutu@gmail.com>
Author
krisztianfekete
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. One follow-up (can be a separate PR if you'd like as it's pre-existent): client_golang strips a trailing +Inf from user-supplied bounds since the bucket is implicit, but Histogram::new([1.0, f64::INFINITY]) now emits two le="+Inf" series.
Filtering infinite bounds in Histogram::new before appending the implicit one would fix this. Let me know if you'd like to include this one here, or if you'd open a new PR/issue for this.
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.
What
Histogram::newappendsf64::MAXas the sentinel for the implicit overflow bucket, and onlyencoding::texttranslates it back tole="+Inf". Both protobuf encoders ship it verbatim, soconsumers receive an overflow bucket whose upper bound is
1.7976931348623157e+308— a very largefinite bound, not an infinity.
This changes the sentinel to
f64::INFINITYrather than translating it in each encoder.Histogram::newis the only construction site (new_classic_and_nativeandHistogramWithExemplars::newboth delegate to it), so one line covers classic, exemplar andclassic+native histograms; the protobuf encoders then need no special case, and the text encoder
only changes what it compares against.
Why it matters
1. Quantiles above the ladder return ~1e308. Prometheus recognises the +Inf bucket with
IsInf, so it never saw one: it ingested the sentinel as an ordinary finite bucket and synthesisedits own
le="+Inf"series fromsample_count. promql'sbucketQuantilenever interpolates intothe +Inf bucket and returns the second-highest bound instead — the sentinel. Every quantile above
the histogram's real ladder therefore returned ~1.1e308 instead of a value. Measured on 0.25.0
scraped by Grafana Alloy into Mimir,
histogram_quantile(0.99, ...)returned1.0968850044231772e+308, which Grafana renders as "5.58e+300 years". It is silent: the seriesscrapes cleanly and the panel only goes wrong once a quantile saturates. Scraping protobuf is not
optional for anyone using native histograms, since that is the only exposition offering them.
2.
+Infobservations were counted in no bucket.observe_classicroutes NaN to the lastbucket explicitly, but everything else through
find(|(upper_bound, _)| upper_bound >= &v).Since
f64::MAX >= f64::INFINITYis false, an infinite observation incrementedsumandcountwhile incrementing no bucket at all, leaving the overflow bucket short of
_count. Prometheus'ssynthetic +Inf series masked that too. Fixing only the encoders would have made this short count
visible rather than fixing it.
Why +Inf is the correct value
metrics.protodocuments this field as holding a "+Inf bucket" (optional, inclusive).client_golangwritesmath.Inf(1)whenever it emits that bucket, and Prometheus branches onmath.IsInf.+Infthreshold,so the OpenMetrics encoder was in outright violation.
doubleas a raw IEEE-754 fixed64, so infinity is bit-exact on the wire(
00 00 00 00 00 00 F0 7F) and decodes back to +Inf on the Go side.The native-histogram
positive_upper_boundreturningf64::MAXis deliberately left alone — itmirrors client_golang's
getBoundand is not on any wire path.Testing
Adds a regression test asserting that every observation, including
+InfandNaN, is counted insome bucket. It fails before this change with
buckets=[(1.0, 1), (2.0, 0), (1.7976931348623157e308, 1)] count=3.cargo fmt --all -- --checkis clean,cargo clippy --locked --workspace --all-targets -- -D warningsreports nothing, and
cargo test --locked --all --all-featurespasses 91/91 with the Pythonprometheus-clientdependency from CONTRIBUTING.md installed.