From a5ff7af3f3737213063651caa549cb86bae42461 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 16 Aug 2023 07:11:13 -0700 Subject: [PATCH] Ignore +/- Inf and NaN for exponential histogram measurement (#4446) * Add acceptance test * Ignore +/- Inf and NaN for expo hist record --- .../aggregate/exponential_histogram.go | 5 ++ .../aggregate/exponential_histogram_test.go | 59 ++++++++++++++++--- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/sdk/metric/internal/aggregate/exponential_histogram.go b/sdk/metric/internal/aggregate/exponential_histogram.go index b46c100de4b..c8f1b630f07 100644 --- a/sdk/metric/internal/aggregate/exponential_histogram.go +++ b/sdk/metric/internal/aggregate/exponential_histogram.go @@ -113,6 +113,11 @@ func newExpoHistogramDataPoint[N int64 | float64](maxSize, maxScale int, noMinMa // record adds a new measurement to the histogram. It will rescale the buckets if needed. func (p *expoHistogramDataPoint[N]) record(v N) { + // Ignore NaN and infinity. + if math.IsInf(float64(v), 0) || math.IsNaN(float64(v)) { + return + } + p.count++ if !p.noMinMax { diff --git a/sdk/metric/internal/aggregate/exponential_histogram_test.go b/sdk/metric/internal/aggregate/exponential_histogram_test.go index f1db4139cdb..4209ffd651d 100644 --- a/sdk/metric/internal/aggregate/exponential_histogram_test.go +++ b/sdk/metric/internal/aggregate/exponential_histogram_test.go @@ -45,10 +45,10 @@ func withHandler(t *testing.T) func() { func TestExpoHistogramDataPointRecord(t *testing.T) { t.Run("float64", testExpoHistogramDataPointRecord[float64]) - t.Run("float64 MinMaxSum", testExpoHistogramDataPointRecordMinMaxSum[float64]) + t.Run("float64 MinMaxSum", testExpoHistogramDataPointRecordMinMaxSumFloat64) t.Run("float64-2", testExpoHistogramDataPointRecordFloat64) t.Run("int64", testExpoHistogramDataPointRecord[int64]) - t.Run("int64 MinMaxSum", testExpoHistogramDataPointRecordMinMaxSum[int64]) + t.Run("int64 MinMaxSum", testExpoHistogramDataPointRecordMinMaxSumInt64) } // TODO: This can be defined in the test after we drop support for go1.19. @@ -171,15 +171,15 @@ type expoHistogramDataPointRecordMinMaxSumTestCase[N int64 | float64] struct { expected expectedMinMaxSum[N] } -func testExpoHistogramDataPointRecordMinMaxSum[N int64 | float64](t *testing.T) { - testCases := []expoHistogramDataPointRecordMinMaxSumTestCase[N]{ +func testExpoHistogramDataPointRecordMinMaxSumInt64(t *testing.T) { + testCases := []expoHistogramDataPointRecordMinMaxSumTestCase[int64]{ { - values: []N{2, 4, 1}, - expected: expectedMinMaxSum[N]{1, 4, 7, 3}, + values: []int64{2, 4, 1}, + expected: expectedMinMaxSum[int64]{1, 4, 7, 3}, }, { - values: []N{4, 4, 4, 2, 16, 1}, - expected: expectedMinMaxSum[N]{1, 16, 31, 6}, + values: []int64{4, 4, 4, 2, 16, 1}, + expected: expectedMinMaxSum[int64]{1, 16, 31, 6}, }, } @@ -188,7 +188,48 @@ func testExpoHistogramDataPointRecordMinMaxSum[N int64 | float64](t *testing.T) restore := withHandler(t) defer restore() - dp := newExpoHistogramDataPoint[N](4, 20, false, false) + dp := newExpoHistogramDataPoint[int64](4, 20, false, false) + for _, v := range tt.values { + dp.record(v) + } + + assert.Equal(t, tt.expected.max, dp.max) + assert.Equal(t, tt.expected.min, dp.min) + assert.Equal(t, tt.expected.sum, dp.sum) + }) + } +} + +func testExpoHistogramDataPointRecordMinMaxSumFloat64(t *testing.T) { + testCases := []expoHistogramDataPointRecordMinMaxSumTestCase[float64]{ + { + values: []float64{2, 4, 1}, + expected: expectedMinMaxSum[float64]{1, 4, 7, 3}, + }, + { + values: []float64{2, 4, 1, math.Inf(1)}, + expected: expectedMinMaxSum[float64]{1, 4, 7, 4}, + }, + { + values: []float64{2, 4, 1, math.Inf(-1)}, + expected: expectedMinMaxSum[float64]{1, 4, 7, 4}, + }, + { + values: []float64{2, 4, 1, math.NaN()}, + expected: expectedMinMaxSum[float64]{1, 4, 7, 4}, + }, + { + values: []float64{4, 4, 4, 2, 16, 1}, + expected: expectedMinMaxSum[float64]{1, 16, 31, 6}, + }, + } + + for _, tt := range testCases { + t.Run(fmt.Sprint(tt.values), func(t *testing.T) { + restore := withHandler(t) + defer restore() + + dp := newExpoHistogramDataPoint[float64](4, 20, false, false) for _, v := range tt.values { dp.record(v) }