Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Extrema type for Histogram min/max #3550

Merged
merged 19 commits into from Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -50,6 +50,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- `Int64UpDownCounter` replaces the `syncint64.UpDownCounter`
- `Int64Histogram` replaces the `syncint64.Histogram`
- Add `NewTracerProvider` to `go.opentelemetry.io/otel/bridge/opentracing` to create `WrapperTracer` instances from a `TracerProvider`. (#3316)
- 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)
- Add the `go.opentelemetry.io/otel/semconv/v1.17.0` package.
The package contains semantic conventions from the `v1.17.0` version of the OpenTelemetry specification. (#3599)

Expand Down Expand Up @@ -106,6 +107,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
The slice of `instrument.Asynchronous` parameter is now passed as a variadic argument. (#3587)
- The `Callback` in `go.opentelemetry.io/otel/metric` has the added `Observer` parameter added.
This new parameter is used by `Callback` implementations to observe values for asynchronous instruments instead of calling the `Observe` method of the instrument directly. (#3584)
- 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)

### Fixed

Expand Down
13 changes: 9 additions & 4 deletions exporters/otlp/otlpmetric/internal/transform/metricdata.go
Expand Up @@ -174,17 +174,22 @@ func HistogramDataPoints(dPts []metricdata.HistogramDataPoint) []*mpb.HistogramD
out := make([]*mpb.HistogramDataPoint, 0, len(dPts))
for _, dPt := range dPts {
sum := dPt.Sum
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
out = append(out, &mpb.HistogramDataPoint{
hdp := &mpb.HistogramDataPoint{
Attributes: AttrIter(dPt.Attributes.Iter()),
StartTimeUnixNano: uint64(dPt.StartTime.UnixNano()),
TimeUnixNano: uint64(dPt.Time.UnixNano()),
Count: dPt.Count,
Sum: &sum,
BucketCounts: dPt.BucketCounts,
ExplicitBounds: dPt.Bounds,
Min: dPt.Min,
Max: dPt.Max,
})
}
if v, ok := dPt.Min.Value(); ok {
hdp.Min = &v
}
if v, ok := dPt.Max.Value(); ok {
hdp.Max = &v
}
out = append(out, hdp)
}
return out
}
Expand Down
Expand Up @@ -60,8 +60,8 @@ var (
Count: 30,
Bounds: []float64{1, 5},
BucketCounts: []uint64{0, 30, 0},
Min: &minA,
Max: &maxA,
Min: metricdata.NewExtrema(minA),
Max: metricdata.NewExtrema(maxA),
Sum: sumA,
}, {
Attributes: bob,
Expand All @@ -70,8 +70,8 @@ var (
Count: 3,
Bounds: []float64{1, 5},
BucketCounts: []uint64{0, 1, 2},
Min: &minB,
Max: &maxB,
Min: metricdata.NewExtrema(minB),
Max: metricdata.NewExtrema(maxB),
Sum: sumB,
}}

Expand Down
10 changes: 4 additions & 6 deletions sdk/metric/internal/histogram.go
Expand Up @@ -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.NewExtrema(b.min)
hdp.Max = metricdata.NewExtrema(b.max)
}
h.DataPoints = append(h.DataPoints, hdp)

Expand Down Expand Up @@ -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.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
Expand Down
4 changes: 2 additions & 2 deletions sdk/metric/internal/histogram_test.go
Expand Up @@ -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.NewExtrema(v),
Max: metricdata.NewExtrema(v),
Sum: v * float64(multi),
}
}
Expand Down
20 changes: 9 additions & 11 deletions sdk/metric/meter_test.go
Expand Up @@ -169,8 +169,8 @@ func TestCallbackUnregisterConcurrency(t *testing.T) {

// Instruments should produce correct ResourceMetrics.
func TestMeterCreatesInstruments(t *testing.T) {
extrema := metricdata.NewExtrema(7.)
attrs := []attribute.KeyValue{attribute.String("name", "alice")}
seven := 7.0
testCases := []struct {
name string
fn func(*testing.T, metric.Meter)
Expand Down Expand Up @@ -391,8 +391,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,
},
},
Expand Down Expand Up @@ -455,8 +455,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,
},
},
Expand Down Expand Up @@ -882,8 +882,6 @@ func TestAttributeFilter(t *testing.T) {
}

func testAttributeFilter(temporality metricdata.Temporality) func(*testing.T) {
one := 1.0
two := 2.0
testcases := []struct {
name string
register func(t *testing.T, mtr metric.Meter) error
Expand Down Expand Up @@ -1130,8 +1128,8 @@ func testAttributeFilter(temporality metricdata.Temporality) func(*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.NewExtrema(1.),
Max: metricdata.NewExtrema(2.),
Sum: 3.0,
},
},
Expand Down Expand Up @@ -1212,8 +1210,8 @@ func testAttributeFilter(temporality metricdata.Temporality) func(*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.NewExtrema(1.),
Max: metricdata.NewExtrema(2.),
Sum: 3.0,
},
},
Expand Down
21 changes: 19 additions & 2 deletions sdk/metric/metricdata/data.go
Expand Up @@ -122,9 +122,26 @@ 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 is the minimum or maximum value of a dataset.
type Extrema struct {
value float64
valid bool
}

// NewExtrema returns an Extrema set to v.
func NewExtrema(v float64) Extrema {
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
}
5 changes: 5 additions & 0 deletions sdk/metric/metricdata/metricdatatest/assertion.go
Expand Up @@ -32,6 +32,7 @@ type Datatypes interface {
metricdata.Gauge[int64] |
metricdata.Histogram |
metricdata.HistogramDataPoint |
metricdata.Extrema |
metricdata.Metrics |
metricdata.ResourceMetrics |
metricdata.ScopeMetrics |
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -152,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:
Expand Down
14 changes: 11 additions & 3 deletions sdk/metric/metricdata/metricdatatest/assertion_test.go
Expand Up @@ -73,14 +73,18 @@ var (
Value: -1.0,
}

max, min = 99.0, 3.
minA = metricdata.NewExtrema(-1.)
minB, maxB = metricdata.NewExtrema(3.), metricdata.NewExtrema(99.)
minC = metricdata.NewExtrema(-1.)

histogramDataPointA = metricdata.HistogramDataPoint{
Attributes: attrA,
StartTime: startA,
Time: endA,
Count: 2,
Bounds: []float64{0, 10},
BucketCounts: []uint64{1, 1},
Min: minA,
Sum: 2,
}
histogramDataPointB = metricdata.HistogramDataPoint{
Expand All @@ -90,8 +94,8 @@ var (
Count: 3,
Bounds: []float64{0, 10, 100},
BucketCounts: []uint64{1, 1, 1},
Max: &max,
Min: &min,
Max: maxB,
Min: minB,
Sum: 3,
}
histogramDataPointC = metricdata.HistogramDataPoint{
Expand All @@ -101,6 +105,7 @@ var (
Count: 2,
Bounds: []float64{0, 10},
BucketCounts: []uint64{1, 1},
Min: minC,
Sum: 2,
}

Expand Down Expand Up @@ -247,6 +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(minA, minB, equalExtrema))
}

func TestAssertEqualIgnoreTime(t *testing.T) {
Expand All @@ -261,6 +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(minA, minC, equalExtrema))
}

type unknownAggregation struct {
Expand Down Expand Up @@ -316,6 +323,7 @@ func TestAssertAggregationsEqual(t *testing.T) {
}

func TestAssertAttributes(t *testing.T) {
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))
Expand Down
21 changes: 15 additions & 6 deletions sdk/metric/metricdata/metricdatatest/comparisons.go
Expand Up @@ -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 {
Expand All @@ -295,12 +295,21 @@ 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 {
aV, aOk := a.Value()
bV, bOk := b.Value()

if !aOk || !bOk {
return aOk == bOk
}
return aV == bV
}

func diffSlices[T any](a, b []T, equal func(T, T) bool) (extraA, extraB []T) {
Expand Down