From 16823a9843da9a02b784ac83ab662b0ee0499b5f Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 16:55:03 -0700 Subject: [PATCH 1/8] Return empty nil aggs if no meas --- sdk/metric/internal/histogram.go | 12 +++++------ sdk/metric/internal/lastvalue.go | 6 +++--- sdk/metric/internal/sum.go | 36 ++++++++++++++++---------------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/sdk/metric/internal/histogram.go b/sdk/metric/internal/histogram.go index b04b71baf12..4e80b0b7efc 100644 --- a/sdk/metric/internal/histogram.go +++ b/sdk/metric/internal/histogram.go @@ -130,15 +130,15 @@ type deltaHistogram[N int64 | float64] struct { } func (s *deltaHistogram[N]) Aggregation() metricdata.Aggregation { - h := metricdata.Histogram{Temporality: metricdata.DeltaTemporality} - s.valuesMu.Lock() defer s.valuesMu.Unlock() if len(s.values) == 0 { - return h + return nil } + h := metricdata.Histogram{Temporality: metricdata.DeltaTemporality} + // Do not allow modification of our copy of bounds. bounds := make([]float64, len(s.bounds)) copy(bounds, s.bounds) @@ -192,15 +192,15 @@ type cumulativeHistogram[N int64 | float64] struct { } func (s *cumulativeHistogram[N]) Aggregation() metricdata.Aggregation { - h := metricdata.Histogram{Temporality: metricdata.CumulativeTemporality} - s.valuesMu.Lock() defer s.valuesMu.Unlock() if len(s.values) == 0 { - return h + return nil } + h := metricdata.Histogram{Temporality: metricdata.CumulativeTemporality} + // Do not allow modification of our copy of bounds. bounds := make([]float64, len(s.bounds)) copy(bounds, s.bounds) diff --git a/sdk/metric/internal/lastvalue.go b/sdk/metric/internal/lastvalue.go index d5ee3b009ba..7b1c54768e5 100644 --- a/sdk/metric/internal/lastvalue.go +++ b/sdk/metric/internal/lastvalue.go @@ -49,15 +49,15 @@ func (s *lastValue[N]) Aggregate(value N, attr attribute.Set) { } func (s *lastValue[N]) Aggregation() metricdata.Aggregation { - gauge := metricdata.Gauge[N]{} - s.Lock() defer s.Unlock() if len(s.values) == 0 { - return gauge + return nil } + gauge := metricdata.Gauge[N]{} + gauge.DataPoints = make([]metricdata.DataPoint[N], 0, len(s.values)) for a, v := range s.values { gauge.DataPoints = append(gauge.DataPoints, metricdata.DataPoint[N]{ diff --git a/sdk/metric/internal/sum.go b/sdk/metric/internal/sum.go index 210d2370120..35f45d18fe0 100644 --- a/sdk/metric/internal/sum.go +++ b/sdk/metric/internal/sum.go @@ -76,16 +76,16 @@ type deltaSum[N int64 | float64] struct { } func (s *deltaSum[N]) Aggregation() metricdata.Aggregation { - out := metricdata.Sum[N]{ - Temporality: metricdata.DeltaTemporality, - IsMonotonic: s.monotonic, - } - s.Lock() defer s.Unlock() if len(s.values) == 0 { - return out + return nil + } + + out := metricdata.Sum[N]{ + Temporality: metricdata.DeltaTemporality, + IsMonotonic: s.monotonic, } t := now() @@ -137,16 +137,16 @@ type cumulativeSum[N int64 | float64] struct { } func (s *cumulativeSum[N]) Aggregation() metricdata.Aggregation { - out := metricdata.Sum[N]{ - Temporality: metricdata.CumulativeTemporality, - IsMonotonic: s.monotonic, - } - s.Lock() defer s.Unlock() if len(s.values) == 0 { - return out + return nil + } + + out := metricdata.Sum[N]{ + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: s.monotonic, } t := now() @@ -204,16 +204,16 @@ func (s *precomputedDeltaSum[N]) Aggregate(value N, attr attribute.Set) { } func (s *precomputedDeltaSum[N]) Aggregation() metricdata.Aggregation { - out := metricdata.Sum[N]{ - Temporality: metricdata.DeltaTemporality, - IsMonotonic: s.monotonic, - } - s.Lock() defer s.Unlock() if len(s.recorded) == 0 { - return out + return nil + } + + out := metricdata.Sum[N]{ + Temporality: metricdata.DeltaTemporality, + IsMonotonic: s.monotonic, } t := now() From ec3b2dd4308d9b325a3581e5c61911e00f9faaa8 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 17:04:25 -0700 Subject: [PATCH 2/8] Update tests with new expected behavior --- sdk/metric/internal/histogram_test.go | 13 ++++++++++--- sdk/metric/internal/lastvalue_test.go | 23 +++++++++++++++-------- sdk/metric/internal/sum_test.go | 26 +++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/sdk/metric/internal/histogram_test.go b/sdk/metric/internal/histogram_test.go index 5b18d8eee4a..fb2968fc325 100644 --- a/sdk/metric/internal/histogram_test.go +++ b/sdk/metric/internal/histogram_test.go @@ -169,17 +169,17 @@ func TestCumulativeHistogramImutableCounts(t *testing.T) { func TestDeltaHistogramReset(t *testing.T) { t.Cleanup(mockTime(now)) - expect := metricdata.Histogram{Temporality: metricdata.DeltaTemporality} a := NewDeltaHistogram[int64](histConf) - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) a.Aggregate(1, alice) + expect := metricdata.Histogram{Temporality: metricdata.DeltaTemporality} expect.DataPoints = []metricdata.HistogramDataPoint{hPoint(alice, 1, 1)} metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) // The attr set should be forgotten once Aggregations is called. expect.DataPoints = nil - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) // Aggregating another set should not affect the original (alice). a.Aggregate(1, bob) @@ -187,6 +187,13 @@ func TestDeltaHistogramReset(t *testing.T) { metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) } +func TestEmptyHistogramNilAggregation(t *testing.T) { + assert.Nil(t, NewCumulativeHistogram[int64](histConf).Aggregation()) + assert.Nil(t, NewCumulativeHistogram[float64](histConf).Aggregation()) + assert.Nil(t, NewDeltaHistogram[int64](histConf).Aggregation()) + assert.Nil(t, NewDeltaHistogram[float64](histConf).Aggregation()) +} + func BenchmarkHistogram(b *testing.B) { b.Run("Int64", benchmarkHistogram[int64]) b.Run("Float64", benchmarkHistogram[float64]) diff --git a/sdk/metric/internal/lastvalue_test.go b/sdk/metric/internal/lastvalue_test.go index 3f84af14304..28309ea7de0 100644 --- a/sdk/metric/internal/lastvalue_test.go +++ b/sdk/metric/internal/lastvalue_test.go @@ -17,6 +17,7 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" import ( "testing" + "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" ) @@ -52,20 +53,21 @@ func testLastValueReset[N int64 | float64](t *testing.T) { t.Cleanup(mockTime(now)) a := NewLastValue[N]() - expect := metricdata.Gauge[N]{} - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) a.Aggregate(1, alice) - expect.DataPoints = []metricdata.DataPoint[N]{{ - Attributes: alice, - Time: now(), - Value: 1, - }} + expect := metricdata.Gauge[N]{ + DataPoints: []metricdata.DataPoint[N]{{ + Attributes: alice, + Time: now(), + Value: 1, + }}, + } metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) // The attr set should be forgotten once Aggregations is called. expect.DataPoints = nil - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) // Aggregating another set should not affect the original (alice). a.Aggregate(1, bob) @@ -82,6 +84,11 @@ func TestLastValueReset(t *testing.T) { t.Run("Float64", testLastValueReset[float64]) } +func TestEmptyLastValueNilAggregation(t *testing.T) { + assert.Nil(t, NewLastValue[int64]().Aggregation()) + assert.Nil(t, NewLastValue[float64]().Aggregation()) +} + func BenchmarkLastValue(b *testing.B) { b.Run("Int64", benchmarkAggregator(NewLastValue[int64])) b.Run("Float64", benchmarkAggregator(NewLastValue[float64])) diff --git a/sdk/metric/internal/sum_test.go b/sdk/metric/internal/sum_test.go index 92e3b394675..3b359a84651 100644 --- a/sdk/metric/internal/sum_test.go +++ b/sdk/metric/internal/sum_test.go @@ -17,6 +17,7 @@ package internal // import "go.opentelemetry.io/otel/sdk/metric/internal" import ( "testing" + "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" @@ -138,17 +139,17 @@ func point[N int64 | float64](a attribute.Set, v N) metricdata.DataPoint[N] { func testDeltaSumReset[N int64 | float64](t *testing.T) { t.Cleanup(mockTime(now)) - expect := metricdata.Sum[N]{Temporality: metricdata.DeltaTemporality} a := NewDeltaSum[N](false) - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) a.Aggregate(1, alice) + expect := metricdata.Sum[N]{Temporality: metricdata.DeltaTemporality} expect.DataPoints = []metricdata.DataPoint[N]{point[N](alice, 1)} metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) // The attr set should be forgotten once Aggregations is called. expect.DataPoints = nil - metricdatatest.AssertAggregationsEqual(t, expect, a.Aggregation()) + assert.Nil(t, a.Aggregation()) // Aggregating another set should not affect the original (alice). a.Aggregate(1, bob) @@ -161,6 +162,25 @@ func TestDeltaSumReset(t *testing.T) { t.Run("Float64", testDeltaSumReset[float64]) } +func TestEmptySumNilAggregation(t *testing.T) { + assert.Nil(t, NewCumulativeSum[int64](true).Aggregation()) + assert.Nil(t, NewCumulativeSum[int64](false).Aggregation()) + assert.Nil(t, NewCumulativeSum[float64](true).Aggregation()) + assert.Nil(t, NewCumulativeSum[float64](false).Aggregation()) + assert.Nil(t, NewDeltaSum[int64](true).Aggregation()) + assert.Nil(t, NewDeltaSum[int64](false).Aggregation()) + assert.Nil(t, NewDeltaSum[float64](true).Aggregation()) + assert.Nil(t, NewDeltaSum[float64](false).Aggregation()) + assert.Nil(t, NewPrecomputedCumulativeSum[int64](true).Aggregation()) + assert.Nil(t, NewPrecomputedCumulativeSum[int64](false).Aggregation()) + assert.Nil(t, NewPrecomputedCumulativeSum[float64](true).Aggregation()) + assert.Nil(t, NewPrecomputedCumulativeSum[float64](false).Aggregation()) + assert.Nil(t, NewPrecomputedDeltaSum[int64](true).Aggregation()) + assert.Nil(t, NewPrecomputedDeltaSum[int64](false).Aggregation()) + assert.Nil(t, NewPrecomputedDeltaSum[float64](true).Aggregation()) + assert.Nil(t, NewPrecomputedDeltaSum[float64](false).Aggregation()) +} + func BenchmarkSum(b *testing.B) { b.Run("Int64", benchmarkSum[int64]) b.Run("Float64", benchmarkSum[float64]) From f31703085ce1e385a3e37043759518ce87330f79 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 17:07:13 -0700 Subject: [PATCH 3/8] Add change to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69e781dc659..ec78f8c5d53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Cumulative metrics from the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) are defined as monotonic sums, instead of non-monotonic. (#3389) - Asynchronous counters (`Counter` and `UpDownCounter`) from the metric SDK now produce delta sums when configured with delta temporality. (#3398) - Exported `Status` codes in the `go.opentelemetry.io/otel/exporters/zipkin` exporter are now exported as all upper case values. (#3340) +- `Aggregation`s with no data are not exported. (#3394, #TBD) ## [1.11.1/0.33.0] 2022-10-19 From c939c357141d424269db32e8f38e775af7c73715 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 17:10:01 -0700 Subject: [PATCH 4/8] Set PR number in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec78f8c5d53..23826b865e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Cumulative metrics from the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) are defined as monotonic sums, instead of non-monotonic. (#3389) - Asynchronous counters (`Counter` and `UpDownCounter`) from the metric SDK now produce delta sums when configured with delta temporality. (#3398) - Exported `Status` codes in the `go.opentelemetry.io/otel/exporters/zipkin` exporter are now exported as all upper case values. (#3340) -- `Aggregation`s with no data are not exported. (#3394, #TBD) +- `Aggregation`s with no data are not exported. (#3394, #3436) ## [1.11.1/0.33.0] 2022-10-19 From 7a05da29fc7ea11549715a95f9d30b9be01a4693 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 17:11:24 -0700 Subject: [PATCH 5/8] Run lint --- sdk/metric/internal/lastvalue_test.go | 1 + sdk/metric/internal/sum_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/sdk/metric/internal/lastvalue_test.go b/sdk/metric/internal/lastvalue_test.go index 28309ea7de0..4d78373c328 100644 --- a/sdk/metric/internal/lastvalue_test.go +++ b/sdk/metric/internal/lastvalue_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" ) diff --git a/sdk/metric/internal/sum_test.go b/sdk/metric/internal/sum_test.go index 3b359a84651..359b73aa50a 100644 --- a/sdk/metric/internal/sum_test.go +++ b/sdk/metric/internal/sum_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest" From c494d34dfbf4a1fad40cb6c4e9d9c45a78dfbe6d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Nov 2022 17:15:11 -0700 Subject: [PATCH 6/8] Fix pipeline_test --- sdk/metric/pipeline_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sdk/metric/pipeline_test.go b/sdk/metric/pipeline_test.go index e47bb6b5f64..2cb2bd12e2d 100644 --- a/sdk/metric/pipeline_test.go +++ b/sdk/metric/pipeline_test.go @@ -168,6 +168,9 @@ func testDefaultViewImplicit[N int64 | float64]() func(t *testing.T) { got, err := i.Instrument(inst, unit.Dimensionless) require.NoError(t, err) assert.Len(t, got, 1, "default view not applied") + for _, a := range got { + a.Aggregate(1, *attribute.EmptySet()) + } out, err := test.pipe.produce(context.Background()) require.NoError(t, err) @@ -181,6 +184,7 @@ func testDefaultViewImplicit[N int64 | float64]() func(t *testing.T) { Data: metricdata.Sum[N]{ Temporality: metricdata.CumulativeTemporality, IsMonotonic: true, + DataPoints: []metricdata.DataPoint[N]{{Value: N(1)}}, }, }, sm.Metrics[0], metricdatatest.IgnoreTimestamp()) }) From ecfdcb32d38347ced700c8140b7e8f9e4ac23971 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Nov 2022 07:56:45 -0700 Subject: [PATCH 7/8] Scope change in changelog to pkg --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23826b865e4..230293eeb08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Cumulative metrics from the OpenCensus bridge (`go.opentelemetry.io/otel/bridge/opencensus`) are defined as monotonic sums, instead of non-monotonic. (#3389) - Asynchronous counters (`Counter` and `UpDownCounter`) from the metric SDK now produce delta sums when configured with delta temporality. (#3398) - Exported `Status` codes in the `go.opentelemetry.io/otel/exporters/zipkin` exporter are now exported as all upper case values. (#3340) -- `Aggregation`s with no data are not exported. (#3394, #3436) +- `Aggregation`s from `go.opentelemetry.io/otel/sdk/metric` with no data are not exported. (#3394, #3436) ## [1.11.1/0.33.0] 2022-10-19 From fc6be6070f353b8a7f919613ead343ff2bbea736 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Nov 2022 08:01:47 -0700 Subject: [PATCH 8/8] Clean up init of agg types --- sdk/metric/internal/histogram.go | 18 ++++++++++-------- sdk/metric/internal/lastvalue.go | 6 +++--- sdk/metric/internal/sum.go | 15 ++++++--------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/sdk/metric/internal/histogram.go b/sdk/metric/internal/histogram.go index 4e80b0b7efc..058af8419b1 100644 --- a/sdk/metric/internal/histogram.go +++ b/sdk/metric/internal/histogram.go @@ -137,13 +137,14 @@ func (s *deltaHistogram[N]) Aggregation() metricdata.Aggregation { return nil } - h := metricdata.Histogram{Temporality: metricdata.DeltaTemporality} - + t := now() // Do not allow modification of our copy of bounds. bounds := make([]float64, len(s.bounds)) copy(bounds, s.bounds) - t := now() - h.DataPoints = make([]metricdata.HistogramDataPoint, 0, len(s.values)) + h := metricdata.Histogram{ + Temporality: metricdata.DeltaTemporality, + DataPoints: make([]metricdata.HistogramDataPoint, 0, len(s.values)), + } for a, b := range s.values { hdp := metricdata.HistogramDataPoint{ Attributes: a, @@ -199,13 +200,14 @@ func (s *cumulativeHistogram[N]) Aggregation() metricdata.Aggregation { return nil } - h := metricdata.Histogram{Temporality: metricdata.CumulativeTemporality} - + t := now() // Do not allow modification of our copy of bounds. bounds := make([]float64, len(s.bounds)) copy(bounds, s.bounds) - t := now() - h.DataPoints = make([]metricdata.HistogramDataPoint, 0, len(s.values)) + h := metricdata.Histogram{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: make([]metricdata.HistogramDataPoint, 0, len(s.values)), + } for a, b := range s.values { // The HistogramDataPoint field values returned need to be copies of // the buckets value as we will keep updating them. diff --git a/sdk/metric/internal/lastvalue.go b/sdk/metric/internal/lastvalue.go index 7b1c54768e5..16ee77362c4 100644 --- a/sdk/metric/internal/lastvalue.go +++ b/sdk/metric/internal/lastvalue.go @@ -56,9 +56,9 @@ func (s *lastValue[N]) Aggregation() metricdata.Aggregation { return nil } - gauge := metricdata.Gauge[N]{} - - gauge.DataPoints = make([]metricdata.DataPoint[N], 0, len(s.values)) + gauge := metricdata.Gauge[N]{ + DataPoints: make([]metricdata.DataPoint[N], 0, len(s.values)), + } for a, v := range s.values { gauge.DataPoints = append(gauge.DataPoints, metricdata.DataPoint[N]{ Attributes: a, diff --git a/sdk/metric/internal/sum.go b/sdk/metric/internal/sum.go index 35f45d18fe0..ecf22f44b6d 100644 --- a/sdk/metric/internal/sum.go +++ b/sdk/metric/internal/sum.go @@ -83,13 +83,12 @@ func (s *deltaSum[N]) Aggregation() metricdata.Aggregation { return nil } + t := now() out := metricdata.Sum[N]{ Temporality: metricdata.DeltaTemporality, IsMonotonic: s.monotonic, + DataPoints: make([]metricdata.DataPoint[N], 0, len(s.values)), } - - t := now() - out.DataPoints = make([]metricdata.DataPoint[N], 0, len(s.values)) for attr, value := range s.values { out.DataPoints = append(out.DataPoints, metricdata.DataPoint[N]{ Attributes: attr, @@ -144,13 +143,12 @@ func (s *cumulativeSum[N]) Aggregation() metricdata.Aggregation { return nil } + t := now() out := metricdata.Sum[N]{ Temporality: metricdata.CumulativeTemporality, IsMonotonic: s.monotonic, + DataPoints: make([]metricdata.DataPoint[N], 0, len(s.values)), } - - t := now() - out.DataPoints = make([]metricdata.DataPoint[N], 0, len(s.values)) for attr, value := range s.values { out.DataPoints = append(out.DataPoints, metricdata.DataPoint[N]{ Attributes: attr, @@ -211,13 +209,12 @@ func (s *precomputedDeltaSum[N]) Aggregation() metricdata.Aggregation { return nil } + t := now() out := metricdata.Sum[N]{ Temporality: metricdata.DeltaTemporality, IsMonotonic: s.monotonic, + DataPoints: make([]metricdata.DataPoint[N], 0, len(s.recorded)), } - - t := now() - out.DataPoints = make([]metricdata.DataPoint[N], 0, len(s.recorded)) for attr, recorded := range s.recorded { value := recorded - s.reported[attr] out.DataPoints = append(out.DataPoints, metricdata.DataPoint[N]{