From 3d7cf4c274bdc0c5ca476383b21944c9f4f64275 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 30 Sep 2025 19:03:46 +0200 Subject: [PATCH] model/histogram: Validate non-negative count and zero bucket We have always validated that none of the bucket is negative. We should do the same for the count of observations and the zero bucket. Note that this was always implied in the protobuf exposition format because a count or a zero bucket population is ignored if it is not positive. Signed-off-by: beorn7 --- model/histogram/float_histogram.go | 6 ++ model/histogram/float_histogram_test.go | 94 +++++++++++++++++++++++++ model/histogram/generic.go | 1 + storage/remote/write_handler.go | 1 + 4 files changed, 102 insertions(+) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 0f89ec38fa..009dcc82cd 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -826,12 +826,18 @@ func (h *FloatHistogram) Validate() error { if err != nil { return fmt.Errorf("negative side: %w", err) } + if h.ZeroCount < 0 { + return fmt.Errorf("zero bucket has observation count of %v: %w", h.ZeroCount, ErrHistogramNegativeBucketCount) + } if h.CustomValues != nil { return ErrHistogramExpSchemaCustomBounds } default: return InvalidSchemaError(h.Schema) } + if h.Count < 0 { + return fmt.Errorf("observation count is %v: %w", h.Count, ErrHistogramNegativeCount) + } err := checkHistogramBuckets(h.PositiveBuckets, &pCount, false) if err != nil { return fmt.Errorf("positive side: %w", err) diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index d20a049e77..b756f641bb 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -3527,6 +3527,100 @@ func TestFloatHistogramString(t *testing.T) { } } +func TestFloatHistogramValidateNegativeHistogram(t *testing.T) { + cases := []struct { + name string + fh *FloatHistogram + }{ + { + "positive bucket with negative population", + &FloatHistogram{ + Schema: 1, + ZeroThreshold: 0.01, + ZeroCount: 5.5, + Count: 3493.3, + Sum: 2349209.324, + PositiveSpans: []Span{ + {-2, 1}, + {2, 3}, + }, + PositiveBuckets: []float64{1, 3.3, -4.2, 0.1}, + NegativeSpans: []Span{ + {3, 2}, + {3, 2}, + }, + NegativeBuckets: []float64{3.1, 3, 1.234e5, 1000}, + }, + }, + { + "negative bucket with negative population", + &FloatHistogram{ + Schema: 1, + ZeroThreshold: 0.01, + ZeroCount: 5.5, + Count: 3493.3, + Sum: 2349209.324, + PositiveSpans: []Span{ + {-2, 1}, + {2, 3}, + }, + PositiveBuckets: []float64{1, 3.3, 4.2, 0.1}, + NegativeSpans: []Span{ + {3, 2}, + {3, 2}, + }, + NegativeBuckets: []float64{3.1, -3, 1.234e5, 1000}, + }, + }, + { + "negative count", + &FloatHistogram{ + Schema: 1, + ZeroThreshold: 0.01, + ZeroCount: 5.5, + Count: -3493.3, + Sum: 2349209.324, + PositiveSpans: []Span{ + {-2, 1}, + {2, 3}, + }, + PositiveBuckets: []float64{1, 3.3, 4.2, 0.1}, + NegativeSpans: []Span{ + {3, 2}, + {3, 2}, + }, + NegativeBuckets: []float64{3.1, 3, 1.234e5, 1000}, + }, + }, + { + "zero bucket with negative population", + &FloatHistogram{ + Schema: 1, + ZeroThreshold: 0.01, + ZeroCount: -5.5, + Count: 3493.3, + Sum: 2349209.324, + PositiveSpans: []Span{ + {-2, 1}, + {2, 3}, + }, + PositiveBuckets: []float64{1, 3.3, 4.2, 0.1}, + NegativeSpans: []Span{ + {3, 2}, + {3, 2}, + }, + NegativeBuckets: []float64{3.1, 3, 1.234e5, 1000}, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + require.Error(t, c.fh.Validate()) + }) + } +} + func BenchmarkFloatHistogramAllBucketIterator(b *testing.B) { rng := rand.New(rand.NewSource(0)) diff --git a/model/histogram/generic.go b/model/histogram/generic.go index bc4c69d40c..327dada3ca 100644 --- a/model/histogram/generic.go +++ b/model/histogram/generic.go @@ -31,6 +31,7 @@ const ( var ( ErrHistogramCountNotBigEnough = errors.New("histogram's observation count should be at least the number of observations found in the buckets") ErrHistogramCountMismatch = errors.New("histogram's observation count should equal the number of observations found in the buckets (in absence of NaN)") + ErrHistogramNegativeCount = errors.New("histogram's observation count is negative") ErrHistogramNegativeBucketCount = errors.New("histogram has a bucket whose observation count is negative") ErrHistogramSpanNegativeOffset = errors.New("histogram has a span whose offset is negative") ErrHistogramSpansBucketsMismatch = errors.New("histogram spans specify different number of buckets than provided") diff --git a/storage/remote/write_handler.go b/storage/remote/write_handler.go index 266fae86a3..dc058cc9b9 100644 --- a/storage/remote/write_handler.go +++ b/storage/remote/write_handler.go @@ -122,6 +122,7 @@ func isHistogramValidationError(err error) bool { // TODO: Consider adding single histogram error type instead of individual sentinel errors. return errors.Is(err, histogram.ErrHistogramCountMismatch) || errors.Is(err, histogram.ErrHistogramCountNotBigEnough) || + errors.Is(err, histogram.ErrHistogramNegativeCount) || errors.Is(err, histogram.ErrHistogramNegativeBucketCount) || errors.Is(err, histogram.ErrHistogramSpanNegativeOffset) || errors.Is(err, histogram.ErrHistogramSpansBucketsMismatch) ||