From 7f19d7d8ed2dc4bbda8ac2b1cd7e34bf0cfc4696 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Mon, 17 Oct 2022 13:57:23 +0000 Subject: [PATCH 1/3] Added WithAggregationSelector to prometheus --- CHANGELOG.md | 1 + exporters/prometheus/confg_test.go | 60 ++++++++++++++++++++++++++++-- exporters/prometheus/config.go | 19 +++++++++- exporters/prometheus/exporter.go | 2 +- 4 files changed, 76 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baab4d165f4..d3eac347ed1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added - Prometheus exporter will register with a prometheus registerer on creation, there are options to control this. (#3239) +- Added an option to change the AggregationSelector for prometheus. (#????) ### Changed diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 91893d40f3a..1705c86ae89 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -19,22 +19,27 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/aggregation" + "go.opentelemetry.io/otel/sdk/metric/view" ) func TestNewConfig(t *testing.T) { registry := prometheus.NewRegistry() + aggregationSelector := func(view.InstrumentKind) aggregation.Aggregation { return nil } + testCases := []struct { - name string - options []Option - wantRegisterer prometheus.Registerer + name string + options []Option + wantRegisterer prometheus.Registerer + wantAggregation metric.AggregationSelector }{ { name: "Default", options: nil, wantRegisterer: prometheus.DefaultRegisterer, }, - { name: "WithRegisterer", options: []Option{ @@ -42,6 +47,23 @@ func TestNewConfig(t *testing.T) { }, wantRegisterer: registry, }, + { + name: "WithAggregationSelector", + options: []Option{ + WithAggregationSelector(aggregationSelector), + }, + wantRegisterer: prometheus.DefaultRegisterer, + wantAggregation: aggregationSelector, + }, + { + name: "With Multiple Options", + options: []Option{ + WithRegisterer(registry), + WithAggregationSelector(aggregationSelector), + }, + wantRegisterer: registry, + wantAggregation: aggregationSelector, + }, { name: "nil options do nothing", options: []Option{ @@ -58,3 +80,33 @@ func TestNewConfig(t *testing.T) { }) } } + +func TestConfigManualReaderOptions(t *testing.T) { + aggregationSelector := func(view.InstrumentKind) aggregation.Aggregation { return nil } + + testCases := []struct { + name string + config config + wantOption []metric.ManualReaderOption + }{ + { + name: "Default", + config: config{}, + wantOption: []metric.ManualReaderOption{}, + }, + + { + name: "WithAggregationSelector", + config: config{aggregation: aggregationSelector}, + wantOption: []metric.ManualReaderOption{ + metric.WithAggregationSelector(aggregationSelector), + }, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + opts := tt.config.manualReaderOptions() + assert.Len(t, opts, len(tt.wantOption)) + }) + } +} diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index 6ee84732556..ff959434c8d 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -16,11 +16,13 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/sdk/metric" ) // config contains options for the exporter. type config struct { - registerer prometheus.Registerer + registerer prometheus.Registerer + aggregation metric.AggregationSelector } // newConfig creates a validated config configured with options. @@ -37,6 +39,14 @@ func newConfig(opts ...Option) config { return cfg } +func (cfg config) manualReaderOptions() []metric.ManualReaderOption { + opts := []metric.ManualReaderOption{} + if cfg.aggregation != nil { + opts = append(opts, metric.WithAggregationSelector(cfg.aggregation)) + } + return opts +} + // Option sets exporter option values. type Option interface { apply(config) config @@ -57,3 +67,10 @@ func WithRegisterer(reg prometheus.Registerer) Option { return cfg }) } + +func WithAggregationSelector(agg metric.AggregationSelector) Option { + return optionFunc(func(cfg config) config { + cfg.aggregation = agg + return cfg + }) +} diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 007dc2f50d9..1f1787aad56 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -50,7 +50,7 @@ func New(opts ...Option) (*Exporter, error) { // this assumes that the default temporality selector will always return cumulative. // we only support cumulative temporality, so building our own reader enforces this. // TODO (#3244): Enable some way to configure the reader, but not change temporality. - reader := metric.NewManualReader() + reader := metric.NewManualReader(cfg.manualReaderOptions()...) collector := &collector{ reader: reader, From 256fe58601548111b691eb39db4c3dddba13384e Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Tue, 18 Oct 2022 14:20:43 -0500 Subject: [PATCH 2/3] Update CHANGELOG.md Co-authored-by: Tyler Yahn --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3eac347ed1..a5fbd89d208 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added - Prometheus exporter will register with a prometheus registerer on creation, there are options to control this. (#3239) -- Added an option to change the AggregationSelector for prometheus. (#????) +- Added the `WithAggregationSelector` option to the `go.opentelemetry.io/otel/exporters/prometheus` package to change the `AggregationSelector` used. (#3341) ### Changed From a5577e0abe7d4369d1347c5ca8712d02ce26235f Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Tue, 18 Oct 2022 20:58:18 +0000 Subject: [PATCH 3/3] Address PR comments --- exporters/prometheus/confg_test.go | 23 +++++++++++------------ exporters/prometheus/config.go | 4 ++++ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 1705c86ae89..52bccff2587 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -19,6 +19,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/metric/aggregation" "go.opentelemetry.io/otel/sdk/metric/view" @@ -85,28 +86,26 @@ func TestConfigManualReaderOptions(t *testing.T) { aggregationSelector := func(view.InstrumentKind) aggregation.Aggregation { return nil } testCases := []struct { - name string - config config - wantOption []metric.ManualReaderOption + name string + config config + wantOptionCount int }{ { - name: "Default", - config: config{}, - wantOption: []metric.ManualReaderOption{}, + name: "Default", + config: config{}, + wantOptionCount: 0, }, { - name: "WithAggregationSelector", - config: config{aggregation: aggregationSelector}, - wantOption: []metric.ManualReaderOption{ - metric.WithAggregationSelector(aggregationSelector), - }, + name: "WithAggregationSelector", + config: config{aggregation: aggregationSelector}, + wantOptionCount: 1, }, } for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { opts := tt.config.manualReaderOptions() - assert.Len(t, opts, len(tt.wantOption)) + assert.Len(t, opts, tt.wantOptionCount) }) } } diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index ff959434c8d..e70dade501d 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -16,6 +16,7 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/sdk/metric" ) @@ -68,6 +69,9 @@ func WithRegisterer(reg prometheus.Registerer) Option { }) } +// WithAggregationSelector configure the Aggregation Selector the exporter will +// use. If no AggregationSelector is provided the DefaultAggregationSelector is +// used. func WithAggregationSelector(agg metric.AggregationSelector) Option { return optionFunc(func(cfg config) config { cfg.aggregation = agg