From 697d245b9ccf375c1d49e7492d36bceae80acfe4 Mon Sep 17 00:00:00 2001 From: Damien Mathieu <42@dmathieu.com> Date: Wed, 5 Oct 2022 00:41:18 +0200 Subject: [PATCH] Update bucket default bounds (#3222) * update bucket default bounds to match the specification * add changelog entry * test custom boundaries with valid histogram Co-authored-by: Tyler Yahn --- CHANGELOG.md | 1 + exporters/prometheus/exporter_test.go | 19 ++++++++++-- exporters/prometheus/testdata/histogram.txt | 30 +++++++++---------- .../prometheus/testdata/sanitized_names.txt | 5 ++++ sdk/metric/meter_test.go | 8 ++--- sdk/metric/reader.go | 2 +- 6 files changed, 42 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf26a4ff104..cb99127ad4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Flush pending measurements with the `PeriodicReader` in the `go.opentelemetry.io/otel/sdk/metric` when `ForceFlush` or `Shutdown` are called. (#3220) - Upgrade `golang.org/x/sys/unix` from `v0.0.0-20210423185535-09eb48e85fd7` to `v0.0.0-20220919091848-fb04ddd9f9c8`. This addresses [GO-2022-0493](https://pkg.go.dev/vuln/GO-2022-0493). (#3235) +- Update histogram default bounds to match the requirements of the latest specification. (#3222) ## [0.32.1] Metric SDK (Alpha) - 2022-09-22 diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 6d46c3be0db..55db838dcd2 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -27,6 +27,8 @@ import ( otelmetric "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/instrument" "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/aggregation" + "go.opentelemetry.io/otel/sdk/metric/view" ) func TestPrometheusExporter(t *testing.T) { @@ -74,7 +76,7 @@ func TestPrometheusExporter(t *testing.T) { attribute.Key("A").String("B"), attribute.Key("C").String("D"), } - histogram, err := meter.SyncFloat64().Histogram("baz", instrument.WithDescription("a very nice histogram")) + histogram, err := meter.SyncFloat64().Histogram("histogram_baz", instrument.WithDescription("a very nice histogram")) require.NoError(t, err) histogram.Record(ctx, 23, attrs...) histogram.Record(ctx, 7, attrs...) @@ -137,11 +139,22 @@ func TestPrometheusExporter(t *testing.T) { ctx := context.Background() exporter := New() - provider := metric.NewMeterProvider(metric.WithReader(exporter)) + + customBucketsView, err := view.New( + view.MatchInstrumentName("histogram_*"), + view.WithSetAggregation(aggregation.ExplicitBucketHistogram{ + Boundaries: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 1000}, + }), + ) + require.NoError(t, err) + defaultView, err := view.New(view.MatchInstrumentName("*")) + require.NoError(t, err) + + provider := metric.NewMeterProvider(metric.WithReader(exporter, customBucketsView, defaultView)) meter := provider.Meter("testmeter") registry := prometheus.NewRegistry() - err := registry.Register(exporter.Collector) + err = registry.Register(exporter.Collector) require.NoError(t, err) tc.recordMetrics(ctx, meter) diff --git a/exporters/prometheus/testdata/histogram.txt b/exporters/prometheus/testdata/histogram.txt index 547599cf6d5..3a8422bb573 100644 --- a/exporters/prometheus/testdata/histogram.txt +++ b/exporters/prometheus/testdata/histogram.txt @@ -1,15 +1,15 @@ -# HELP baz a very nice histogram -# TYPE baz histogram -baz_bucket{A="B",C="D",le="0"} 0 -baz_bucket{A="B",C="D",le="5"} 0 -baz_bucket{A="B",C="D",le="10"} 1 -baz_bucket{A="B",C="D",le="25"} 1 -baz_bucket{A="B",C="D",le="50"} 0 -baz_bucket{A="B",C="D",le="75"} 0 -baz_bucket{A="B",C="D",le="100"} 0 -baz_bucket{A="B",C="D",le="250"} 2 -baz_bucket{A="B",C="D",le="500"} 0 -baz_bucket{A="B",C="D",le="1000"} 0 -baz_bucket{A="B",C="D",le="+Inf"} 4 -baz_sum{A="B",C="D"} 236 -baz_count{A="B",C="D"} 4 +# HELP histogram_baz a very nice histogram +# TYPE histogram_baz histogram +histogram_baz_bucket{A="B",C="D",le="0"} 0 +histogram_baz_bucket{A="B",C="D",le="5"} 0 +histogram_baz_bucket{A="B",C="D",le="10"} 1 +histogram_baz_bucket{A="B",C="D",le="25"} 1 +histogram_baz_bucket{A="B",C="D",le="50"} 0 +histogram_baz_bucket{A="B",C="D",le="75"} 0 +histogram_baz_bucket{A="B",C="D",le="100"} 0 +histogram_baz_bucket{A="B",C="D",le="250"} 2 +histogram_baz_bucket{A="B",C="D",le="500"} 0 +histogram_baz_bucket{A="B",C="D",le="1000"} 0 +histogram_baz_bucket{A="B",C="D",le="+Inf"} 4 +histogram_baz_sum{A="B",C="D"} 236 +histogram_baz_count{A="B",C="D"} 4 diff --git a/exporters/prometheus/testdata/sanitized_names.txt b/exporters/prometheus/testdata/sanitized_names.txt index 5e9516716ae..0158d17aeb6 100644 --- a/exporters/prometheus/testdata/sanitized_names.txt +++ b/exporters/prometheus/testdata/sanitized_names.txt @@ -18,7 +18,12 @@ invalid_hist_name_bucket{A="B",C="D",le="75"} 0 invalid_hist_name_bucket{A="B",C="D",le="100"} 0 invalid_hist_name_bucket{A="B",C="D",le="250"} 0 invalid_hist_name_bucket{A="B",C="D",le="500"} 0 +invalid_hist_name_bucket{A="B",C="D",le="750"} 0 invalid_hist_name_bucket{A="B",C="D",le="1000"} 0 +invalid_hist_name_bucket{A="B",C="D",le="2500"} 0 +invalid_hist_name_bucket{A="B",C="D",le="5000"} 0 +invalid_hist_name_bucket{A="B",C="D",le="7500"} 0 +invalid_hist_name_bucket{A="B",C="D",le="10000"} 0 invalid_hist_name_bucket{A="B",C="D",le="+Inf"} 1 invalid_hist_name_sum{A="B",C="D"} 23 invalid_hist_name_count{A="B",C="D"} 1 diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 7d6923fdd58..a67aa507d06 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -333,8 +333,8 @@ func TestMeterCreatesInstruments(t *testing.T) { { Attributes: attribute.Set{}, Count: 1, - Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 1000}, - BucketCounts: []uint64{0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0}, + 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, Sum: 7.0, @@ -397,8 +397,8 @@ func TestMeterCreatesInstruments(t *testing.T) { { Attributes: attribute.Set{}, Count: 1, - Bounds: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 1000}, - BucketCounts: []uint64{0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0}, + 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, Sum: 7.0, diff --git a/sdk/metric/reader.go b/sdk/metric/reader.go index bf596c27887..e5a1282db6b 100644 --- a/sdk/metric/reader.go +++ b/sdk/metric/reader.go @@ -166,7 +166,7 @@ func DefaultAggregationSelector(ik view.InstrumentKind) aggregation.Aggregation return aggregation.LastValue{} case view.SyncHistogram: return aggregation.ExplicitBucketHistogram{ - Boundaries: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 1000}, + Boundaries: []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}, NoMinMax: false, } }