From cbd01fc296a48f0512d8c071b70c24dac5e21c0d Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Mon, 25 Sep 2023 16:03:55 +0300 Subject: [PATCH 1/5] Fix NaN sum check in [Float]Histogram.Equals method Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 3 ++- model/histogram/histogram.go | 3 ++- model/histogram/histogram_test.go | 12 ++++++++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 41873278c..6272b6683 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -313,7 +313,8 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.Schema != h2.Schema || h.ZeroThreshold != h2.ZeroThreshold || - h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || h.Sum != h2.Sum { + h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || + math.Float64bits(h.Sum) != math.Float64bits(h2.Sum) { return false } diff --git a/model/histogram/histogram.go b/model/histogram/histogram.go index 6d425307c..31c3ce9fe 100644 --- a/model/histogram/histogram.go +++ b/model/histogram/histogram.go @@ -178,7 +178,8 @@ func (h *Histogram) Equals(h2 *Histogram) bool { } if h.Schema != h2.Schema || h.ZeroThreshold != h2.ZeroThreshold || - h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || h.Sum != h2.Sum { + h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || + math.Float64bits(h.Sum) != math.Float64bits(h2.Sum) { return false } diff --git a/model/histogram/histogram_test.go b/model/histogram/histogram_test.go index 3975353d2..2af90501e 100644 --- a/model/histogram/histogram_test.go +++ b/model/histogram/histogram_test.go @@ -19,6 +19,8 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/prometheus/prometheus/model/value" ) func TestHistogramString(t *testing.T) { @@ -411,8 +413,8 @@ func TestHistogramToFloat(t *testing.T) { require.Equal(t, h.String(), fh.String()) } -// TestHistogramMatches tests both Histogram and FloatHistogram. -func TestHistogramMatches(t *testing.T) { +// TestHistogramEquals tests both Histogram and FloatHistogram. +func TestHistogramEquals(t *testing.T) { h1 := Histogram{ Schema: 3, Count: 61, @@ -537,6 +539,12 @@ func TestHistogramMatches(t *testing.T) { }) h2.NegativeBuckets = append(h2.NegativeBuckets, 1) notEquals(h1, *h2) + + // StaleNaN. + h2 = h1.Copy() + h2.Sum = math.Float64frombits(value.StaleNaN) + notEquals(h1, *h2) + equals(*h2, *h2) } func TestHistogramCompact(t *testing.T) { From dfb62926008e85310bc05cbf6b132cfb98662173 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Thu, 28 Sep 2023 09:06:54 +0300 Subject: [PATCH 2/5] Compare FloatHistogram.[Zero]Count float values as binary Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 3 ++- model/histogram/float_histogram_test.go | 36 +++++++++++++++++++++++++ model/histogram/histogram_test.go | 19 +++++++++---- 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 6272b6683..54ff5d4df 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -313,7 +313,8 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { } if h.Schema != h2.Schema || h.ZeroThreshold != h2.ZeroThreshold || - h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || + math.Float64bits(h.ZeroCount) != math.Float64bits(h2.ZeroCount) || + math.Float64bits(h.Count) != math.Float64bits(h2.Count) || math.Float64bits(h.Sum) != math.Float64bits(h2.Sum) { return false } diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index 0b712be43..61e8fc515 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -2291,3 +2291,39 @@ func TestFloatBucketIteratorTargetSchema(t *testing.T) { } require.False(t, it.Next(), "negative iterator not exhausted") } + +// TestFloatHistogramEquals tests FloatHistogram with float-specific cases that +// cannot be covered by TestHistogramEquals. +func TestFloatHistogramEquals(t *testing.T) { + h1 := FloatHistogram{ + Schema: 3, + Count: 2.2, + Sum: 9.7, + ZeroThreshold: 0.1, + ZeroCount: 1.1, + } + + equals := func(h1, h2 FloatHistogram) { + require.True(t, h1.Equals(&h2)) + require.True(t, h2.Equals(&h1)) + } + notEquals := func(h1, h2 FloatHistogram) { + require.False(t, h1.Equals(&h2)) + require.False(t, h2.Equals(&h1)) + } + + h2 := h1.Copy() + equals(h1, *h2) + + // Count is NaN (but not a StaleNaN). + hCountNaN := h1.Copy() + hCountNaN.Count = math.NaN() + notEquals(h1, *hCountNaN) + equals(*hCountNaN, *hCountNaN) + + // ZeroCount is NaN (but not a StaleNaN). + hZeroCountNaN := h1.Copy() + hZeroCountNaN.ZeroCount = math.NaN() + notEquals(h1, *hZeroCountNaN) + equals(*hZeroCountNaN, *hZeroCountNaN) +} diff --git a/model/histogram/histogram_test.go b/model/histogram/histogram_test.go index 2af90501e..23fb1779e 100644 --- a/model/histogram/histogram_test.go +++ b/model/histogram/histogram_test.go @@ -540,11 +540,20 @@ func TestHistogramEquals(t *testing.T) { h2.NegativeBuckets = append(h2.NegativeBuckets, 1) notEquals(h1, *h2) - // StaleNaN. - h2 = h1.Copy() - h2.Sum = math.Float64frombits(value.StaleNaN) - notEquals(h1, *h2) - equals(*h2, *h2) + // Sum is StaleNaN. + hStale := h1.Copy() + hStale.Sum = math.Float64frombits(value.StaleNaN) + notEquals(h1, *hStale) + equals(*hStale, *hStale) + + // Sum is NaN (but not a StaleNaN). + hNaN := h1.Copy() + hNaN.Sum = math.NaN() + notEquals(h1, *hNaN) + equals(*hNaN, *hNaN) + + // Sum StaleNaN vs regular NaN. + notEquals(*hStale, *hNaN) } func TestHistogramCompact(t *testing.T) { From 3c047a35184db4ff351dbf29f0ee9860b063c313 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Thu, 28 Sep 2023 09:08:09 +0300 Subject: [PATCH 3/5] Expand docs comments Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 1 + model/histogram/histogram.go | 1 + 2 files changed, 2 insertions(+) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 54ff5d4df..f1cb6e5a9 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -307,6 +307,7 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. +// Non-metadata float fields (Sum, Count, ZeroCount) are compared as binary (using math.Float64bits). func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { if h2 == nil { return false diff --git a/model/histogram/histogram.go b/model/histogram/histogram.go index 31c3ce9fe..88883472d 100644 --- a/model/histogram/histogram.go +++ b/model/histogram/histogram.go @@ -172,6 +172,7 @@ func (h *Histogram) CumulativeBucketIterator() BucketIterator[uint64] { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. +// Non-metadata float field (Sum) is compared as binary (using math.Float64bits). func (h *Histogram) Equals(h2 *Histogram) bool { if h2 == nil { return false From ec823d9daf24ed2a9c3d9176759be78b6e0e1c52 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Mon, 9 Oct 2023 16:09:46 +0300 Subject: [PATCH 4/5] Update comments, bitwise comparison of float buckets Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 19 ++++++++++++++++--- model/histogram/float_histogram_test.go | 24 +++++++++++++++++++----- model/histogram/generic.go | 12 ------------ model/histogram/histogram.go | 9 ++++++--- 4 files changed, 41 insertions(+), 23 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index f1cb6e5a9..e6e6ec684 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -307,7 +307,8 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. -// Non-metadata float fields (Sum, Count, ZeroCount) are compared as binary (using math.Float64bits). +// Sum, Count, and ZeroCount are compared based on their bit patterns because this method +// is about data equality rather than mathematical equality. func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { if h2 == nil { return false @@ -327,10 +328,10 @@ func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { return false } - if !bucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { + if !floatBucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { return false } - if !bucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { + if !floatBucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { return false } @@ -1105,3 +1106,15 @@ func addBuckets( return spansA, bucketsA } + +func floatBucketsMatch(b1, b2 []float64) bool { + if len(b1) != len(b2) { + return false + } + for i, b := range b1 { + if math.Float64bits(b) != math.Float64bits(b2[i]) { + return false + } + } + return true +} diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index 61e8fc515..ae8ba3ea2 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -2296,11 +2296,13 @@ func TestFloatBucketIteratorTargetSchema(t *testing.T) { // cannot be covered by TestHistogramEquals. func TestFloatHistogramEquals(t *testing.T) { h1 := FloatHistogram{ - Schema: 3, - Count: 2.2, - Sum: 9.7, - ZeroThreshold: 0.1, - ZeroCount: 1.1, + Schema: 3, + Count: 2.2, + Sum: 9.7, + ZeroThreshold: 0.1, + ZeroCount: 1.1, + PositiveBuckets: []float64{3}, + NegativeBuckets: []float64{4}, } equals := func(h1, h2 FloatHistogram) { @@ -2326,4 +2328,16 @@ func TestFloatHistogramEquals(t *testing.T) { hZeroCountNaN.ZeroCount = math.NaN() notEquals(h1, *hZeroCountNaN) equals(*hZeroCountNaN, *hZeroCountNaN) + + // Positive bucket value is NaN. + hPosBucketNaN := h1.Copy() + hPosBucketNaN.PositiveBuckets[0] = math.NaN() + notEquals(h1, *hPosBucketNaN) + equals(*hPosBucketNaN, *hPosBucketNaN) + + // Negative bucket value is NaN. + hNegBucketNaN := h1.Copy() + hNegBucketNaN.NegativeBuckets[0] = math.NaN() + notEquals(h1, *hNegBucketNaN) + equals(*hNegBucketNaN, *hNegBucketNaN) } diff --git a/model/histogram/generic.go b/model/histogram/generic.go index dad54cb06..48fb906ce 100644 --- a/model/histogram/generic.go +++ b/model/histogram/generic.go @@ -333,18 +333,6 @@ func compactBuckets[IBC InternalBucketCount](buckets []IBC, spans []Span, maxEmp return buckets, spans } -func bucketsMatch[IBC InternalBucketCount](b1, b2 []IBC) bool { - if len(b1) != len(b2) { - return false - } - for i, b := range b1 { - if b != b2[i] { - return false - } - } - return true -} - func getBound(idx, schema int32) float64 { // Here a bit of context about the behavior for the last bucket counting // regular numbers (called simply "last bucket" below) and the bucket diff --git a/model/histogram/histogram.go b/model/histogram/histogram.go index 88883472d..1a7a2de7f 100644 --- a/model/histogram/histogram.go +++ b/model/histogram/histogram.go @@ -17,6 +17,8 @@ import ( "fmt" "math" "strings" + + "golang.org/x/exp/slices" ) // CounterResetHint contains the known information about a counter reset, @@ -172,7 +174,8 @@ func (h *Histogram) CumulativeBucketIterator() BucketIterator[uint64] { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. -// Non-metadata float field (Sum) is compared as binary (using math.Float64bits). +// Sum is compared based on its bit pattern because this method +// is about data equality rather than mathematical equality. func (h *Histogram) Equals(h2 *Histogram) bool { if h2 == nil { return false @@ -191,10 +194,10 @@ func (h *Histogram) Equals(h2 *Histogram) bool { return false } - if !bucketsMatch(h.PositiveBuckets, h2.PositiveBuckets) { + if !slices.Equal(h.PositiveBuckets, h2.PositiveBuckets) { return false } - if !bucketsMatch(h.NegativeBuckets, h2.NegativeBuckets) { + if !slices.Equal(h.NegativeBuckets, h2.NegativeBuckets) { return false } From 62bbb81e2924e67ebf7119b9b456806a072a43d6 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Sat, 14 Oct 2023 21:30:40 +0300 Subject: [PATCH 5/5] Mention bucket values in the comment Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index e6e6ec684..6ee72f24e 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -307,8 +307,8 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { // Exact match is when there are no new buckets (even empty) and no missing buckets, // and all the bucket values match. Spans can have different empty length spans in between, // but they must represent the same bucket layout to match. -// Sum, Count, and ZeroCount are compared based on their bit patterns because this method -// is about data equality rather than mathematical equality. +// Sum, Count, ZeroCount and bucket values are compared based on their bit patterns +// because this method is about data equality rather than mathematical equality. func (h *FloatHistogram) Equals(h2 *FloatHistogram) bool { if h2 == nil { return false