From fe9d1f7ec5265272db4cb6bdc959dbffbf22c56c Mon Sep 17 00:00:00 2001 From: Joshua MacDonald Date: Tue, 5 Jan 2021 23:17:20 -0800 Subject: [PATCH] Use uint64 Count consistently in metric aggregation (#1430) * Use uint64 Count consistently * Number --- CHANGELOG.md | 1 + exporters/otlp/internal/transform/metric.go | 12 ++++-------- exporters/otlp/internal/transform/metric_test.go | 4 ++-- sdk/export/metric/aggregation/aggregation.go | 14 ++++++-------- sdk/metric/aggregator/aggregatortest/test.go | 8 ++++---- sdk/metric/aggregator/array/array.go | 4 ++-- sdk/metric/aggregator/array/array_test.go | 4 ++-- sdk/metric/aggregator/ddsketch/ddsketch.go | 4 ++-- sdk/metric/aggregator/ddsketch/ddsketch_test.go | 2 +- sdk/metric/aggregator/histogram/histogram.go | 8 ++++---- sdk/metric/aggregator/histogram/histogram_test.go | 2 +- sdk/metric/aggregator/minmaxsumcount/mmsc.go | 4 ++-- sdk/metric/aggregator/minmaxsumcount/mmsc_test.go | 4 ++-- sdk/metric/correct_test.go | 2 +- sdk/metric/histogram_stress_test.go | 5 ++--- sdk/metric/minmaxsumcount_stress_test.go | 2 +- 16 files changed, 37 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93bde7f4f9c..1914e7ba0eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `NewExporter` from `exporters/otlp` now takes a `ProtocolDriver` as a parameter. (#1369) - Many OTLP Exporter options became gRPC ProtocolDriver options. (#1369) - Unify endpoint API that related to OTel exporter. (#1401) +- Metric aggregator Count() and histogram Bucket.Counts are consistently `uint64`. (1430) ### Removed diff --git a/exporters/otlp/internal/transform/metric.go b/exporters/otlp/internal/transform/metric.go index ad0432a6391..d2da8ab8854 100644 --- a/exporters/otlp/internal/transform/metric.go +++ b/exporters/otlp/internal/transform/metric.go @@ -458,7 +458,7 @@ func sumPoint(record export.Record, num number.Number, start, end time.Time, ek // minMaxSumCountValue returns the values of the MinMaxSumCount Aggregator // as discrete values. -func minMaxSumCountValues(a aggregation.MinMaxSumCount) (min, max, sum number.Number, count int64, err error) { +func minMaxSumCountValues(a aggregation.MinMaxSumCount) (min, max, sum number.Number, count uint64, err error) { if min, err = a.Min(); err != nil { return } @@ -531,7 +531,7 @@ func minMaxSumCount(record export.Record, a aggregation.MinMaxSumCount) (*metric return m, nil } -func histogramValues(a aggregation.Histogram) (boundaries []float64, counts []float64, err error) { +func histogramValues(a aggregation.Histogram) (boundaries []float64, counts []uint64, err error) { var buckets aggregation.Buckets if buckets, err = a.Histogram(); err != nil { return @@ -563,10 +563,6 @@ func histogramPoint(record export.Record, ek export.ExportKind, a aggregation.Hi return nil, err } - buckets := make([]uint64, len(counts)) - for i := 0; i < len(counts); i++ { - buckets[i] = uint64(counts[i]) - } m := &metricpb.Metric{ Name: desc.Name(), Description: desc.Description(), @@ -584,7 +580,7 @@ func histogramPoint(record export.Record, ek export.ExportKind, a aggregation.Hi StartTimeUnixNano: toNanos(record.StartTime()), TimeUnixNano: toNanos(record.EndTime()), Count: uint64(count), - BucketCounts: buckets, + BucketCounts: counts, ExplicitBounds: boundaries, }, }, @@ -601,7 +597,7 @@ func histogramPoint(record export.Record, ek export.ExportKind, a aggregation.Hi StartTimeUnixNano: toNanos(record.StartTime()), TimeUnixNano: toNanos(record.EndTime()), Count: uint64(count), - BucketCounts: buckets, + BucketCounts: counts, ExplicitBounds: boundaries, }, }, diff --git a/exporters/otlp/internal/transform/metric_test.go b/exporters/otlp/internal/transform/metric_test.go index f6fa64e58f3..6519a579c75 100644 --- a/exporters/otlp/internal/transform/metric_test.go +++ b/exporters/otlp/internal/transform/metric_test.go @@ -117,7 +117,7 @@ func TestMinMaxSumCountValue(t *testing.T) { assert.Equal(t, min, number.NewInt64Number(1)) assert.Equal(t, max, number.NewInt64Number(10)) assert.Equal(t, sum, number.NewInt64Number(11)) - assert.Equal(t, count, int64(2)) + assert.Equal(t, count, uint64(2)) } } @@ -369,7 +369,7 @@ func (te *testErrMinMaxSumCount) Max() (number.Number, error) { return 0, te.err } -func (te *testErrMinMaxSumCount) Count() (int64, error) { +func (te *testErrMinMaxSumCount) Count() (uint64, error) { return 0, te.err } diff --git a/sdk/export/metric/aggregation/aggregation.go b/sdk/export/metric/aggregation/aggregation.go index 7f76b0baeb9..0d17d0d3d21 100644 --- a/sdk/export/metric/aggregation/aggregation.go +++ b/sdk/export/metric/aggregation/aggregation.go @@ -43,7 +43,7 @@ type ( // Count returns the number of values that were aggregated. Count interface { Aggregation - Count() (int64, error) + Count() (uint64, error) } // Min returns the minimum value over the set of values that were aggregated. @@ -86,16 +86,14 @@ type ( // aggregating integers. Boundaries []float64 - // Counts are floating point numbers to account for - // the possibility of sampling which allows for - // non-integer count values. - Counts []float64 + // Counts holds the count in each bucket. + Counts []uint64 } // Histogram returns the count of events in pre-determined buckets. Histogram interface { Aggregation - Count() (int64, error) + Count() (uint64, error) Sum() (number.Number, error) Histogram() (Buckets, error) } @@ -106,7 +104,7 @@ type ( Min() (number.Number, error) Max() (number.Number, error) Sum() (number.Number, error) - Count() (int64, error) + Count() (uint64, error) } // Distribution supports the Min, Max, Sum, Count, and Quantile @@ -116,7 +114,7 @@ type ( Min() (number.Number, error) Max() (number.Number, error) Sum() (number.Number, error) - Count() (int64, error) + Count() (uint64, error) Quantile(float64) (number.Number, error) } ) diff --git a/sdk/metric/aggregator/aggregatortest/test.go b/sdk/metric/aggregator/aggregatortest/test.go index 3a965551256..4313868fdd6 100644 --- a/sdk/metric/aggregator/aggregatortest/test.go +++ b/sdk/metric/aggregator/aggregatortest/test.go @@ -134,8 +134,8 @@ func (n *Numbers) Sum() number.Number { return sum } -func (n *Numbers) Count() int64 { - return int64(len(n.numbers)) +func (n *Numbers) Count() uint64 { + return uint64(len(n.numbers)) } func (n *Numbers) Min() number.Number { @@ -223,7 +223,7 @@ func SynchronizedMoveResetTest(t *testing.T, mkind metric.InstrumentKind, nf fun if count, ok := agg.(aggregation.Count); ok { c, err := count.Count() - require.Equal(t, int64(0), c) + require.Equal(t, uint64(0), c) require.NoError(t, err) } @@ -270,7 +270,7 @@ func SynchronizedMoveResetTest(t *testing.T, mkind metric.InstrumentKind, nf fun if count, ok := agg.(aggregation.Count); ok { c, err := count.Count() - require.Equal(t, int64(1), c) + require.Equal(t, uint64(1), c) require.NoError(t, err) } diff --git a/sdk/metric/aggregator/array/array.go b/sdk/metric/aggregator/array/array.go index 1ac1d82a5f1..86305de953b 100644 --- a/sdk/metric/aggregator/array/array.go +++ b/sdk/metric/aggregator/array/array.go @@ -68,8 +68,8 @@ func (c *Aggregator) Sum() (number.Number, error) { } // Count returns the number of values in the checkpoint. -func (c *Aggregator) Count() (int64, error) { - return int64(len(c.points)), nil +func (c *Aggregator) Count() (uint64, error) { + return uint64(len(c.points)), nil } // Max returns the maximum value in the checkpoint. diff --git a/sdk/metric/aggregator/array/array_test.go b/sdk/metric/aggregator/array/array_test.go index e9ba40b7001..a4ecdb0c56f 100644 --- a/sdk/metric/aggregator/array/array_test.go +++ b/sdk/metric/aggregator/array/array_test.go @@ -42,7 +42,7 @@ func checkZero(t *testing.T, agg *Aggregator, desc *metric.Descriptor) { count, err := agg.Count() require.NoError(t, err) - require.Equal(t, int64(0), count) + require.Equal(t, uint64(0), count) max, err := agg.Max() require.True(t, errors.Is(err, aggregation.ErrNoData)) @@ -237,7 +237,7 @@ func TestArrayErrors(t *testing.T) { require.NoError(t, agg.SynchronizedMove(ckpt, descriptor)) count, err := ckpt.Count() - require.Equal(t, int64(1), count, "NaN value was not counted") + require.Equal(t, uint64(1), count, "NaN value was not counted") require.Nil(t, err) num, err := ckpt.Quantile(0) diff --git a/sdk/metric/aggregator/ddsketch/ddsketch.go b/sdk/metric/aggregator/ddsketch/ddsketch.go index cbd97a776cf..f72360effd2 100644 --- a/sdk/metric/aggregator/ddsketch/ddsketch.go +++ b/sdk/metric/aggregator/ddsketch/ddsketch.go @@ -79,8 +79,8 @@ func (c *Aggregator) Sum() (number.Number, error) { } // Count returns the number of values in the checkpoint. -func (c *Aggregator) Count() (int64, error) { - return c.sketch.Count(), nil +func (c *Aggregator) Count() (uint64, error) { + return uint64(c.sketch.Count()), nil } // Max returns the maximum value in the checkpoint. diff --git a/sdk/metric/aggregator/ddsketch/ddsketch_test.go b/sdk/metric/aggregator/ddsketch/ddsketch_test.go index d05ab7b1ad8..8fc274218b7 100644 --- a/sdk/metric/aggregator/ddsketch/ddsketch_test.go +++ b/sdk/metric/aggregator/ddsketch/ddsketch_test.go @@ -51,7 +51,7 @@ func checkZero(t *testing.T, agg *Aggregator, desc *metric.Descriptor) { count, err := agg.Count() require.NoError(t, err) - require.Equal(t, int64(0), count) + require.Equal(t, uint64(0), count) max, err := agg.Max() require.True(t, errors.Is(err, aggregation.ErrNoData)) diff --git a/sdk/metric/aggregator/histogram/histogram.go b/sdk/metric/aggregator/histogram/histogram.go index eb41fb012b6..c2a4c40032f 100644 --- a/sdk/metric/aggregator/histogram/histogram.go +++ b/sdk/metric/aggregator/histogram/histogram.go @@ -45,9 +45,9 @@ type ( // the sum and counts for all observed values and // the less than equal bucket count for the pre-determined boundaries. state struct { - bucketCounts []float64 + bucketCounts []uint64 sum number.Number - count int64 + count uint64 } ) @@ -100,7 +100,7 @@ func (c *Aggregator) Sum() (number.Number, error) { } // Count returns the number of values in the checkpoint. -func (c *Aggregator) Count() (int64, error) { +func (c *Aggregator) Count() (uint64, error) { return c.state.count, nil } @@ -135,7 +135,7 @@ func (c *Aggregator) SynchronizedMove(oa export.Aggregator, desc *metric.Descrip func emptyState(boundaries []float64) state { return state{ - bucketCounts: make([]float64, len(boundaries)+1), + bucketCounts: make([]uint64, len(boundaries)+1), } } diff --git a/sdk/metric/aggregator/histogram/histogram_test.go b/sdk/metric/aggregator/histogram/histogram_test.go index e1a096f46d1..ed0bf3fdd5f 100644 --- a/sdk/metric/aggregator/histogram/histogram_test.go +++ b/sdk/metric/aggregator/histogram/histogram_test.go @@ -78,7 +78,7 @@ func checkZero(t *testing.T, agg *histogram.Aggregator, desc *metric.Descriptor) require.NoError(t, err) count, err := agg.Count() - require.Equal(t, int64(0), count, "Empty checkpoint count = 0") + require.Equal(t, uint64(0), count, "Empty checkpoint count = 0") require.NoError(t, err) buckets, err := agg.Histogram() diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc.go b/sdk/metric/aggregator/minmaxsumcount/mmsc.go index 66a888a59d0..45ecb26edfb 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc.go @@ -38,7 +38,7 @@ type ( sum number.Number min number.Number max number.Number - count int64 + count uint64 } ) @@ -78,7 +78,7 @@ func (c *Aggregator) Sum() (number.Number, error) { } // Count returns the number of values in the checkpoint. -func (c *Aggregator) Count() (int64, error) { +func (c *Aggregator) Count() (uint64, error) { return c.count, nil } diff --git a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go index 0fb2be99146..085c7a546d2 100644 --- a/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go +++ b/sdk/metric/aggregator/minmaxsumcount/mmsc_test.go @@ -97,7 +97,7 @@ func checkZero(t *testing.T, agg *Aggregator, desc *metric.Descriptor) { count, err := agg.Count() require.NoError(t, err) - require.Equal(t, int64(0), count) + require.Equal(t, uint64(0), count) max, err := agg.Max() require.True(t, errors.Is(err, aggregation.ErrNoData)) @@ -228,7 +228,7 @@ func TestMaxSumCountNotSet(t *testing.T) { require.Nil(t, err) count, err := ckpt.Count() - require.Equal(t, int64(0), count, "Empty checkpoint count = 0") + require.Equal(t, uint64(0), count, "Empty checkpoint count = 0") require.Nil(t, err) max, err := ckpt.Max() diff --git a/sdk/metric/correct_test.go b/sdk/metric/correct_test.go index 5ccba31af82..151f3bdf6fc 100644 --- a/sdk/metric/correct_test.go +++ b/sdk/metric/correct_test.go @@ -168,7 +168,7 @@ func TestInputRangeValueRecorder(t *testing.T) { checkpointed = sdk.Collect(ctx) count, err := processor.accumulations[0].Aggregator().(aggregation.Distribution).Count() - require.Equal(t, int64(2), count) + require.Equal(t, uint64(2), count) require.Equal(t, 1, checkpointed) require.Nil(t, testHandler.Flush()) require.Nil(t, err) diff --git a/sdk/metric/histogram_stress_test.go b/sdk/metric/histogram_stress_test.go index a09c950cb18..10aa7e1a86f 100644 --- a/sdk/metric/histogram_stress_test.go +++ b/sdk/metric/histogram_stress_test.go @@ -54,10 +54,9 @@ func TestStressInt64Histogram(t *testing.T) { b, _ := ckpt.Histogram() c, _ := ckpt.Count() - var realCount int64 + var realCount uint64 for _, c := range b.Counts { - v := int64(c) - realCount += v + realCount += c } if realCount != c { diff --git a/sdk/metric/minmaxsumcount_stress_test.go b/sdk/metric/minmaxsumcount_stress_test.go index 4aafb18d160..ecac5bf7293 100644 --- a/sdk/metric/minmaxsumcount_stress_test.go +++ b/sdk/metric/minmaxsumcount_stress_test.go @@ -63,7 +63,7 @@ func TestStressInt64MinMaxSumCount(t *testing.T) { } lo, hi, sum := min.AsInt64(), max.AsInt64(), s.AsInt64() - if hi-lo+1 != c { + if uint64(hi-lo)+1 != c { t.Fail() } if c == 1 {