goodhistogram: add WithExactMinMax wrapper for exact extremes - #14
Merged
Merged
Conversation
Bucket-based quantile estimation cannot recover the true smallest and
largest observed values: q=1.0 clamps to the top occupied bucket's edge
and out-of-range observations lose their magnitude entirely. Callers who
need exact extremes (e.g. a truthful "max latency") had no way to get them.
WithExactMinMax wraps a Histogram and additionally tracks exact min/max,
completing an exact {count, sum, min, max} summary (the base already tracks
sum and count exactly, matching Prometheus). It is additive and opt-in:
- Embeds *Histogram, so every existing method still works and the base
recording path is untouched for callers who don't need extremes.
- Record updates min/max via load-then-CAS, which degrades to two relaxed
loads once the extremes stabilize — cheap under contention.
- Min/Max span all observations, including out-of-range and zero/negative
values, mirroring the exact sum. Max may exceed hi and Min may be below
lo by design.
- ExactSnapshot.ValueAtQuantile returns the exact min at q<=0 and max at
q>=1 (which may fall outside [lo, hi]); interior quantiles delegate to
the base trapezoidal estimate. Merge composes via min-of-mins /
max-of-maxes.
The base Histogram, Windowed, and the Prometheus export are unchanged;
windowed histograms keep their existing bucket-derived extremes.
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
RaduBerinde
approved these changes
Sep 2, 2026
| // (Underflow/Overflow) and zero/negative values, so Max may exceed hi and Min | ||
| // may be below lo. | ||
| type WithExactMinMax struct { | ||
| *Histogram |
Member
There was a problem hiding this comment.
Can this be a non-pointer, so we don't force two allocations?
Also, does it have to be embedded? It would be better to export only the methods that make sense.
Contributor
Author
There was a problem hiding this comment.
Definitely, will change it too to only expose the appropriate subset of methods.
| // ExactSnapshot is a Snapshot with exact extremes. Min and Max are meaningful | ||
| // only when TotalCount > 0. | ||
| type ExactSnapshot struct { | ||
| Snapshot |
Member
There was a problem hiding this comment.
I would not embed this. For example, we get a "sketchy" Sub() Snapshot method.
Contributor
Author
There was a problem hiding this comment.
Good point, removing 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
2 participants
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.
Summary
Bucket-based quantile estimation can't recover the true extremes:
q=1.0clamps to the top occupied bucket's edge, and out-of-range observations lose their magnitude entirely. Callers who need a truthful min/max (e.g. "max latency") had no way to get one.WithExactMinMaxwraps aHistogramand additionally tracks exact min/max, completing an exact{count, sum, min, max}summary. The base already tracks sum and count exactly (matching Prometheus, which maintains an exact_sum/_count), so this only adds min/max.Design
Additive and opt-in — no base API change:
*Histogram, so every existing method still works and the base recording path is untouched for callers who don't opt in.Recordupdates min/max via load-then-CAS, which degrades to two relaxed loads once the extremes stabilize — cheap under contention (writes only while the extreme is climbing).Maxmay exceedhiandMinmay be belowlo— by design; that's precisely what bucket interpolation can't recover.ExactSnapshot.ValueAtQuantilereturns the exact min atq<=0and max atq>=1(possibly outside[lo, hi]); interior quantiles delegate to the base trapezoidal estimate.Mergecomposes via min-of-mins / max-of-maxes.Empty vs. single-zero is disambiguated by the
minVal <= maxValsentinel invariant (MaxInt64/MinInt64before any record), which are also identities for min/max so merges stay clean.Scope / out of scope
Histogram,Windowed, and the Prometheus export are unchanged. Windowed histograms keep their existing bucket-derived extremes (exact windowed extremes would need a per-interval min/max ring, since the windowed view is derived by subtraction and a max isn't subtractable — a possible follow-up).Tests
exact_minmax_test.gocovers: exact summary incl. out-of-range/negative/zero values, empty and single-zero edge cases, quantile endpoints (exact min/max even outside range) for both single and batch paths,Merge(incl. empty operands),Reset, concurrent correctness (race-clean), and that the embedded base behavior (sum/count,Mean, Prometheus export) is intact.