From 30e726ca146d82a66923eb1fef6d65c94b6752b3 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 19 Dec 2022 12:19:28 -0800 Subject: [PATCH 1/9] Use Extrema type for Histogram min/max --- .../internal/transform/metricdata.go | 15 ++++++++++---- .../internal/transform/metricdata_test.go | 8 ++++---- sdk/metric/internal/histogram.go | 10 ++++------ sdk/metric/internal/histogram_test.go | 4 ++-- sdk/metric/meter_test.go | 20 +++++++++---------- sdk/metric/metricdata/data.go | 14 +++++++++++-- .../metricdata/metricdatatest/assertion.go | 3 +++ .../metricdatatest/assertion_test.go | 5 ++--- .../metricdata/metricdatatest/comparisons.go | 18 +++++++++++------ 9 files changed, 59 insertions(+), 38 deletions(-) diff --git a/exporters/otlp/otlpmetric/internal/transform/metricdata.go b/exporters/otlp/otlpmetric/internal/transform/metricdata.go index 4d6edc90330..152ea554281 100644 --- a/exporters/otlp/otlpmetric/internal/transform/metricdata.go +++ b/exporters/otlp/otlpmetric/internal/transform/metricdata.go @@ -174,7 +174,7 @@ func HistogramDataPoints(dPts []metricdata.HistogramDataPoint) []*mpb.HistogramD out := make([]*mpb.HistogramDataPoint, 0, len(dPts)) for _, dPt := range dPts { sum := dPt.Sum - out = append(out, &mpb.HistogramDataPoint{ + hdp := &mpb.HistogramDataPoint{ Attributes: AttrIter(dPt.Attributes.Iter()), StartTimeUnixNano: uint64(dPt.StartTime.UnixNano()), TimeUnixNano: uint64(dPt.Time.UnixNano()), @@ -182,9 +182,16 @@ func HistogramDataPoints(dPts []metricdata.HistogramDataPoint) []*mpb.HistogramD Sum: &sum, BucketCounts: dPt.BucketCounts, ExplicitBounds: dPt.Bounds, - Min: dPt.Min, - Max: dPt.Max, - }) + } + if dPt.Min.Valid { + min := dPt.Min.Value + hdp.Min = &min + } + if dPt.Max.Valid { + max := dPt.Max.Value + hdp.Max = &max + } + out = append(out, hdp) } return out } diff --git a/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go b/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go index db102b4f2aa..64c7c3d8716 100644 --- a/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go +++ b/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go @@ -60,8 +60,8 @@ var ( Count: 30, Bounds: []float64{1, 5}, BucketCounts: []uint64{0, 30, 0}, - Min: &minA, - Max: &maxA, + Min: metricdata.Extrema{Value: minA, Valid: true}, + Max: metricdata.Extrema{Value: maxA, Valid: true}, Sum: sumA, }, { Attributes: bob, @@ -70,8 +70,8 @@ var ( Count: 3, Bounds: []float64{1, 5}, BucketCounts: []uint64{0, 1, 2}, - Min: &minB, - Max: &maxB, + Min: metricdata.Extrema{Value: minB, Valid: true}, + Max: metricdata.Extrema{Value: maxB, Valid: true}, Sum: sumB, }} diff --git a/sdk/metric/internal/histogram.go b/sdk/metric/internal/histogram.go index 058af8419b1..61b907be44c 100644 --- a/sdk/metric/internal/histogram.go +++ b/sdk/metric/internal/histogram.go @@ -156,8 +156,8 @@ func (s *deltaHistogram[N]) Aggregation() metricdata.Aggregation { Sum: b.sum, } if !s.noMinMax { - hdp.Min = &b.min - hdp.Max = &b.max + hdp.Min = metricdata.Extrema{Value: b.min, Valid: true} + hdp.Max = metricdata.Extrema{Value: b.max, Valid: true} } h.DataPoints = append(h.DataPoints, hdp) @@ -227,10 +227,8 @@ func (s *cumulativeHistogram[N]) Aggregation() metricdata.Aggregation { Sum: b.sum, } if !s.noMinMax { - // Similar to counts, make a copy. - min, max := b.min, b.max - hdp.Min = &min - hdp.Max = &max + hdp.Min = metricdata.Extrema{Value: b.min, Valid: true} + hdp.Max = metricdata.Extrema{Value: b.max, Valid: true} } h.DataPoints = append(h.DataPoints, hdp) // TODO (#3006): This will use an unbounded amount of memory if there diff --git a/sdk/metric/internal/histogram_test.go b/sdk/metric/internal/histogram_test.go index fb2968fc325..6bf2a78ac9a 100644 --- a/sdk/metric/internal/histogram_test.go +++ b/sdk/metric/internal/histogram_test.go @@ -92,8 +92,8 @@ func hPoint(a attribute.Set, v float64, multi uint64) metricdata.HistogramDataPo Count: multi, Bounds: bounds, BucketCounts: counts, - Min: &v, - Max: &v, + Min: metricdata.Extrema{Value: v, Valid: true}, + Max: metricdata.Extrema{Value: v, Valid: true}, Sum: v * float64(multi), } } diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index d904b118ad4..a5cd85c3b06 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -167,7 +167,7 @@ func TestCallbackUnregisterConcurrency(t *testing.T) { // Instruments should produce correct ResourceMetrics. func TestMeterCreatesInstruments(t *testing.T) { - seven := 7.0 + extrema := metricdata.Extrema{Value: 7., Valid: true} testCases := []struct { name string fn func(*testing.T, metric.Meter) @@ -370,8 +370,8 @@ func TestMeterCreatesInstruments(t *testing.T) { Count: 1, Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, - Min: &seven, - Max: &seven, + Min: extrema, + Max: extrema, Sum: 7.0, }, }, @@ -434,8 +434,8 @@ func TestMeterCreatesInstruments(t *testing.T) { Count: 1, Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, - Min: &seven, - Max: &seven, + Min: extrema, + Max: extrema, Sum: 7.0, }, }, @@ -624,8 +624,6 @@ func TestRegisterCallbackDropAggregations(t *testing.T) { } func TestAttributeFilter(t *testing.T) { - one := 1.0 - two := 2.0 testcases := []struct { name string register func(t *testing.T, mtr metric.Meter) error @@ -862,8 +860,8 @@ func TestAttributeFilter(t *testing.T) { Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, Count: 2, - Min: &one, - Max: &two, + Min: metricdata.Extrema{Value: 1., Valid: true}, + Max: metricdata.Extrema{Value: 2., Valid: true}, Sum: 3.0, }, }, @@ -944,8 +942,8 @@ func TestAttributeFilter(t *testing.T) { Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, Count: 2, - Min: &one, - Max: &two, + Min: metricdata.Extrema{Value: 1., Valid: true}, + Max: metricdata.Extrema{Value: 2., Valid: true}, Sum: 3.0, }, }, diff --git a/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index a729879e673..da6f69dcf34 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -122,9 +122,19 @@ type HistogramDataPoint struct { BucketCounts []uint64 // Min is the minimum value recorded. (optional) - Min *float64 `json:",omitempty"` + Min Extrema // Max is the maximum value recorded. (optional) - Max *float64 `json:",omitempty"` + Max Extrema // Sum is the sum of the values recorded. Sum float64 } + +// Extrema are the minimum or maximum values of a data set. +type Extrema struct { + // Value is the extrema value. + Value float64 + + // Valid is true if Value has been assigned a value. It is false if Value + // is the zero-value. + Valid bool +} diff --git a/sdk/metric/metricdata/metricdatatest/assertion.go b/sdk/metric/metricdata/metricdatatest/assertion.go index 193be1ff708..52d2ecb6d17 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion.go +++ b/sdk/metric/metricdata/metricdatatest/assertion.go @@ -32,6 +32,7 @@ type Datatypes interface { metricdata.Gauge[int64] | metricdata.Histogram | metricdata.HistogramDataPoint | + metricdata.Extrema | metricdata.Metrics | metricdata.ResourceMetrics | metricdata.ScopeMetrics | @@ -92,6 +93,8 @@ func AssertEqual[T Datatypes](t *testing.T, expected, actual T, opts ...Option) r = equalHistograms(e, aIface.(metricdata.Histogram), cfg) case metricdata.HistogramDataPoint: r = equalHistogramDataPoints(e, aIface.(metricdata.HistogramDataPoint), cfg) + case metricdata.Extrema: + r = equalExtrema(e, aIface.(metricdata.Extrema), cfg) case metricdata.Metrics: r = equalMetrics(e, aIface.(metricdata.Metrics), cfg) case metricdata.ResourceMetrics: diff --git a/sdk/metric/metricdata/metricdatatest/assertion_test.go b/sdk/metric/metricdata/metricdatatest/assertion_test.go index b7da4344353..a7b0b2a2891 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion_test.go +++ b/sdk/metric/metricdata/metricdatatest/assertion_test.go @@ -73,7 +73,6 @@ var ( Value: -1.0, } - max, min = 99.0, 3. histogramDataPointA = metricdata.HistogramDataPoint{ Attributes: attrA, StartTime: startA, @@ -90,8 +89,8 @@ var ( Count: 3, Bounds: []float64{0, 10, 100}, BucketCounts: []uint64{1, 1, 1}, - Max: &max, - Min: &min, + Max: metricdata.Extrema{Valid: true, Value: 99.}, + Min: metricdata.Extrema{Valid: true, Value: 3.}, Sum: 3, } histogramDataPointC = metricdata.HistogramDataPoint{ diff --git a/sdk/metric/metricdata/metricdatatest/comparisons.go b/sdk/metric/metricdata/metricdatatest/comparisons.go index 9879c96f109..2827962678d 100644 --- a/sdk/metric/metricdata/metricdatatest/comparisons.go +++ b/sdk/metric/metricdata/metricdatatest/comparisons.go @@ -267,10 +267,10 @@ func equalHistogramDataPoints(a, b metricdata.HistogramDataPoint, cfg config) (r if !equalSlices(a.BucketCounts, b.BucketCounts) { reasons = append(reasons, notEqualStr("BucketCounts", a.BucketCounts, b.BucketCounts)) } - if !equalPtrValues(a.Min, b.Min) { + if !eqExtrema(a.Min, b.Min) { reasons = append(reasons, notEqualStr("Min", a.Min, b.Min)) } - if !equalPtrValues(a.Max, b.Max) { + if !eqExtrema(a.Max, b.Max) { reasons = append(reasons, notEqualStr("Max", a.Max, b.Max)) } if a.Sum != b.Sum { @@ -295,12 +295,18 @@ func equalSlices[T comparable](a, b []T) bool { return true } -func equalPtrValues[T comparable](a, b *T) bool { - if a == nil || b == nil { - return a == b +func equalExtrema(a, b metricdata.Extrema, _ config) (reasons []string) { + if !eqExtrema(a, b) { + reasons = append(reasons, notEqualStr("Extrema", a, b)) } + return reasons +} - return *a == *b +func eqExtrema(a, b metricdata.Extrema) bool { + if !a.Valid || !b.Valid { + return a.Valid == b.Valid + } + return a.Value == b.Value } func diffSlices[T any](a, b []T, equal func(T, T) bool) (extraA, extraB []T) { From 8817b9d3930cd6edb0c9e0b458c0cb611292fbee Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 19 Dec 2022 12:50:24 -0800 Subject: [PATCH 2/9] Add case for Extrema in AssertHasAttributes --- sdk/metric/metricdata/metricdatatest/assertion.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/metric/metricdata/metricdatatest/assertion.go b/sdk/metric/metricdata/metricdatatest/assertion.go index 52d2ecb6d17..d1ad1e70195 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion.go +++ b/sdk/metric/metricdata/metricdatatest/assertion.go @@ -155,6 +155,8 @@ func AssertHasAttributes[T Datatypes](t *testing.T, actual T, attrs ...attribute reasons = hasAttributesSum(e, attrs...) case metricdata.HistogramDataPoint: reasons = hasAttributesHistogramDataPoints(e, attrs...) + case metricdata.Extrema: + // Nothing to check. case metricdata.Histogram: reasons = hasAttributesHistogram(e, attrs...) case metricdata.Metrics: From a36cdd9e68c82bcf3b3601d3da7907f7180b186a Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 19 Dec 2022 12:54:21 -0800 Subject: [PATCH 3/9] Add changes to changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f310bc2655..4813f012c34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Return a `Registration` from the `RegisterCallback` method of a `Meter` in the `go.opentelemetry.io/otel/metric` package. This `Registration` can be used to unregister callbacks. (#3522) - Add `Producer` interface and `Reader.RegisterProducer(Producer)` to `go.opentelemetry.io/otel/sdk/metric` to enable external metric Producers. (#3524) +- The `Extrema` type to `go.opentelemetry.io/otel/sdk/metric/metricdata` to represent min/max values and still be able to distinguish unset and zero values. (#3487) ### Removed @@ -24,6 +25,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add `Producer` interface and `Reader.RegisterProducer(Producer)` to `go.opentelemetry.io/otel/sdk/metric` to enable external metric Producers. (#3524) - Add `NewMetricProducer` to `go.opentelemetry.io/otel/bridge/opencensus`, which can be used to pass OpenCensus metrics to an OpenTelemetry Reader. (#3541) - Global logger uses an atomic value instead of a mutex. (#3545) +- The `Min` and `Max` fields of the `HistogramDataPoint` in `go.opentelemetry.io/otel/sdk/metric/metricdata` are now defined with the added `Extrema` type instead of a `*float64`. (#3487) ### Deprecated From 7a19a912168a5e7bcf3d0c752dffa48d16b4aedf Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 20 Dec 2022 08:09:23 -0800 Subject: [PATCH 4/9] Add NewExtrema --- .../otlpmetric/internal/transform/metricdata_test.go | 8 ++++---- sdk/metric/internal/histogram.go | 8 ++++---- sdk/metric/internal/histogram_test.go | 4 ++-- sdk/metric/meter_test.go | 10 +++++----- sdk/metric/metricdata/data.go | 5 +++++ sdk/metric/metricdata/metricdatatest/assertion_test.go | 4 ++-- 6 files changed, 22 insertions(+), 17 deletions(-) diff --git a/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go b/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go index 64c7c3d8716..f9c3c58646f 100644 --- a/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go +++ b/exporters/otlp/otlpmetric/internal/transform/metricdata_test.go @@ -60,8 +60,8 @@ var ( Count: 30, Bounds: []float64{1, 5}, BucketCounts: []uint64{0, 30, 0}, - Min: metricdata.Extrema{Value: minA, Valid: true}, - Max: metricdata.Extrema{Value: maxA, Valid: true}, + Min: metricdata.NewExtrema(minA), + Max: metricdata.NewExtrema(maxA), Sum: sumA, }, { Attributes: bob, @@ -70,8 +70,8 @@ var ( Count: 3, Bounds: []float64{1, 5}, BucketCounts: []uint64{0, 1, 2}, - Min: metricdata.Extrema{Value: minB, Valid: true}, - Max: metricdata.Extrema{Value: maxB, Valid: true}, + Min: metricdata.NewExtrema(minB), + Max: metricdata.NewExtrema(maxB), Sum: sumB, }} diff --git a/sdk/metric/internal/histogram.go b/sdk/metric/internal/histogram.go index 61b907be44c..fb4f4d47db1 100644 --- a/sdk/metric/internal/histogram.go +++ b/sdk/metric/internal/histogram.go @@ -156,8 +156,8 @@ func (s *deltaHistogram[N]) Aggregation() metricdata.Aggregation { Sum: b.sum, } if !s.noMinMax { - hdp.Min = metricdata.Extrema{Value: b.min, Valid: true} - hdp.Max = metricdata.Extrema{Value: b.max, Valid: true} + hdp.Min = metricdata.NewExtrema(b.min) + hdp.Max = metricdata.NewExtrema(b.max) } h.DataPoints = append(h.DataPoints, hdp) @@ -227,8 +227,8 @@ func (s *cumulativeHistogram[N]) Aggregation() metricdata.Aggregation { Sum: b.sum, } if !s.noMinMax { - hdp.Min = metricdata.Extrema{Value: b.min, Valid: true} - hdp.Max = metricdata.Extrema{Value: b.max, Valid: true} + hdp.Min = metricdata.NewExtrema(b.min) + hdp.Max = metricdata.NewExtrema(b.max) } h.DataPoints = append(h.DataPoints, hdp) // TODO (#3006): This will use an unbounded amount of memory if there diff --git a/sdk/metric/internal/histogram_test.go b/sdk/metric/internal/histogram_test.go index 6bf2a78ac9a..57f04560a16 100644 --- a/sdk/metric/internal/histogram_test.go +++ b/sdk/metric/internal/histogram_test.go @@ -92,8 +92,8 @@ func hPoint(a attribute.Set, v float64, multi uint64) metricdata.HistogramDataPo Count: multi, Bounds: bounds, BucketCounts: counts, - Min: metricdata.Extrema{Value: v, Valid: true}, - Max: metricdata.Extrema{Value: v, Valid: true}, + Min: metricdata.NewExtrema(v), + Max: metricdata.NewExtrema(v), Sum: v * float64(multi), } } diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index a5cd85c3b06..0af96cd37da 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -167,7 +167,7 @@ func TestCallbackUnregisterConcurrency(t *testing.T) { // Instruments should produce correct ResourceMetrics. func TestMeterCreatesInstruments(t *testing.T) { - extrema := metricdata.Extrema{Value: 7., Valid: true} + extrema := metricdata.NewExtrema(7.) testCases := []struct { name string fn func(*testing.T, metric.Meter) @@ -860,8 +860,8 @@ func TestAttributeFilter(t *testing.T) { Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, Count: 2, - Min: metricdata.Extrema{Value: 1., Valid: true}, - Max: metricdata.Extrema{Value: 2., Valid: true}, + Min: metricdata.NewExtrema(1.), + Max: metricdata.NewExtrema(2.), Sum: 3.0, }, }, @@ -942,8 +942,8 @@ func TestAttributeFilter(t *testing.T) { Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, BucketCounts: []uint64{0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, Count: 2, - Min: metricdata.Extrema{Value: 1., Valid: true}, - Max: metricdata.Extrema{Value: 2., Valid: true}, + Min: metricdata.NewExtrema(1.), + Max: metricdata.NewExtrema(2.), Sum: 3.0, }, }, diff --git a/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index da6f69dcf34..c11a728b26f 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -138,3 +138,8 @@ type Extrema struct { // is the zero-value. Valid bool } + +// NewExtrema returns an Extrema set to v. +func NewExtrema(v float64) Extrema { + return Extrema{Value: v, Valid: true} +} diff --git a/sdk/metric/metricdata/metricdatatest/assertion_test.go b/sdk/metric/metricdata/metricdatatest/assertion_test.go index a7b0b2a2891..051849879e6 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion_test.go +++ b/sdk/metric/metricdata/metricdatatest/assertion_test.go @@ -89,8 +89,8 @@ var ( Count: 3, Bounds: []float64{0, 10, 100}, BucketCounts: []uint64{1, 1, 1}, - Max: metricdata.Extrema{Valid: true, Value: 99.}, - Min: metricdata.Extrema{Valid: true, Value: 3.}, + Max: metricdata.NewExtrema(99.), + Min: metricdata.NewExtrema(3.), Sum: 3, } histogramDataPointC = metricdata.HistogramDataPoint{ From da5356e15e26458102188bbfcc83d4535231cc35 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 12 Jan 2023 16:31:54 -0800 Subject: [PATCH 5/9] Add metricdatatest tests --- .../metricdata/metricdatatest/assertion_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sdk/metric/metricdata/metricdatatest/assertion_test.go b/sdk/metric/metricdata/metricdatatest/assertion_test.go index 051849879e6..5444550a5dc 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion_test.go +++ b/sdk/metric/metricdata/metricdatatest/assertion_test.go @@ -73,6 +73,10 @@ var ( Value: -1.0, } + extremaA = metricdata.NewExtrema(99.) + extremaB = metricdata.NewExtrema(3.) + extremaC = metricdata.NewExtrema(99.) + histogramDataPointA = metricdata.HistogramDataPoint{ Attributes: attrA, StartTime: startA, @@ -89,8 +93,8 @@ var ( Count: 3, Bounds: []float64{0, 10, 100}, BucketCounts: []uint64{1, 1, 1}, - Max: metricdata.NewExtrema(99.), - Min: metricdata.NewExtrema(3.), + Max: extremaA, + Min: extremaB, Sum: 3, } histogramDataPointC = metricdata.HistogramDataPoint{ @@ -246,6 +250,7 @@ func TestAssertEqual(t *testing.T) { t.Run("HistogramDataPoint", testDatatype(histogramDataPointA, histogramDataPointB, equalHistogramDataPoints)) t.Run("DataPointInt64", testDatatype(dataPointInt64A, dataPointInt64B, equalDataPoints[int64])) t.Run("DataPointFloat64", testDatatype(dataPointFloat64A, dataPointFloat64B, equalDataPoints[float64])) + t.Run("Extrema", testDatatype(extremaA, extremaB, equalExtrema)) } func TestAssertEqualIgnoreTime(t *testing.T) { @@ -260,6 +265,7 @@ func TestAssertEqualIgnoreTime(t *testing.T) { t.Run("HistogramDataPoint", testDatatypeIgnoreTime(histogramDataPointA, histogramDataPointC, equalHistogramDataPoints)) t.Run("DataPointInt64", testDatatypeIgnoreTime(dataPointInt64A, dataPointInt64C, equalDataPoints[int64])) t.Run("DataPointFloat64", testDatatypeIgnoreTime(dataPointFloat64A, dataPointFloat64C, equalDataPoints[float64])) + t.Run("Extrema", testDatatypeIgnoreTime(extremaA, extremaC, equalExtrema)) } type unknownAggregation struct { @@ -315,6 +321,7 @@ func TestAssertAggregationsEqual(t *testing.T) { } func TestAssertAttributes(t *testing.T) { + AssertHasAttributes(t, extremaA, attribute.Bool("A", true)) // No-op, always pass. AssertHasAttributes(t, dataPointInt64A, attribute.Bool("A", true)) AssertHasAttributes(t, dataPointFloat64A, attribute.Bool("A", true)) AssertHasAttributes(t, gaugeInt64A, attribute.Bool("A", true)) From 7e0f5e5f2f03a32c428c1dd35b7ea0b5fbd144cb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 Jan 2023 13:33:22 -0800 Subject: [PATCH 6/9] Use getter for Extrema --- .../otlpmetric/internal/transform/metricdata.go | 10 ++++------ sdk/metric/metricdata/data.go | 16 +++++++++------- .../metricdata/metricdatatest/comparisons.go | 9 ++++++--- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/exporters/otlp/otlpmetric/internal/transform/metricdata.go b/exporters/otlp/otlpmetric/internal/transform/metricdata.go index 152ea554281..d952f8b06be 100644 --- a/exporters/otlp/otlpmetric/internal/transform/metricdata.go +++ b/exporters/otlp/otlpmetric/internal/transform/metricdata.go @@ -183,13 +183,11 @@ func HistogramDataPoints(dPts []metricdata.HistogramDataPoint) []*mpb.HistogramD BucketCounts: dPt.BucketCounts, ExplicitBounds: dPt.Bounds, } - if dPt.Min.Valid { - min := dPt.Min.Value - hdp.Min = &min + if v, ok := dPt.Min.Value(); ok { + hdp.Min = &v } - if dPt.Max.Valid { - max := dPt.Max.Value - hdp.Max = &max + if v, ok := dPt.Max.Value(); ok { + hdp.Max = &v } out = append(out, hdp) } diff --git a/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index c11a728b26f..52a1b613e31 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -131,15 +131,17 @@ type HistogramDataPoint struct { // Extrema are the minimum or maximum values of a data set. type Extrema struct { - // Value is the extrema value. - Value float64 - - // Valid is true if Value has been assigned a value. It is false if Value - // is the zero-value. - Valid bool + value float64 + valid bool } // NewExtrema returns an Extrema set to v. func NewExtrema(v float64) Extrema { - return Extrema{Value: v, Valid: true} + return Extrema{value: v, valid: true} +} + +// Value returns the Extrema value and true if the Extrema is defined. +// Otherwise, if the Extrema is its zero-value, defined will be false. +func (e Extrema) Value() (v float64, defined bool) { + return e.value, e.valid } diff --git a/sdk/metric/metricdata/metricdatatest/comparisons.go b/sdk/metric/metricdata/metricdatatest/comparisons.go index 2827962678d..a6bc83f6b7d 100644 --- a/sdk/metric/metricdata/metricdatatest/comparisons.go +++ b/sdk/metric/metricdata/metricdatatest/comparisons.go @@ -303,10 +303,13 @@ func equalExtrema(a, b metricdata.Extrema, _ config) (reasons []string) { } func eqExtrema(a, b metricdata.Extrema) bool { - if !a.Valid || !b.Valid { - return a.Valid == b.Valid + aV, aOk := a.Value() + bV, bOk := b.Value() + + if !aOk || !bOk { + return aOk == bOk } - return a.Value == b.Value + return aV == bV } func diffSlices[T any](a, b []T, equal func(T, T) bool) (extraA, extraB []T) { From 7db1801cec836e6e90313f387d176ddecb3d35b2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 Jan 2023 13:37:31 -0800 Subject: [PATCH 7/9] Fix Extrema doc language --- sdk/metric/metricdata/data.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index 52a1b613e31..712e79e27cb 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -129,7 +129,7 @@ type HistogramDataPoint struct { Sum float64 } -// Extrema are the minimum or maximum values of a data set. +// Extrema is the minimum or maximum value of a data set. type Extrema struct { value float64 valid bool From f49baa44878d12c68d629e594a883a374bad557f Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 Jan 2023 13:38:17 -0800 Subject: [PATCH 8/9] Correct dataset to be one word --- sdk/metric/metricdata/data.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index 712e79e27cb..cd882c0bd2d 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -129,7 +129,7 @@ type HistogramDataPoint struct { Sum float64 } -// Extrema is the minimum or maximum value of a data set. +// Extrema is the minimum or maximum value of a dataset. type Extrema struct { value float64 valid bool From 5e7d975bcedacddc448d48a893e6eda06b74d9b1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 25 Jan 2023 13:45:00 -0800 Subject: [PATCH 9/9] Ensure multiple extrema are tested in a dataset --- .../metricdatatest/assertion_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/sdk/metric/metricdata/metricdatatest/assertion_test.go b/sdk/metric/metricdata/metricdatatest/assertion_test.go index 5444550a5dc..7056ee71293 100644 --- a/sdk/metric/metricdata/metricdatatest/assertion_test.go +++ b/sdk/metric/metricdata/metricdatatest/assertion_test.go @@ -73,9 +73,9 @@ var ( Value: -1.0, } - extremaA = metricdata.NewExtrema(99.) - extremaB = metricdata.NewExtrema(3.) - extremaC = metricdata.NewExtrema(99.) + minA = metricdata.NewExtrema(-1.) + minB, maxB = metricdata.NewExtrema(3.), metricdata.NewExtrema(99.) + minC = metricdata.NewExtrema(-1.) histogramDataPointA = metricdata.HistogramDataPoint{ Attributes: attrA, @@ -84,6 +84,7 @@ var ( Count: 2, Bounds: []float64{0, 10}, BucketCounts: []uint64{1, 1}, + Min: minA, Sum: 2, } histogramDataPointB = metricdata.HistogramDataPoint{ @@ -93,8 +94,8 @@ var ( Count: 3, Bounds: []float64{0, 10, 100}, BucketCounts: []uint64{1, 1, 1}, - Max: extremaA, - Min: extremaB, + Max: maxB, + Min: minB, Sum: 3, } histogramDataPointC = metricdata.HistogramDataPoint{ @@ -104,6 +105,7 @@ var ( Count: 2, Bounds: []float64{0, 10}, BucketCounts: []uint64{1, 1}, + Min: minC, Sum: 2, } @@ -250,7 +252,7 @@ func TestAssertEqual(t *testing.T) { t.Run("HistogramDataPoint", testDatatype(histogramDataPointA, histogramDataPointB, equalHistogramDataPoints)) t.Run("DataPointInt64", testDatatype(dataPointInt64A, dataPointInt64B, equalDataPoints[int64])) t.Run("DataPointFloat64", testDatatype(dataPointFloat64A, dataPointFloat64B, equalDataPoints[float64])) - t.Run("Extrema", testDatatype(extremaA, extremaB, equalExtrema)) + t.Run("Extrema", testDatatype(minA, minB, equalExtrema)) } func TestAssertEqualIgnoreTime(t *testing.T) { @@ -265,7 +267,7 @@ func TestAssertEqualIgnoreTime(t *testing.T) { t.Run("HistogramDataPoint", testDatatypeIgnoreTime(histogramDataPointA, histogramDataPointC, equalHistogramDataPoints)) t.Run("DataPointInt64", testDatatypeIgnoreTime(dataPointInt64A, dataPointInt64C, equalDataPoints[int64])) t.Run("DataPointFloat64", testDatatypeIgnoreTime(dataPointFloat64A, dataPointFloat64C, equalDataPoints[float64])) - t.Run("Extrema", testDatatypeIgnoreTime(extremaA, extremaC, equalExtrema)) + t.Run("Extrema", testDatatypeIgnoreTime(minA, minC, equalExtrema)) } type unknownAggregation struct { @@ -321,7 +323,7 @@ func TestAssertAggregationsEqual(t *testing.T) { } func TestAssertAttributes(t *testing.T) { - AssertHasAttributes(t, extremaA, attribute.Bool("A", true)) // No-op, always pass. + AssertHasAttributes(t, minA, attribute.Bool("A", true)) // No-op, always pass. AssertHasAttributes(t, dataPointInt64A, attribute.Bool("A", true)) AssertHasAttributes(t, dataPointFloat64A, attribute.Bool("A", true)) AssertHasAttributes(t, gaugeInt64A, attribute.Bool("A", true))