From b4f77d015ae844a8083f1ca9e256683993ffa270 Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Fri, 11 Sep 2026 13:12:36 -0400 Subject: [PATCH 1/7] goodhistogram: make snapshots serializable Snapshot currently stores its bucket configuration behind an unexported pointer. Encoders preserve the counts and totals but cannot restore the layout needed for quantiles and Prometheus export. Store the schema and tracked range directly on Snapshot and reconstruct the cached derived configuration when it is consumed. This makes snapshots portable through ordinary JSON encoding without a second public wire type. --- README.md | 9 ++++ export.go | 8 ++-- histogram.go | 80 ++++++++++++++++++++++------------ histogram_test.go | 8 ++-- mapping_test.go | 10 +++-- quantile.go | 50 +++++++++++---------- snapshot_serialization_test.go | 39 +++++++++++++++++ 7 files changed, 141 insertions(+), 63 deletions(-) create mode 100644 snapshot_serialization_test.go diff --git a/README.md b/README.md index 4ec8b28..17e9684 100644 --- a/README.md +++ b/README.md @@ -99,6 +99,15 @@ mean := snap.Mean() count, sum := snap.Total() ``` +Snapshots include their bucket layout and can be serialized directly. + +```go +data, err := json.Marshal(snap) + +var restored goodhistogram.Snapshot +err = json.Unmarshal(data, &restored) +``` + ### Register with Prometheus A histogram can be registered with a Prometheus registry via diff --git a/export.go b/export.go index 5565906..de80523 100644 --- a/export.go +++ b/export.go @@ -42,13 +42,14 @@ func (s *Snapshot) ToPrometheusHistogram() *prometheusgo.Histogram { // +Inf bucket has CumulativeCount == SampleCount, as required by the // Prometheus exposition format. func (s *Snapshot) conventionalBuckets() []*prometheusgo.Bucket { + cfg := s.config() buckets := make([]*prometheusgo.Bucket, 0, len(s.Counts)+1) // Zeros and underflow values are below all bucket upper bounds, // so they contribute to every bucket's cumulative count. cumCount := s.ZeroCount + s.Underflow for i, c := range s.Counts { cumCount += c - ub := s.cfg.boundaries[i+1] + ub := cfg.boundaries[i+1] cc := cumCount buckets = append(buckets, &prometheusgo.Bucket{ CumulativeCount: &cc, @@ -72,7 +73,8 @@ func (s *Snapshot) conventionalBuckets() []*prometheusgo.Bucket { // the mapping from internal indices to Prometheus bucket keys is a simple // offset addition: promKey = internalIndex + config.minKey. func (s *Snapshot) populateNativeFields(h *prometheusgo.Histogram) { - schema := s.cfg.schema + cfg := s.config() + schema := cfg.schema h.Schema = &schema // Zero bucket: values at or below zero. @@ -99,7 +101,7 @@ func (s *Snapshot) populateNativeFields(h *prometheusgo.Histogram) { var offset int32 if len(spans) == 0 { // First span: offset is the absolute Prometheus bucket key. - offset = int32(s.cfg.minKey + i) + offset = int32(cfg.minKey + i) } else { // Subsequent spans: offset is the gap since the previous span ended. offset = int32(gapSinceLastSpan) diff --git a/histogram.go b/histogram.go index aa50fa2..29dd118 100644 --- a/histogram.go +++ b/histogram.go @@ -408,20 +408,36 @@ func (h *Histogram) Record(v int64) { } // Snapshot is a point-in-time, non-atomic copy of a Histogram, suitable for -// quantile computation and export. +// quantile computation, export, and serialization. type Snapshot struct { - cfg *config - Counts []uint64 - ZeroCount uint64 - Underflow uint64 - Overflow uint64 - TotalCount uint64 - TotalSum int64 + // These fields define the bucket layout for Counts. + PrometheusSchema int32 + LowestTrackable float64 + HighestTrackable float64 + Counts []uint64 + ZeroCount uint64 + Underflow uint64 + Overflow uint64 + TotalCount uint64 + TotalSum int64 } // Schema returns the Prometheus native histogram schema (0–8). func (s *Snapshot) Schema() int32 { - return s.cfg.schema + return s.PrometheusSchema +} + +// config reconstructs the derived configuration from the portable fields. +// Configurations are cached by their construction parameters. +func (s *Snapshot) config() *config { + if s.PrometheusSchema < 0 || s.PrometheusSchema > maxSchema { + panic("goodhistogram: invalid snapshot schema") + } + return getOrCreateConfig(Params{ + Lo: s.LowestTrackable, + Hi: s.HighestTrackable, + ErrorBound: schemaRelativeError(s.PrometheusSchema), + }) } // Snapshot returns a point-in-time copy of the histogram. The snapshot is @@ -430,12 +446,14 @@ func (s *Snapshot) Schema() int32 { // Prometheus makes. func (h *Histogram) Snapshot() Snapshot { s := Snapshot{ - cfg: h.cfg, - Counts: make([]uint64, h.cfg.numBuckets), - ZeroCount: h.ZeroCount.Load(), - Underflow: h.Underflow.Load(), - Overflow: h.Overflow.Load(), - TotalSum: h.sum.Load(), + PrometheusSchema: h.cfg.schema, + LowestTrackable: h.cfg.lo, + HighestTrackable: h.cfg.hi, + Counts: make([]uint64, h.cfg.numBuckets), + ZeroCount: h.ZeroCount.Load(), + Underflow: h.Underflow.Load(), + Overflow: h.Overflow.Load(), + TotalSum: h.sum.Load(), } for i := range s.Counts { c := h.counts[i].Load() @@ -459,13 +477,15 @@ func (h *Histogram) Schema() int32 { // in the tick-based windowing pattern. func (s *Snapshot) Merge(other *Snapshot) Snapshot { merged := Snapshot{ - cfg: s.cfg, - Counts: make([]uint64, len(s.Counts)), - ZeroCount: s.ZeroCount + other.ZeroCount, - Underflow: s.Underflow + other.Underflow, - Overflow: s.Overflow + other.Overflow, - TotalCount: s.TotalCount + other.TotalCount, - TotalSum: s.TotalSum + other.TotalSum, + PrometheusSchema: s.PrometheusSchema, + LowestTrackable: s.LowestTrackable, + HighestTrackable: s.HighestTrackable, + Counts: make([]uint64, len(s.Counts)), + ZeroCount: s.ZeroCount + other.ZeroCount, + Underflow: s.Underflow + other.Underflow, + Overflow: s.Overflow + other.Overflow, + TotalCount: s.TotalCount + other.TotalCount, + TotalSum: s.TotalSum + other.TotalSum, } for i := range s.Counts { merged.Counts[i] = s.Counts[i] + other.Counts[i] @@ -479,13 +499,15 @@ func (s *Snapshot) Merge(other *Snapshot) Snapshot { // current cumulative snapshot. func (s *Snapshot) Sub(other *Snapshot) Snapshot { diff := Snapshot{ - cfg: s.cfg, - Counts: make([]uint64, len(s.Counts)), - ZeroCount: s.ZeroCount - other.ZeroCount, - Underflow: s.Underflow - other.Underflow, - Overflow: s.Overflow - other.Overflow, - TotalCount: s.TotalCount - other.TotalCount, - TotalSum: s.TotalSum - other.TotalSum, + PrometheusSchema: s.PrometheusSchema, + LowestTrackable: s.LowestTrackable, + HighestTrackable: s.HighestTrackable, + Counts: make([]uint64, len(s.Counts)), + ZeroCount: s.ZeroCount - other.ZeroCount, + Underflow: s.Underflow - other.Underflow, + Overflow: s.Overflow - other.Overflow, + TotalCount: s.TotalCount - other.TotalCount, + TotalSum: s.TotalSum - other.TotalSum, } for i := range s.Counts { diff.Counts[i] = s.Counts[i] - other.Counts[i] diff --git a/histogram_test.go b/histogram_test.go index be9ca1c..d3ce4b4 100644 --- a/histogram_test.go +++ b/histogram_test.go @@ -513,9 +513,11 @@ func TestQuantileTopBucketDensity(t *testing.T) { n := cfg.numBuckets snap := Snapshot{ - cfg: cfg, - Counts: make([]uint64, n), - TotalCount: 1000, + PrometheusSchema: cfg.schema, + LowestTrackable: cfg.lo, + HighestTrackable: cfg.hi, + Counts: make([]uint64, n), + TotalCount: 1000, } snap.Counts[n-1] = 1000 diff --git a/mapping_test.go b/mapping_test.go index fd32853..26f6705 100644 --- a/mapping_test.go +++ b/mapping_test.go @@ -113,10 +113,12 @@ func TestBucketMappingConsistency(t *testing.T) { // They should be very close since mismatches shift values by at // most one bucket width. refSnap := Snapshot{ - cfg: &cfg, - Counts: refCounts, - TotalCount: newSnap.TotalCount, - TotalSum: newSnap.TotalSum, + PrometheusSchema: cfg.schema, + LowestTrackable: cfg.lo, + HighestTrackable: cfg.hi, + Counts: refCounts, + TotalCount: newSnap.TotalCount, + TotalSum: newSnap.TotalSum, } for _, q := range []float64{0.50, 0.75, 0.90, 0.95, 0.99, 0.999} { refQ := refSnap.ValueAtQuantile(q) diff --git a/quantile.go b/quantile.go index 96f5f1a..53d56a0 100644 --- a/quantile.go +++ b/quantile.go @@ -31,39 +31,40 @@ func (s *Snapshot) ValueAtQuantile(q float64) float64 { if s.TotalCount == 0 { return 0 } + cfg := s.config() // Target rank (fractional, 0-based) across all observations, // including underflow and overflow. rank := q * float64(s.TotalCount) if rank <= 0 { // Return the lower bound of the first non-empty region. if s.ZeroCount+s.Underflow > 0 { - return s.cfg.lo + return cfg.lo } for i, c := range s.Counts { if c > 0 { - return s.cfg.boundaries[i] + return cfg.boundaries[i] } } - return s.cfg.hi + return cfg.hi } if rank >= float64(s.TotalCount) { // Return the upper bound of the last non-empty region. if s.Overflow > 0 { - return s.cfg.hi + return cfg.hi } for i := len(s.Counts) - 1; i >= 0; i-- { if s.Counts[i] > 0 { - return s.cfg.boundaries[i+1] + return cfg.boundaries[i+1] } } - return s.cfg.lo + return cfg.lo } // Underflow and zero observations form an implicit region below lo. // If the quantile falls within this region, clamp to lo. belowLo := float64(s.ZeroCount + s.Underflow) if rank <= belowLo { - return s.cfg.lo + return cfg.lo } // Adjust rank to be relative to the in-range buckets. rank -= belowLo @@ -79,7 +80,7 @@ func (s *Snapshot) ValueAtQuantile(q float64) float64 { // in the overflow region. Clamp to hi, matching Prometheus's // behavior of clamping to the last explicit bucket boundary. if rank > float64(inRangeCount) { - return s.cfg.hi + return cfg.hi } n := len(s.Counts) @@ -88,7 +89,7 @@ func (s *Snapshot) ValueAtQuantile(q float64) float64 { // density[i] = count[i] / width[i] avgDensity := make([]float64, n) for i := range n { - w := s.cfg.boundaries[i+1] - s.cfg.boundaries[i] + w := cfg.boundaries[i+1] - cfg.boundaries[i] if w > 0 && s.Counts[i] > 0 { avgDensity[i] = float64(s.Counts[i]) / w } @@ -114,8 +115,8 @@ func (s *Snapshot) ValueAtQuantile(q float64) float64 { fc := float64(s.Counts[i]) if cumCount+fc >= rank { localRank := rank - cumCount - lo := s.cfg.boundaries[i] - hi := s.cfg.boundaries[i+1] + lo := cfg.boundaries[i] + hi := cfg.boundaries[i+1] w := hi - lo if w <= 0 || fc == 0 { return lo @@ -129,7 +130,7 @@ func (s *Snapshot) ValueAtQuantile(q float64) float64 { cumCount += fc } // Should not reach here, but clamp to upper bound. - return s.cfg.boundaries[n] + return cfg.boundaries[n] } // ValuesAtQuantiles returns the estimated values at the given quantiles @@ -144,6 +145,7 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { if len(qs) == 0 || s.TotalCount == 0 { return results } + cfg := s.config() belowLo := float64(s.ZeroCount + s.Underflow) var inRangeCount uint64 @@ -161,12 +163,12 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { rank := q * float64(s.TotalCount) if rank <= 0 { if s.ZeroCount+s.Underflow > 0 { - results[i] = s.cfg.lo + results[i] = cfg.lo } else { - results[i] = s.cfg.hi + results[i] = cfg.hi for j, c := range s.Counts { if c > 0 { - results[i] = s.cfg.boundaries[j] + results[i] = cfg.boundaries[j] break } } @@ -175,12 +177,12 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { } if rank >= float64(s.TotalCount) { if s.Overflow > 0 { - results[i] = s.cfg.hi + results[i] = cfg.hi } else { - results[i] = s.cfg.lo + results[i] = cfg.lo for j := len(s.Counts) - 1; j >= 0; j-- { if s.Counts[j] > 0 { - results[i] = s.cfg.boundaries[j+1] + results[i] = cfg.boundaries[j+1] break } } @@ -188,12 +190,12 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { continue } if rank <= belowLo { - results[i] = s.cfg.lo + results[i] = cfg.lo continue } adjusted := rank - belowLo if adjusted > float64(inRangeCount) { - results[i] = s.cfg.hi + results[i] = cfg.hi continue } walk = append(walk, walkEntry{idx: i, rank: adjusted}) @@ -213,7 +215,7 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { // Compute densities once (same as ValueAtQuantile). avgDensity := make([]float64, n) for i := range n { - w := s.cfg.boundaries[i+1] - s.cfg.boundaries[i] + w := cfg.boundaries[i+1] - cfg.boundaries[i] if w > 0 && s.Counts[i] > 0 { avgDensity[i] = float64(s.Counts[i]) / w } @@ -234,8 +236,8 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { nextCum := cumCount + fc for wi < len(walk) && nextCum >= walk[wi].rank { localRank := walk[wi].rank - cumCount - lo := s.cfg.boundaries[i] - hi := s.cfg.boundaries[i+1] + lo := cfg.boundaries[i] + hi := cfg.boundaries[i+1] w := hi - lo if w <= 0 || fc == 0 { results[walk[wi].idx] = lo @@ -254,7 +256,7 @@ func (s *Snapshot) ValuesAtQuantiles(qs []float64) []float64 { // Any remaining entries (shouldn't happen, but safety). for ; wi < len(walk); wi++ { - results[walk[wi].idx] = s.cfg.boundaries[n] + results[walk[wi].idx] = cfg.boundaries[n] } return results diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go new file mode 100644 index 0000000..0e2f602 --- /dev/null +++ b/snapshot_serialization_test.go @@ -0,0 +1,39 @@ +// Copyright 2026 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package goodhistogram + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSnapshotSerialization(t *testing.T) { + h := New(Params{Lo: 10, Hi: 10_000, ErrorBound: 0.2}) + for _, value := range []int64{-1, 0, 5, 10, 25, 100, 1_000, 10_000, 20_000} { + h.Record(value) + } + original := h.Snapshot() + + encoded, err := json.Marshal(original) + require.NoError(t, err) + var decoded Snapshot + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Equal(t, original, decoded) + require.Equal(t, original.ValuesAtQuantiles([]float64{0, 0.5, 0.9, 1}), + decoded.ValuesAtQuantiles([]float64{0, 0.5, 0.9, 1})) + require.Equal(t, original.ToPrometheusHistogram(), decoded.ToPrometheusHistogram()) +} From 5c93e5090f14d8e27fcaa4fbfa404343c3bc0884 Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Mon, 14 Sep 2026 10:15:42 -0400 Subject: [PATCH 2/7] goodhistogram: test snapshot restoration Verify that a serialized snapshot carries enough state to rebuild a live histogram and continue recording observations. Keep the reconstruction test-only until a production caller needs the API. --- snapshot_serialization_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index 0e2f602..f345648 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -36,4 +36,34 @@ func TestSnapshotSerialization(t *testing.T) { require.Equal(t, original.ValuesAtQuantiles([]float64{0, 0.5, 0.9, 1}), decoded.ValuesAtQuantiles([]float64{0, 0.5, 0.9, 1})) require.Equal(t, original.ToPrometheusHistogram(), decoded.ToPrometheusHistogram()) + + restored := histogramFromSnapshotForTest(t, decoded) + require.Equal(t, decoded, restored.Snapshot()) + for _, value := range []int64{-2, 0, 9, 50, 20_001} { + h.Record(value) + restored.Record(value) + } + require.Equal(t, h.Snapshot(), restored.Snapshot()) +} + +// histogramFromSnapshotForTest deliberately initializes every mutable field +// in Histogram. This makes the test fail if Snapshot stops carrying enough +// state to restore a histogram and continue recording observations. +func histogramFromSnapshotForTest(t *testing.T, s Snapshot) *Histogram { + t.Helper() + h := New(Params{ + Lo: s.LowestTrackable, + Hi: s.HighestTrackable, + ErrorBound: schemaRelativeError(s.PrometheusSchema), + }) + require.Len(t, s.Counts, len(h.counts)) + for i, count := range s.Counts { + h.counts[i].Store(count) + } + h.ZeroCount.Store(s.ZeroCount) + h.Underflow.Store(s.Underflow) + h.Overflow.Store(s.Overflow) + h.sum.Store(s.TotalSum) + require.Equal(t, s.TotalCount, h.Snapshot().TotalCount) + return h } From c698a8b1c747c098023118687eaf32cc2d98f304 Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Mon, 14 Sep 2026 15:31:20 -0400 Subject: [PATCH 3/7] fixup! goodhistogram: make snapshots serializable Reject malformed snapshot layouts during JSON decoding and expose validation for other wire formats. Export the base snapshot in ExactSnapshot so its histogram state also survives a round trip. --- exact_minmax.go | 28 ++++++++-------- exact_minmax_test.go | 19 ++++++++++- histogram.go | 47 ++++++++++++++++++++++++--- snapshot_json.go | 25 ++++++++++++++ snapshot_serialization_test.go | 59 ++++++++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+), 20 deletions(-) create mode 100644 snapshot_json.go diff --git a/exact_minmax.go b/exact_minmax.go index eadf87f..f62455f 100644 --- a/exact_minmax.go +++ b/exact_minmax.go @@ -98,13 +98,13 @@ func (h *WithExactMinMax) Summary() Summary { // only when Summary().Count > 0. Subtraction is not supported because exact // extremes cannot be recovered by subtracting snapshots. type ExactSnapshot struct { - snapshot Snapshot + Snapshot Snapshot Min int64 Max int64 } func (h *WithExactMinMax) Snapshot() ExactSnapshot { - es := ExactSnapshot{snapshot: h.histogram.Snapshot()} + es := ExactSnapshot{Snapshot: h.histogram.Snapshot()} if mn, mx := h.minVal.Load(), h.maxVal.Load(); mn <= mx { es.Min, es.Max = mn, mx } @@ -114,7 +114,7 @@ func (h *WithExactMinMax) Snapshot() ExactSnapshot { // ValueAtQuantile returns the exact min at q<=0 and max at q>=1 (which may fall // outside [lo, hi]); interior quantiles use the base estimate. func (s *ExactSnapshot) ValueAtQuantile(q float64) float64 { - if s.snapshot.TotalCount == 0 { + if s.Snapshot.TotalCount == 0 { return 0 } if q <= 0 { @@ -123,12 +123,12 @@ func (s *ExactSnapshot) ValueAtQuantile(q float64) float64 { if q >= 1 { return float64(s.Max) } - return s.snapshot.ValueAtQuantile(q) + return s.Snapshot.ValueAtQuantile(q) } func (s *ExactSnapshot) ValuesAtQuantiles(qs []float64) []float64 { - res := s.snapshot.ValuesAtQuantiles(qs) - if s.snapshot.TotalCount == 0 { + res := s.Snapshot.ValuesAtQuantiles(qs) + if s.Snapshot.TotalCount == 0 { return res } for i, q := range qs { @@ -143,11 +143,11 @@ func (s *ExactSnapshot) ValuesAtQuantiles(qs []float64) []float64 { } func (s *ExactSnapshot) Merge(other *ExactSnapshot) ExactSnapshot { - m := ExactSnapshot{snapshot: s.snapshot.Merge(&other.snapshot)} + m := ExactSnapshot{Snapshot: s.Snapshot.Merge(&other.Snapshot)} switch { - case s.snapshot.TotalCount == 0: + case s.Snapshot.TotalCount == 0: m.Min, m.Max = other.Min, other.Max - case other.snapshot.TotalCount == 0: + case other.Snapshot.TotalCount == 0: m.Min, m.Max = s.Min, s.Max default: m.Min = min(s.Min, other.Min) @@ -158,26 +158,26 @@ func (s *ExactSnapshot) Merge(other *ExactSnapshot) ExactSnapshot { // Summary returns the exact count, sum, and extremes in the snapshot. func (s *ExactSnapshot) Summary() Summary { - return Summary{Count: s.snapshot.TotalCount, Sum: s.snapshot.TotalSum, Min: s.Min, Max: s.Max} + return Summary{Count: s.Snapshot.TotalCount, Sum: s.Snapshot.TotalSum, Min: s.Min, Max: s.Max} } // Schema returns the Prometheus native histogram schema (0–8). func (s *ExactSnapshot) Schema() int32 { - return s.snapshot.Schema() + return s.Snapshot.Schema() } // Mean returns the arithmetic mean, or zero for an empty snapshot. func (s *ExactSnapshot) Mean() float64 { - return s.snapshot.Mean() + return s.Snapshot.Mean() } // Total returns the observation count and sum. func (s *ExactSnapshot) Total() (int64, float64) { - return s.snapshot.Total() + return s.Snapshot.Total() } // ToPrometheusHistogram exports the bucket counts, sum, and count. Exact // extremes are not represented in the Prometheus histogram format. func (s *ExactSnapshot) ToPrometheusHistogram() *prometheusgo.Histogram { - return s.snapshot.ToPrometheusHistogram() + return s.Snapshot.ToPrometheusHistogram() } diff --git a/exact_minmax_test.go b/exact_minmax_test.go index 764a6c2..8de2c8e 100644 --- a/exact_minmax_test.go +++ b/exact_minmax_test.go @@ -9,6 +9,7 @@ package goodhistogram import ( + "encoding/json" "math/rand" "sync" "testing" @@ -82,7 +83,7 @@ func TestExactMinMaxQuantileEndpoints(t *testing.T) { // Interior quantiles must delegate unchanged to the base estimate. for _, q := range []float64{0.25, 0.5, 0.75, 0.99} { - require.Equalf(t, snap.snapshot.ValueAtQuantile(q), snap.ValueAtQuantile(q), + require.Equalf(t, snap.Snapshot.ValueAtQuantile(q), snap.ValueAtQuantile(q), "interior q=%.2f must match base estimate", q) } } @@ -205,6 +206,22 @@ func TestExactMinMaxSnapshotMethods(t *testing.T) { require.Equal(t, h.Schema(), ph.GetSchema()) } +func TestExactSnapshotSerialization(t *testing.T) { + h := NewWithExactMinMax(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + for _, value := range []int64{5, 25, 50, 200} { + h.Record(value) + } + original := h.Snapshot() + + encoded, err := json.Marshal(original) + require.NoError(t, err) + var decoded ExactSnapshot + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Equal(t, original, decoded) + require.Equal(t, original.ValuesAtQuantiles([]float64{0, 0.5, 1}), + decoded.ValuesAtQuantiles([]float64{0, 0.5, 1})) +} + func TestExactMinMaxAllocations(t *testing.T) { p := Params{Lo: 1, Hi: 1000, ErrorBound: 0.05} // AllocsPerRun warms the shared config cache before measuring. Retain diff --git a/histogram.go b/histogram.go index 29dd118..05b33bf 100644 --- a/histogram.go +++ b/histogram.go @@ -23,6 +23,7 @@ package goodhistogram import ( + "fmt" "math" "sort" "sync" @@ -427,17 +428,53 @@ func (s *Snapshot) Schema() int32 { return s.PrometheusSchema } -// config reconstructs the derived configuration from the portable fields. -// Configurations are cached by their construction parameters. -func (s *Snapshot) config() *config { +// Validate checks that the snapshot has a valid and internally consistent +// bucket layout and observation count. +func (s *Snapshot) Validate() error { + if _, err := s.layoutConfig(); err != nil { + return err + } + var totalCount uint64 + for _, count := range s.Counts { + totalCount += count + } + totalCount += s.ZeroCount + s.Underflow + s.Overflow + if totalCount != s.TotalCount { + return fmt.Errorf("goodhistogram: snapshot total count %d does not match component count %d", + s.TotalCount, totalCount) + } + return nil +} + +func (s *Snapshot) layoutConfig() (*config, error) { if s.PrometheusSchema < 0 || s.PrometheusSchema > maxSchema { - panic("goodhistogram: invalid snapshot schema") + return nil, fmt.Errorf("goodhistogram: invalid snapshot schema %d", s.PrometheusSchema) } - return getOrCreateConfig(Params{ + if math.IsNaN(s.LowestTrackable) || math.IsInf(s.LowestTrackable, 0) || + math.IsNaN(s.HighestTrackable) || math.IsInf(s.HighestTrackable, 0) || + s.LowestTrackable <= 0 || s.HighestTrackable <= s.LowestTrackable { + return nil, fmt.Errorf("goodhistogram: invalid snapshot bounds: need finite 0 < lowest trackable < highest trackable") + } + cfg := getOrCreateConfig(Params{ Lo: s.LowestTrackable, Hi: s.HighestTrackable, ErrorBound: schemaRelativeError(s.PrometheusSchema), }) + if len(s.Counts) != cfg.numBuckets { + return nil, fmt.Errorf("goodhistogram: snapshot has %d counts, expected %d", + len(s.Counts), cfg.numBuckets) + } + return cfg, nil +} + +// config reconstructs the derived configuration from the portable fields. +// Configurations are cached by their construction parameters. +func (s *Snapshot) config() *config { + cfg, err := s.layoutConfig() + if err != nil { + panic(err) + } + return cfg } // Snapshot returns a point-in-time copy of the histogram. The snapshot is diff --git a/snapshot_json.go b/snapshot_json.go new file mode 100644 index 0000000..7ecdc9b --- /dev/null +++ b/snapshot_json.go @@ -0,0 +1,25 @@ +// Copyright 2026 The Cockroach Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 + +package goodhistogram + +import "encoding/json" + +// UnmarshalJSON decodes and validates a Snapshot. +func (s *Snapshot) UnmarshalJSON(data []byte) error { + type snapshot Snapshot + var decoded snapshot + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + if err := (*Snapshot)(&decoded).Validate(); err != nil { + return err + } + *s = Snapshot(decoded) + return nil +} diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index f345648..106de92 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -16,6 +16,7 @@ package goodhistogram import ( "encoding/json" + "math" "testing" "github.com/stretchr/testify/require" @@ -46,6 +47,64 @@ func TestSnapshotSerialization(t *testing.T) { require.Equal(t, h.Snapshot(), restored.Snapshot()) } +func TestSnapshotValidation(t *testing.T) { + h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + h.Record(50) + valid := h.Snapshot() + require.NoError(t, valid.Validate()) + + testCases := []struct { + name string + mutate func(*Snapshot) + err string + }{ + {"negative schema", func(s *Snapshot) { s.PrometheusSchema = -1 }, "invalid snapshot schema"}, + {"large schema", func(s *Snapshot) { s.PrometheusSchema = maxSchema + 1 }, "invalid snapshot schema"}, + {"NaN lower bound", func(s *Snapshot) { s.LowestTrackable = math.NaN() }, "invalid snapshot bounds"}, + {"infinite upper bound", func(s *Snapshot) { s.HighestTrackable = math.Inf(1) }, "invalid snapshot bounds"}, + {"zero lower bound", func(s *Snapshot) { s.LowestTrackable = 0 }, "invalid snapshot bounds"}, + {"unordered bounds", func(s *Snapshot) { s.HighestTrackable = s.LowestTrackable }, "invalid snapshot bounds"}, + {"short counts", func(s *Snapshot) { s.Counts = s.Counts[:len(s.Counts)-1] }, "counts, expected"}, + {"long counts", func(s *Snapshot) { s.Counts = append(s.Counts, 0) }, "counts, expected"}, + {"incorrect total", func(s *Snapshot) { s.TotalCount++ }, "does not match component count"}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := valid + s.Counts = append([]uint64(nil), valid.Counts...) + tc.mutate(&s) + require.ErrorContains(t, s.Validate(), tc.err) + }) + } +} + +func TestSnapshotUnmarshalRejectsInvalid(t *testing.T) { + h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + h.Record(50) + valid := h.Snapshot() + + testCases := []struct { + name string + mutate func(*Snapshot) + }{ + {"schema", func(s *Snapshot) { s.PrometheusSchema = maxSchema + 1 }}, + {"bounds", func(s *Snapshot) { s.LowestTrackable = 0 }}, + {"counts", func(s *Snapshot) { s.Counts = s.Counts[:len(s.Counts)-1] }}, + {"total", func(s *Snapshot) { s.TotalCount++ }}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + s := valid + s.Counts = append([]uint64(nil), valid.Counts...) + tc.mutate(&s) + encoded, err := json.Marshal(s) + require.NoError(t, err) + var decoded Snapshot + require.Error(t, json.Unmarshal(encoded, &decoded)) + }) + } +} + // histogramFromSnapshotForTest deliberately initializes every mutable field // in Histogram. This makes the test fail if Snapshot stops carrying enough // state to restore a histogram and continue recording observations. From b1200e0e825df0bcf01ff5f24574184a58072c1f Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Tue, 15 Sep 2026 13:23:49 -0400 Subject: [PATCH 4/7] fixup! goodhistogram: make snapshots serializable Check the serialized bucket count before allocating or caching derived configuration. Share the layout arithmetic with newConfig and cover rejected extreme-range payloads and every schema. --- histogram.go | 35 +++++++++++++------------ snapshot_serialization_test.go | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/histogram.go b/histogram.go index 05b33bf..9e24cef 100644 --- a/histogram.go +++ b/histogram.go @@ -127,6 +127,16 @@ type config struct { boundaries []float64 } +// bucketLayout computes the layout without allocating its derived tables. +func bucketLayout(lo, hi float64, schema int32) (minKey, numBuckets int) { + minKey = promBucketKey(lo, schema) + // Skip a zero-width first bucket when lo is exactly on a boundary. + if getLe(minKey, schema) <= lo { + minKey++ + } + return minKey, promBucketKey(hi, schema) - minKey + 1 +} + // newConfig creates a config for the given range [lo, hi] and desired // relative error. The schema is chosen as the tightest Prometheus schema // whose error is at or below desiredError. Panics if lo <= 0, hi <= lo, @@ -136,15 +146,7 @@ func newConfig(lo, hi, desiredError float64) config { panic("goodhistogram: invalid config: need 0 < lo < hi and desiredError > 0") } schema := pickSchema(desiredError) - minKey := promBucketKey(lo, schema) - // If lo lands exactly on a bucket boundary, the first bucket would span - // [lo, lo] — a zero-width degenerate bucket. Skip it so the first bucket - // starts at lo and ends at the next real boundary above it. - if getLe(minKey, schema) <= lo { - minKey++ - } - maxKey := promBucketKey(hi, schema) - numBuckets := maxKey - minKey + 1 + minKey, numBuckets := bucketLayout(lo, hi, schema) // Precompute bucket boundaries for quantile estimation. boundaries := make([]float64, numBuckets+1) @@ -455,16 +457,17 @@ func (s *Snapshot) layoutConfig() (*config, error) { s.LowestTrackable <= 0 || s.HighestTrackable <= s.LowestTrackable { return nil, fmt.Errorf("goodhistogram: invalid snapshot bounds: need finite 0 < lowest trackable < highest trackable") } - cfg := getOrCreateConfig(Params{ + // Reject mismatched counts before allocating and permanently caching tables. + _, numBuckets := bucketLayout(s.LowestTrackable, s.HighestTrackable, s.PrometheusSchema) + if len(s.Counts) != numBuckets { + return nil, fmt.Errorf("goodhistogram: snapshot has %d counts, expected %d", + len(s.Counts), numBuckets) + } + return getOrCreateConfig(Params{ Lo: s.LowestTrackable, Hi: s.HighestTrackable, ErrorBound: schemaRelativeError(s.PrometheusSchema), - }) - if len(s.Counts) != cfg.numBuckets { - return nil, fmt.Errorf("goodhistogram: snapshot has %d counts, expected %d", - len(s.Counts), cfg.numBuckets) - } - return cfg, nil + }), nil } // config reconstructs the derived configuration from the portable fields. diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index 106de92..6d6a6de 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -16,12 +16,59 @@ package goodhistogram import ( "encoding/json" + "fmt" "math" "testing" "github.com/stretchr/testify/require" ) +func TestSnapshotRejectedLayoutDoesNotCacheConfig(t *testing.T) { + for i := uint64(1); i <= 3; i++ { + s := Snapshot{ + PrometheusSchema: maxSchema, + LowestTrackable: math.Float64frombits(i), + HighestTrackable: 1.797e308, + Counts: []uint64{1}, + TotalCount: 1, + } + params := Params{Lo: s.LowestTrackable, Hi: s.HighestTrackable, + ErrorBound: schemaRelativeError(s.PrometheusSchema)} + _, cached := configCache.Load(params) + require.False(t, cached) + t.Cleanup(func() { configCache.Delete(params) }) + + require.ErrorContains(t, s.Validate(), "counts, expected") + _, cached = configCache.Load(params) + require.False(t, cached, "rejected layouts must not populate the config cache") + + encoded, err := json.Marshal(s) + require.NoError(t, err) + var decoded Snapshot + require.ErrorContains(t, json.Unmarshal(encoded, &decoded), "counts, expected") + _, cached = configCache.Load(params) + require.False(t, cached, "rejected JSON must not populate the config cache") + } +} + +func TestSnapshotLayoutSchemas(t *testing.T) { + for schema := int32(0); schema <= maxSchema; schema++ { + for _, bounds := range [][2]float64{{1, 1024}, {1.1, 99.9}, {10, math.Nextafter(10, 11)}} { + t.Run(fmt.Sprintf("schema=%d/bounds=%v", schema, bounds), func(t *testing.T) { + h := New(Params{Lo: bounds[0], Hi: bounds[1], ErrorBound: schemaRelativeError(schema)}) + s := h.Snapshot() + require.Equal(t, schema, s.Schema()) + require.NoError(t, s.Validate()) + encoded, err := json.Marshal(s) + require.NoError(t, err) + var decoded Snapshot + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Equal(t, s, decoded) + }) + } + } +} + func TestSnapshotSerialization(t *testing.T) { h := New(Params{Lo: 10, Hi: 10_000, ErrorBound: 0.2}) for _, value := range []int64{-1, 0, 5, 10, 25, 100, 1_000, 10_000, 20_000} { From 416a7b213b2d03bf08476325b1ffa67dc3b6be7f Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Tue, 15 Sep 2026 13:25:16 -0400 Subject: [PATCH 5/7] fixup! goodhistogram: make snapshots serializable Accept the completely empty snapshot as an unset sentinel, export it without a bucket layout, and treat JSON null as a no-op. Cover nested zero values, populated null destinations, and rejection of partial snapshots. --- README.md | 5 +++ export.go | 4 ++ histogram.go | 12 +++++- snapshot_json.go | 10 ++++- snapshot_serialization_test.go | 70 ++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 17e9684..cc4a338 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,11 @@ var restored goodhistogram.Snapshot err = json.Unmarshal(data, &restored) ``` +JSON decoding validates the snapshot's layout and counts. For other encodings, +call `Validate()` after decoding. The zero value is an unset snapshot: it +round-trips through JSON, returns zero quantiles and a NaN mean, and exports zero +count and sum without a bucket layout. JSON `null` leaves the destination unchanged. + ### Register with Prometheus A histogram can be registered with a Prometheus registry via diff --git a/export.go b/export.go index de80523..bbf9f30 100644 --- a/export.go +++ b/export.go @@ -17,6 +17,7 @@ import ( // ToPrometheusHistogram converts a Snapshot to a prometheusgo.Histogram, // populating both conventional bucket fields (for backward compatibility) and // native histogram sparse fields (for efficient Prometheus scraping). +// An unset snapshot exports zero count and sum without a bucket layout. func (s *Snapshot) ToPrometheusHistogram() *prometheusgo.Histogram { h := &prometheusgo.Histogram{} @@ -24,6 +25,9 @@ func (s *Snapshot) ToPrometheusHistogram() *prometheusgo.Histogram { sampleSum := float64(s.TotalSum) h.SampleCount = &sampleCount h.SampleSum = &sampleSum + if s.isUnset() { + return h + } // Conventional buckets: cumulative counts with upper bounds. h.Bucket = s.conventionalBuckets() diff --git a/histogram.go b/histogram.go index 9e24cef..76d177b 100644 --- a/histogram.go +++ b/histogram.go @@ -412,6 +412,7 @@ func (h *Histogram) Record(v int64) { // Snapshot is a point-in-time, non-atomic copy of a Histogram, suitable for // quantile computation, export, and serialization. +// The zero value represents an unset snapshot with no observations or layout. type Snapshot struct { // These fields define the bucket layout for Counts. PrometheusSchema int32 @@ -431,8 +432,11 @@ func (s *Snapshot) Schema() int32 { } // Validate checks that the snapshot has a valid and internally consistent -// bucket layout and observation count. +// bucket layout and observation count. An unset (zero-value) snapshot is valid. func (s *Snapshot) Validate() error { + if s.isUnset() { + return nil + } if _, err := s.layoutConfig(); err != nil { return err } @@ -448,6 +452,12 @@ func (s *Snapshot) Validate() error { return nil } +func (s *Snapshot) isUnset() bool { + return s.PrometheusSchema == 0 && s.LowestTrackable == 0 && s.HighestTrackable == 0 && + len(s.Counts) == 0 && s.ZeroCount == 0 && s.Underflow == 0 && s.Overflow == 0 && + s.TotalCount == 0 && s.TotalSum == 0 +} + func (s *Snapshot) layoutConfig() (*config, error) { if s.PrometheusSchema < 0 || s.PrometheusSchema > maxSchema { return nil, fmt.Errorf("goodhistogram: invalid snapshot schema %d", s.PrometheusSchema) diff --git a/snapshot_json.go b/snapshot_json.go index 7ecdc9b..46fa410 100644 --- a/snapshot_json.go +++ b/snapshot_json.go @@ -8,10 +8,16 @@ package goodhistogram -import "encoding/json" +import ( + "bytes" + "encoding/json" +) -// UnmarshalJSON decodes and validates a Snapshot. +// UnmarshalJSON decodes and validates a Snapshot. JSON null leaves s unchanged. func (s *Snapshot) UnmarshalJSON(data []byte) error { + if bytes.Equal(bytes.TrimSpace(data), []byte("null")) { + return nil + } type snapshot Snapshot var decoded snapshot if err := json.Unmarshal(data, &decoded); err != nil { diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index 6d6a6de..912a1c0 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -152,6 +152,76 @@ func TestSnapshotUnmarshalRejectsInvalid(t *testing.T) { } } +func TestSnapshotZeroValue(t *testing.T) { + var zero Snapshot + t.Run("round trip", func(t *testing.T) { + type envelope struct { + Snapshot Snapshot + Exact ExactSnapshot + } + original := envelope{} + encoded, err := json.Marshal(original) + require.NoError(t, err) + var decoded envelope + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Equal(t, original, decoded) + }) + t.Run("consumers", func(t *testing.T) { + require.NoError(t, zero.Validate()) + require.Zero(t, zero.ValueAtQuantile(0.5)) + require.Equal(t, []float64{0, 0, 0}, zero.ValuesAtQuantiles([]float64{0, 0.5, 1})) + require.True(t, math.IsNaN(zero.Mean())) + count, sum := zero.Total() + require.Zero(t, count) + require.Zero(t, sum) + require.Zero(t, zero.Schema()) + for _, s := range []Snapshot{zero.Merge(&zero), zero.Sub(&zero)} { + require.NoError(t, s.Validate()) + } + exported := zero.ToPrometheusHistogram() + require.Zero(t, exported.GetSampleCount()) + require.Zero(t, exported.GetSampleSum()) + require.Empty(t, exported.Bucket) + require.Nil(t, exported.Schema, "an unset snapshot has no native bucket layout") + }) + t.Run("empty counts", func(t *testing.T) { + var decoded Snapshot + require.NoError(t, json.Unmarshal([]byte(`{"Counts":[]}`), &decoded)) + require.NoError(t, decoded.Validate()) + require.Equal(t, zero.ToPrometheusHistogram(), decoded.ToPrometheusHistogram()) + }) + t.Run("partial snapshots remain invalid", func(t *testing.T) { + for _, payload := range []string{ + `{"PrometheusSchema":1}`, `{"LowestTrackable":1}`, `{"HighestTrackable":100}`, + `{"Counts":[0]}`, `{"ZeroCount":1}`, `{"Underflow":1}`, `{"Overflow":1}`, + `{"TotalCount":1}`, `{"TotalSum":1}`, + } { + t.Run(payload, func(t *testing.T) { + var decoded Snapshot + require.Error(t, json.Unmarshal([]byte(payload), &decoded)) + }) + } + }) +} + +func TestSnapshotUnmarshalNull(t *testing.T) { + h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + h.Record(50) + for _, original := range []Snapshot{{}, h.Snapshot()} { + decoded := original + require.NoError(t, json.Unmarshal([]byte(" \nnull\t"), &decoded)) + require.Equal(t, original, decoded) + // The method also accepts whitespace when called directly. + require.NoError(t, decoded.UnmarshalJSON([]byte(" \nnull\t"))) + require.Equal(t, original, decoded) + } + for _, payload := range []string{`{"Min":1,"Max":2}`, `{"Snapshot":null,"Min":1,"Max":2}`} { + var decoded ExactSnapshot + require.NoError(t, json.Unmarshal([]byte(payload), &decoded)) + require.Equal(t, ExactSnapshot{Min: 1, Max: 2}, decoded) + } +} + // histogramFromSnapshotForTest deliberately initializes every mutable field // in Histogram. This makes the test fail if Snapshot stops carrying enough // state to restore a histogram and continue recording observations. From 252fbc4de62de4d27bdc2a20d1f742a71a4992ac Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Tue, 15 Sep 2026 13:27:15 -0400 Subject: [PATCH 6/7] fixup! goodhistogram: make snapshots serializable Check schema, bounds, and bucket count before merging or subtracting snapshots. Reject incompatible layouts in either operand order and document how to initialize a configured empty accumulator. --- README.md | 5 ++++ histogram.go | 25 ++++++++++++---- snapshot_serialization_test.go | 55 ++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index cc4a338..8c97ef6 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,11 @@ call `Validate()` after decoding. The zero value is an unset snapshot: it round-trips through JSON, returns zero quantiles and a NaN mean, and exports zero count and sum without a bucket layout. JSON `null` leaves the destination unchanged. +`Merge` and `Sub` require matching schemas, bounds, and bucket counts; they panic +on a mismatch. An unset snapshot has no layout, so it cannot be combined with a +configured snapshot. To initialize an empty accumulator, take a snapshot of an +empty histogram with the intended parameters. + ### Register with Prometheus A histogram can be registered with a Prometheus registry via diff --git a/histogram.go b/histogram.go index 76d177b..f473cc7 100644 --- a/histogram.go +++ b/histogram.go @@ -521,11 +521,21 @@ func (h *Histogram) Schema() int32 { return h.cfg.schema } +func (s *Snapshot) sameLayout(other *Snapshot) bool { + return s.PrometheusSchema == other.PrometheusSchema && + s.LowestTrackable == other.LowestTrackable && + s.HighestTrackable == other.HighestTrackable && + len(s.Counts) == len(other.Counts) +} + // Merge returns a new Snapshot whose counts are the element-wise sum of s -// and other. Both snapshots must share the same config (same schema and -// bucket boundaries). This is used to merge prev and cur window snapshots -// in the tick-based windowing pattern. +// and other. It panics if the snapshots have different bucket layouts +// (schema, bounds, or count lengths). This is used to merge prev and cur window +// snapshots in the tick-based windowing pattern. func (s *Snapshot) Merge(other *Snapshot) Snapshot { + if !s.sameLayout(other) { + panic("goodhistogram: cannot merge snapshots with different bucket layouts") + } merged := Snapshot{ PrometheusSchema: s.PrometheusSchema, LowestTrackable: s.LowestTrackable, @@ -544,10 +554,13 @@ func (s *Snapshot) Merge(other *Snapshot) Snapshot { } // Sub returns a new Snapshot whose counts are the element-wise difference -// of s minus other. Both snapshots must share the same config. This is used -// to compute windowed views by subtracting a baseline snapshot from a -// current cumulative snapshot. +// of s minus other. It panics if the snapshots have different bucket layouts. +// This is used to compute windowed views by subtracting a baseline snapshot +// from a current cumulative snapshot. func (s *Snapshot) Sub(other *Snapshot) Snapshot { + if !s.sameLayout(other) { + panic("goodhistogram: cannot subtract snapshots with different bucket layouts") + } diff := Snapshot{ PrometheusSchema: s.PrometheusSchema, LowestTrackable: s.LowestTrackable, diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index 912a1c0..3830370 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -222,6 +222,61 @@ func TestSnapshotUnmarshalNull(t *testing.T) { } } +func TestSnapshotArithmeticRejectsDifferentLayouts(t *testing.T) { + base := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}).Snapshot() + short, long := base, base + short.Counts = short.Counts[:len(short.Counts)-1] + long.Counts = append(append([]uint64(nil), long.Counts...), 0) + for _, tc := range []struct { + name string + other Snapshot + }{ + {"schema", New(Params{Lo: 10, Hi: 100, ErrorBound: 0.2}).Snapshot()}, + {"lower bound", New(Params{Lo: 11, Hi: 100, ErrorBound: 0.5}).Snapshot()}, + {"upper bound", New(Params{Lo: 10, Hi: 101, ErrorBound: 0.5}).Snapshot()}, + {"short counts", short}, + {"long counts", long}, + {"unset", Snapshot{}}, + } { + t.Run(tc.name, func(t *testing.T) { + for name, op := range map[string]func(*Snapshot, *Snapshot) Snapshot{ + "merge": (*Snapshot).Merge, + "subtract": (*Snapshot).Sub, + } { + t.Run(name, func(t *testing.T) { + message := "goodhistogram: cannot " + name + " snapshots with different bucket layouts" + require.PanicsWithValue(t, message, func() { op(&base, &tc.other) }) + require.PanicsWithValue(t, message, func() { op(&tc.other, &base) }) + }) + } + }) + } +} + +func TestSnapshotArithmeticAfterDecode(t *testing.T) { + h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + empty := h.Snapshot() + for _, value := range []int64{-1, 5, 25, 200} { + h.Record(value) + } + original := h.Snapshot() + encoded, err := json.Marshal(original) + require.NoError(t, err) + var decoded Snapshot + require.NoError(t, json.Unmarshal(encoded, &decoded)) + + merged := original.Merge(&decoded) + for _, value := range []int64{-1, 5, 25, 200} { + h.Record(value) + } + require.Equal(t, h.Snapshot(), merged) + require.NoError(t, merged.Validate()) + require.Equal(t, original, merged.Sub(&decoded)) + require.Equal(t, original, empty.Merge(&decoded)) + require.Equal(t, original, decoded.Merge(&empty)) + require.Equal(t, original, decoded.Sub(&empty)) +} + // histogramFromSnapshotForTest deliberately initializes every mutable field // in Histogram. This makes the test fail if Snapshot stops carrying enough // state to restore a histogram and continue recording observations. From c968d0a3e0f874d858c539318d0ea2ca69da0468 Mon Sep 17 00:00:00 2001 From: Brian Dillmann Date: Tue, 15 Sep 2026 16:36:56 -0400 Subject: [PATCH 7/7] fixup! goodhistogram: make snapshots serializable Treat unset snapshots as merge identities and allow subtracting an unset baseline, copying counts to preserve independent results. Cover decoded empty snapshots and ExactSnapshot accumulator batches. Document the nondecreasing-counter precondition for Sub and its unchecked underflow behavior. --- README.md | 9 +++--- histogram.go | 34 +++++++++++++++++---- snapshot_serialization_test.go | 54 +++++++++++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 8c97ef6..b2a6f70 100644 --- a/README.md +++ b/README.md @@ -113,10 +113,11 @@ call `Validate()` after decoding. The zero value is an unset snapshot: it round-trips through JSON, returns zero quantiles and a NaN mean, and exports zero count and sum without a bucket layout. JSON `null` leaves the destination unchanged. -`Merge` and `Sub` require matching schemas, bounds, and bucket counts; they panic -on a mismatch. An unset snapshot has no layout, so it cannot be combined with a -configured snapshot. To initialize an empty accumulator, take a snapshot of an -empty histogram with the intended parameters. +An unset snapshot is the identity for `Merge`, so either `var acc Snapshot` or +`var acc ExactSnapshot` can start an accumulator that adopts the first configured +snapshot's layout. `Sub` also accepts an unset operand to subtract, but an unset +receiver cannot subtract a configured snapshot. Otherwise, `Merge` and `Sub` +require matching schemas, bounds, and bucket counts; they panic on a mismatch. ### Register with Prometheus diff --git a/histogram.go b/histogram.go index f473cc7..4da9caa 100644 --- a/histogram.go +++ b/histogram.go @@ -25,6 +25,7 @@ package goodhistogram import ( "fmt" "math" + "slices" "sort" "sync" "sync/atomic" @@ -528,11 +529,23 @@ func (s *Snapshot) sameLayout(other *Snapshot) bool { len(s.Counts) == len(other.Counts) } +func (s Snapshot) clone() Snapshot { + s.Counts = slices.Clone(s.Counts) + return s +} + // Merge returns a new Snapshot whose counts are the element-wise sum of s -// and other. It panics if the snapshots have different bucket layouts -// (schema, bounds, or count lengths). This is used to merge prev and cur window -// snapshots in the tick-based windowing pattern. +// and other. An unset snapshot is the identity: the result copies the other +// operand's layout and counts. Otherwise, it panics if the snapshots have +// different bucket layouts (schema, bounds, or count lengths). This is used to +// merge prev and cur window snapshots in the tick-based windowing pattern. func (s *Snapshot) Merge(other *Snapshot) Snapshot { + if s.isUnset() { + return other.clone() + } + if other.isUnset() { + return s.clone() + } if !s.sameLayout(other) { panic("goodhistogram: cannot merge snapshots with different bucket layouts") } @@ -554,10 +567,19 @@ func (s *Snapshot) Merge(other *Snapshot) Snapshot { } // Sub returns a new Snapshot whose counts are the element-wise difference -// of s minus other. It panics if the snapshots have different bucket layouts. -// This is used to compute windowed views by subtracting a baseline snapshot -// from a current cumulative snapshot. +// of s minus other. An unset other returns a copy of s. Otherwise, it panics +// if the snapshots have different bucket layouts, including when s is unset. +// This is used to compute windowed views by subtracting a baseline snapshot from +// a current cumulative snapshot. +// +// Each unsigned count in s (Counts[i], ZeroCount, Underflow, Overflow, and +// TotalCount) must be at least the corresponding count in other. Subtraction +// does not check this precondition: counts wrap modulo 2^64 on underflow, for +// example if counters reset after the baseline was taken. func (s *Snapshot) Sub(other *Snapshot) Snapshot { + if other.isUnset() { + return s.clone() + } if !s.sameLayout(other) { panic("goodhistogram: cannot subtract snapshots with different bucket layouts") } diff --git a/snapshot_serialization_test.go b/snapshot_serialization_test.go index 3830370..03fbca5 100644 --- a/snapshot_serialization_test.go +++ b/snapshot_serialization_test.go @@ -236,7 +236,6 @@ func TestSnapshotArithmeticRejectsDifferentLayouts(t *testing.T) { {"upper bound", New(Params{Lo: 10, Hi: 101, ErrorBound: 0.5}).Snapshot()}, {"short counts", short}, {"long counts", long}, - {"unset", Snapshot{}}, } { t.Run(tc.name, func(t *testing.T) { for name, op := range map[string]func(*Snapshot, *Snapshot) Snapshot{ @@ -253,6 +252,59 @@ func TestSnapshotArithmeticRejectsDifferentLayouts(t *testing.T) { } } +func TestSnapshotArithmeticWithUnset(t *testing.T) { + h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + for _, value := range []int64{-1, 5, 25, 200} { + h.Record(value) + } + want := h.Snapshot() + for _, payload := range []string{`{}`, `null`, `{"Counts":[]}`} { + t.Run(payload, func(t *testing.T) { + var unset Snapshot + require.NoError(t, json.Unmarshal([]byte(payload), &unset)) + require.NoError(t, unset.Validate()) + original := h.Snapshot() + for _, result := range []Snapshot{ + unset.Merge(&original), original.Merge(&unset), original.Sub(&unset), + } { + require.Equal(t, want, result) + require.NoError(t, result.Validate()) + result.Counts[0]++ + require.Equal(t, want, original, "result counts must not alias the operand") + } + require.PanicsWithValue(t, + "goodhistogram: cannot subtract snapshots with different bucket layouts", + func() { unset.Sub(&original) }) + }) + } +} + +func TestExactSnapshotUnsetAccumulator(t *testing.T) { + for _, payload := range []string{`{}`, `{"Min":1,"Max":2}`, `{"Snapshot":null,"Min":1,"Max":2}`} { + t.Run(payload, func(t *testing.T) { + h := NewWithExactMinMax(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) + h.Record(50) + populated := h.Snapshot() + var unset ExactSnapshot + require.NoError(t, json.Unmarshal([]byte(payload), &unset)) + require.NoError(t, unset.Snapshot.Validate()) + require.Equal(t, populated, unset.Merge(&populated)) + require.Equal(t, populated, populated.Merge(&unset)) + + var acc ExactSnapshot + for _, s := range []ExactSnapshot{populated, unset, populated} { + acc = acc.Merge(&s) + } + h.Record(50) + want := h.Snapshot() + require.Equal(t, want, acc) + require.Equal(t, want.Summary(), acc.Summary()) + require.Equal(t, want.ValueAtQuantile(0.5), acc.ValueAtQuantile(0.5)) + require.Equal(t, want.ToPrometheusHistogram(), acc.ToPrometheusHistogram()) + }) + } +} + func TestSnapshotArithmeticAfterDecode(t *testing.T) { h := New(Params{Lo: 10, Hi: 100, ErrorBound: 0.5}) empty := h.Snapshot()