Merge pull request #12891 from linasm/fix-gaps-in-histogram-equals

Fix NaN checks in [Float]Histogram.Equals method

model/histogram/float_histogram.go

@@ -307,13 +307,17 @@ 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, 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
 	}
 
 	if h.Schema != h2.Schema || h.ZeroThreshold != h2.ZeroThreshold ||
-		h.ZeroCount != h2.ZeroCount || h.Count != h2.Count || h.Sum != h2.Sum {
+		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
 	}
 
@@ -324,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
 	}
 
@@ -1102,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
+}

model/histogram/float_histogram_test.go

@@ -2291,3 +2291,53 @@ 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,
+		PositiveBuckets: []float64{3},
+		NegativeBuckets: []float64{4},
+	}
+
+	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)
+
+	// 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)
+}

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

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,13 +174,16 @@ 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.
+// 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
 	}
 
 	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
 	}
 
@@ -189,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
 	}
 

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,21 @@ func TestHistogramMatches(t *testing.T) {
 	})
 	h2.NegativeBuckets = append(h2.NegativeBuckets, 1)
 	notEquals(h1, *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) {