goodhistogram: spell out the native histogram bounds table - #10
Open
angles-n-daemons wants to merge 1 commit into
Open
goodhistogram: spell out the native histogram bounds table#10angles-n-daemons wants to merge 1 commit into
angles-n-daemons wants to merge 1 commit into
Conversation
nativeHistogramBounds was computed at init time with math.Pow. math.Pow is not bit-portable: it has architecture-specific implementations, so the table came out one ULP different on arm64 than on amd64, and matched Prometheus's published table on neither. The boundaries are not an internal detail. getLe derives the classic Prometheus bucket upper bounds from them, and those are exported as `le` label values, so a boundary that varies by architecture splits a single bucket into two time series across a mixed-architecture deployment, and makes any golden-file test of the exported format architecture-dependent. Transcribe the table from client_golang instead, which spells the values out as literals for exactly this reason. Boundaries are now bit-identical everywhere and identical to Prometheus's own, so the classic buckets we export line up with what Prometheus computes for the same native bucket. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
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.
nativeHistogramBoundswas computed at init time withmath.Pow:math.Powis not bit-portable — Go ships architecture-specificimplementations — so the table comes out one ULP different on arm64 than on
amd64, and matches Prometheus's published table on neither. (
math.Ldexpisexact, so all of the drift originates in that one call.)
0.70710678118654760.70710678118654760.70710678118654750.84089641525371460.84089641525371450.8408964152537144The magnitude is irrelevant; the identity is not.
getLederives the classicPrometheus bucket upper bounds from this table, and those are exported as
lelabel values. Prometheus keys time series on the exact label string, so a
boundary that varies by architecture splits one bucket into two series across a
mixed-architecture deployment, and
histogram_quantile/rateaggregations seehalf the counts in each. It also makes any golden-file test of the exported
format architecture-dependent — which is how this surfaced.
Transcribe the table from
client_golanginstead, which spells the values outas literals for exactly this reason. Boundaries are now bit-identical across
architectures and identical to Prometheus's own, so the classic buckets we
export line up with what Prometheus computes for the same native bucket.
The literal is typed
[maxSchema + 1][]float64rather than[][]float64so thecompiler enforces that the table covers every schema.
TestNativeHistogramBoundslocks this down: structural invariants, a loose(few-ULP) sanity check against
2^(j / 2^s) / 2, and exact equality against thecanonical values at the entries where
math.Powdisagrees.Verified by exporting the boundaries for an identical config on darwin/arm64 and
linux/amd64 — byte-identical after this change, divergent before.