From f99ecc376ee4c052527abf80f4cc0c3e112f4355 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Sun, 26 Nov 2023 09:26:34 +0200 Subject: [PATCH] Fix FloatHistogram.Add/Sub mutating its argument Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 38 +++++++++++++++++++------ model/histogram/float_histogram_test.go | 6 ++++ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index e0f5d208e..18e5112ce 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -268,12 +268,23 @@ func (h *FloatHistogram) Add(other *FloatHistogram) *FloatHistogram { h.Count += other.Count h.Sum += other.Sum - if other.Schema != h.Schema { - other = other.ReduceResolution(h.Schema) + var ( + otherPositiveSpans = other.PositiveSpans + otherPositiveBuckets = other.PositiveBuckets + otherNegativeSpans = other.NegativeSpans + otherNegativeBuckets = other.NegativeBuckets + ) + + if other.Schema < h.Schema { + panic(fmt.Errorf("cannot add histogram with schema %d to %d", other.Schema, h.Schema)) + } else if other.Schema > h.Schema { + otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false) + otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false) } - h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, false, h.PositiveSpans, h.PositiveBuckets, other.PositiveSpans, other.PositiveBuckets) - h.NegativeSpans, h.NegativeBuckets = addBuckets(h.Schema, h.ZeroThreshold, false, h.NegativeSpans, h.NegativeBuckets, other.NegativeSpans, other.NegativeBuckets) + h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, false, h.PositiveSpans, h.PositiveBuckets, otherPositiveSpans, otherPositiveBuckets) + h.NegativeSpans, h.NegativeBuckets = addBuckets(h.Schema, h.ZeroThreshold, false, h.NegativeSpans, h.NegativeBuckets, otherNegativeSpans, otherNegativeBuckets) + return h } @@ -284,12 +295,23 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { h.Count -= other.Count h.Sum -= other.Sum - if other.Schema != h.Schema { - other = other.ReduceResolution(h.Schema) + var ( + otherPositiveSpans = other.PositiveSpans + otherPositiveBuckets = other.PositiveBuckets + otherNegativeSpans = other.NegativeSpans + otherNegativeBuckets = other.NegativeBuckets + ) + + if other.Schema < h.Schema { + panic(fmt.Errorf("cannot subtract histigram with schema %d to %d", other.Schema, h.Schema)) + } else if other.Schema > h.Schema { + otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false) + otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false) } - h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, true, h.PositiveSpans, h.PositiveBuckets, other.PositiveSpans, other.PositiveBuckets) - h.NegativeSpans, h.NegativeBuckets = addBuckets(h.Schema, h.ZeroThreshold, true, h.NegativeSpans, h.NegativeBuckets, other.NegativeSpans, other.NegativeBuckets) + h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, true, h.PositiveSpans, h.PositiveBuckets, otherPositiveSpans, otherPositiveBuckets) + h.NegativeSpans, h.NegativeBuckets = addBuckets(h.Schema, h.ZeroThreshold, true, h.NegativeSpans, h.NegativeBuckets, otherNegativeSpans, otherNegativeBuckets) + return h } diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index bfe3525fa..cbcd2a1f0 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -1573,9 +1573,12 @@ func TestFloatHistogramAdd(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + in2Copy := c.in2.Copy() require.Equal(t, c.expected, c.in1.Add(c.in2)) // Has it also happened in-place? require.Equal(t, c.expected, c.in1) + // Check that the argument was not mutated. + require.Equal(t, in2Copy, c.in2) }) } } @@ -1659,9 +1662,12 @@ func TestFloatHistogramSub(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + in2Copy := c.in2.Copy() require.Equal(t, c.expected, c.in1.Sub(c.in2)) // Has it also happened in-place? require.Equal(t, c.expected, c.in1) + // Check that the argument was not mutated. + require.Equal(t, in2Copy, c.in2) }) } }