From c5b4b9b6b7a1b1122e40e4bd4a27c2090439d428 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Wed, 28 Sep 2022 20:02:43 +0000 Subject: [PATCH 1/8] refactor prometheus exporter This change will automatically register the exporter with prometheus, and provides an httpHandler for triggering collect --- example/prometheus/go.mod | 2 +- example/prometheus/main.go | 27 +++--- example/view/main.go | 5 +- exporters/prometheus/benchmark_test.go | 8 +- exporters/prometheus/confg_test.go | 109 +++++++++++++++++++++++++ exporters/prometheus/config.go | 74 +++++++++++++++++ exporters/prometheus/exporter.go | 52 +++++++----- exporters/prometheus/exporter_test.go | 8 +- 8 files changed, 236 insertions(+), 49 deletions(-) create mode 100644 exporters/prometheus/confg_test.go create mode 100644 exporters/prometheus/config.go diff --git a/example/prometheus/go.mod b/example/prometheus/go.mod index 1672cc93961..1a597fc8d1a 100644 --- a/example/prometheus/go.mod +++ b/example/prometheus/go.mod @@ -3,7 +3,6 @@ module go.opentelemetry.io/otel/example/prometheus go 1.18 require ( - github.com/prometheus/client_golang v1.13.0 go.opentelemetry.io/otel v1.10.0 go.opentelemetry.io/otel/exporters/prometheus v0.32.1 go.opentelemetry.io/otel/metric v0.32.1 @@ -17,6 +16,7 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect + github.com/prometheus/client_golang v1.13.0 // indirect github.com/prometheus/client_model v0.2.0 // indirect github.com/prometheus/common v0.37.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect diff --git a/example/prometheus/main.go b/example/prometheus/main.go index 0517da33c2f..7a93454d58e 100644 --- a/example/prometheus/main.go +++ b/example/prometheus/main.go @@ -22,11 +22,8 @@ import ( "os" "os/signal" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promhttp" - "go.opentelemetry.io/otel/attribute" - otelprom "go.opentelemetry.io/otel/exporters/prometheus" + "go.opentelemetry.io/otel/exporters/prometheus" "go.opentelemetry.io/otel/metric/instrument" "go.opentelemetry.io/otel/sdk/metric" ) @@ -37,12 +34,15 @@ func main() { // The exporter embeds a default OpenTelemetry Reader and // implements prometheus.Collector, allowing it to be used as // both a Reader and Collector. - exporter := otelprom.New() + exporter, err := prometheus.New() + if err != nil { + log.Fatal(err) + } provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("github.com/open-telemetry/opentelemetry-go/example/prometheus") // Start the prometheus HTTP server and pass the exporter Collector to it - go serveMetrics(exporter.Collector) + go serveMetrics(exporter) attrs := []attribute.KeyValue{ attribute.Key("A").String("B"), @@ -77,17 +77,10 @@ func main() { <-ctx.Done() } -func serveMetrics(collector prometheus.Collector) { - registry := prometheus.NewRegistry() - err := registry.Register(collector) - if err != nil { - fmt.Printf("error registering collector: %v", err) - return - } - - log.Printf("serving metrics at localhost:2222/metrics") - http.Handle("/metrics", promhttp.HandlerFor(registry, promhttp.HandlerOpts{})) - err = http.ListenAndServe(":2222", nil) +func serveMetrics(exp *prometheus.Exporter) { + log.Printf("serving metrics at localhost:2223/metrics") + http.Handle("/metrics", exp) + err := http.ListenAndServe(":2223", nil) if err != nil { fmt.Printf("error serving http: %v", err) return diff --git a/example/view/main.go b/example/view/main.go index 872e12dde1f..5e7eb8265a6 100644 --- a/example/view/main.go +++ b/example/view/main.go @@ -40,7 +40,10 @@ func main() { ctx := context.Background() // The exporter embeds a default OpenTelemetry Reader, allowing it to be used in WithReader. - exporter := otelprom.New() + exporter, err := otelprom.New() + if err != nil { + log.Fatal(err) + } // View to customize histogram buckets and rename a single histogram instrument. customBucketsView, err := view.New( diff --git a/exporters/prometheus/benchmark_test.go b/exporters/prometheus/benchmark_test.go index dee0814ed75..224626ee781 100644 --- a/exporters/prometheus/benchmark_test.go +++ b/exporters/prometheus/benchmark_test.go @@ -27,14 +27,12 @@ import ( func benchmarkCollect(b *testing.B, n int) { ctx := context.Background() - exporter := New() + registry := prometheus.NewRegistry() // This is the default behavior, this is used to manually gather. + exporter, err := New(WithRegistry(registry)) + require.NoError(b, err) provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("testmeter") - registry := prometheus.NewRegistry() - err := registry.Register(exporter.Collector) - require.NoError(b, err) - for i := 0; i < n; i++ { counter, err := meter.SyncFloat64().Counter(fmt.Sprintf("foo_%d", i)) require.NoError(b, err) diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go new file mode 100644 index 00000000000..7210ea97a76 --- /dev/null +++ b/exporters/prometheus/confg_test.go @@ -0,0 +1,109 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" + +import ( + "context" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" +) + +type testExporter struct{} + +func (e testExporter) Export(_ context.Context, _ metricdata.ResourceMetrics) error { + return nil +} + +func (e testExporter) ForceFlush(_ context.Context) error { + return nil +} + +func (e testExporter) Shutdown(_ context.Context) error { + return nil +} + +func TestNewConfig(t *testing.T) { + registry := prometheus.NewRegistry() + + testCases := []struct { + name string + options []Option + wantReaderType metric.Reader + wantRegisterer prometheus.Registerer + wantGatherer prometheus.Gatherer + }{ + { + name: "Default", + options: nil, + wantReaderType: metric.NewManualReader(), + wantRegisterer: prometheus.DefaultRegisterer, + wantGatherer: prometheus.DefaultGatherer, + }, + { + name: "WithReader", + options: []Option{ + WithReader(metric.NewPeriodicReader(testExporter{})), + }, + wantReaderType: metric.NewPeriodicReader(testExporter{}), + wantRegisterer: prometheus.DefaultRegisterer, + wantGatherer: prometheus.DefaultGatherer, + }, + { + name: "WithRegistry", + options: []Option{ + WithRegistry(registry), + }, + wantReaderType: metric.NewManualReader(), + wantRegisterer: registry, + wantGatherer: registry, + }, + { + name: "Multiple Options", + options: []Option{ + WithReader(metric.NewPeriodicReader(testExporter{})), + WithRegistry(registry), + }, + wantReaderType: metric.NewPeriodicReader(testExporter{}), + wantRegisterer: registry, + wantGatherer: registry, + }, + { + name: "nil options do nothing", + options: []Option{ + WithReader(nil), + WithRegistry(nil), + }, + wantReaderType: metric.NewManualReader(), + wantRegisterer: prometheus.DefaultRegisterer, + wantGatherer: prometheus.DefaultGatherer, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + cfg := newConfig(tt.options...) + + // If no reader is provided you should get a new ManualReader. + assert.IsType(t, tt.wantReaderType, cfg.reader) + + // If no Registry is provided you should get the DefaultRegisterer and DefaultGatherer. + assert.Equal(t, tt.wantRegisterer, cfg.registerer) + assert.Equal(t, tt.wantGatherer, cfg.gatherer) + }) + } +} diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go new file mode 100644 index 00000000000..43bea2f9228 --- /dev/null +++ b/exporters/prometheus/config.go @@ -0,0 +1,74 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" + +import ( + "github.com/prometheus/client_golang/prometheus" + + "go.opentelemetry.io/otel/sdk/metric" +) // config is added here to allow for options expansion in the future. +type config struct { + reader metric.Reader + + registry *prometheus.Registry + registerer prometheus.Registerer + gatherer prometheus.Gatherer +} + +func newConfig(opts ...Option) config { + cfg := config{} + for _, opt := range opts { + cfg = opt.apply(cfg) + } + + if cfg.reader == nil { + cfg.reader = metric.NewManualReader() + } + + if cfg.registry != nil { + cfg.registerer = cfg.registry + cfg.gatherer = cfg.registry + } else { + cfg.registerer = prometheus.DefaultRegisterer + cfg.gatherer = prometheus.DefaultGatherer + } + + return cfg +} + +// Option may be used in the future to apply options to a Prometheus Exporter config. +type Option interface { + apply(config) config +} + +type optionFunc func(config) config + +func (fn optionFunc) apply(cfg config) config { + return fn(cfg) +} + +func WithReader(rdr metric.Reader) Option { + return optionFunc(func(cfg config) config { + cfg.reader = rdr + return cfg + }) +} + +func WithRegistry(reg *prometheus.Registry) Option { + return optionFunc(func(cfg config) config { + cfg.registry = reg + return cfg + }) +} diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 3fe848f1e03..7f48603ad88 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -16,12 +16,15 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "context" + "fmt" + "net/http" "sort" "strings" "unicode" "unicode/utf8" "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -34,38 +37,47 @@ import ( type Exporter struct { metric.Reader Collector prometheus.Collector + + handler http.Handler } +var _ metric.Reader = &Exporter{} + // collector is used to implement prometheus.Collector. type collector struct { - metric.Reader + reader metric.Reader } -// config is added here to allow for options expansion in the future. -type config struct{} +// New returns a Prometheus Exporter. +func New(opts ...Option) (*Exporter, error) { + cfg := newConfig(opts...) -// Option may be used in the future to apply options to a Prometheus Exporter config. -type Option interface { - apply(config) config -} + handler := promhttp.HandlerFor(cfg.gatherer, promhttp.HandlerOpts{}) + collector := &collector{ + reader: cfg.reader, + } -// New returns a Prometheus Exporter. -func New(_ ...Option) Exporter { - // this assumes that the default temporality selector will always return cumulative. - // we only support cumulative temporality, so building our own reader enforces this. - reader := metric.NewManualReader() - e := Exporter{ - Reader: reader, - Collector: &collector{ - Reader: reader, - }, + if err := cfg.registerer.Register(collector); err != nil { + return nil, fmt.Errorf("cannot register the collector: %w", err) } - return e + + e := &Exporter{ + Reader: cfg.reader, + Collector: collector, + + handler: handler, + } + + return e, nil +} + +func (e *Exporter) ServeHTTP(w http.ResponseWriter, r *http.Request) { + e.handler.ServeHTTP(w, r) } // Describe implements prometheus.Collector. func (c *collector) Describe(ch chan<- *prometheus.Desc) { - metrics, err := c.Reader.Collect(context.TODO()) + metrics, err := c.reader.Collect(context.TODO()) if err != nil { otel.Handle(err) } @@ -76,7 +88,7 @@ func (c *collector) Describe(ch chan<- *prometheus.Desc) { // Collect implements prometheus.Collector. func (c *collector) Collect(ch chan<- prometheus.Metric) { - metrics, err := c.Reader.Collect(context.TODO()) + metrics, err := c.reader.Collect(context.TODO()) if err != nil { otel.Handle(err) } diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 6d46c3be0db..8de3afd133a 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -135,15 +135,13 @@ func TestPrometheusExporter(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { ctx := context.Background() + registry := prometheus.NewRegistry() // This is the default behavior, this is used to manually gather. - exporter := New() + exporter, err := New(WithRegistry(registry)) + require.NoError(t, err) provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("testmeter") - registry := prometheus.NewRegistry() - err := registry.Register(exporter.Collector) - require.NoError(t, err) - tc.recordMetrics(ctx, meter) file, err := os.Open(tc.expectedFile) From 5b92528b490f3bb62091f69de103bf9db69cad51 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 29 Sep 2022 16:03:01 +0000 Subject: [PATCH 2/8] Fix Lint Remote WithRegistry Adds WithRegisterer and WithGatherer --- exporters/prometheus/config.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index 43bea2f9228..37cae9a742d 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -22,7 +22,6 @@ import ( type config struct { reader metric.Reader - registry *prometheus.Registry registerer prometheus.Registerer gatherer prometheus.Gatherer } @@ -37,13 +36,12 @@ func newConfig(opts ...Option) config { cfg.reader = metric.NewManualReader() } - if cfg.registry != nil { - cfg.registerer = cfg.registry - cfg.gatherer = cfg.registry - } else { - cfg.registerer = prometheus.DefaultRegisterer + if cfg.gatherer == nil { cfg.gatherer = prometheus.DefaultGatherer } + if cfg.registerer == nil { + cfg.registerer = prometheus.DefaultRegisterer + } return cfg } @@ -59,6 +57,8 @@ func (fn optionFunc) apply(cfg config) config { return fn(cfg) } +// WithReader controls where the Exporter reader Collects() from. If no reader +// is passed a ManualReader will be used. func WithReader(rdr metric.Reader) Option { return optionFunc(func(cfg config) config { cfg.reader = rdr @@ -66,9 +66,22 @@ func WithReader(rdr metric.Reader) Option { }) } -func WithRegistry(reg *prometheus.Registry) Option { +// WithRegisterer configures which prometheus Registerer the Exporter will +// register with. If no registerer is used the prometheus DefaultRegisterer is +// used. +func WithRegisterer(reg prometheus.Registerer) Option { + return optionFunc(func(cfg config) config { + cfg.registerer = reg + return cfg + }) +} + +// WithRegisterer configures which prometheus Gatherer the Exporter will +// Gather from. If no gatherer is used the prometheus DefaultGatherer is +// used. +func WithGatherer(gatherer prometheus.Gatherer) Option { return optionFunc(func(cfg config) config { - cfg.registry = reg + cfg.gatherer = gatherer return cfg }) } From 96abfede3d5594ebf50a4c16cfef4a78b0e3440a Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 29 Sep 2022 16:51:14 +0000 Subject: [PATCH 3/8] Fix Tests --- exporters/prometheus/benchmark_test.go | 4 ++-- exporters/prometheus/confg_test.go | 21 ++++++++++++++++----- exporters/prometheus/exporter_test.go | 4 ++-- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/exporters/prometheus/benchmark_test.go b/exporters/prometheus/benchmark_test.go index 224626ee781..be85665b951 100644 --- a/exporters/prometheus/benchmark_test.go +++ b/exporters/prometheus/benchmark_test.go @@ -27,8 +27,8 @@ import ( func benchmarkCollect(b *testing.B, n int) { ctx := context.Background() - registry := prometheus.NewRegistry() // This is the default behavior, this is used to manually gather. - exporter, err := New(WithRegistry(registry)) + registry := prometheus.NewRegistry() + exporter, err := New(WithGatherer(registry), WithRegisterer(registry)) require.NoError(b, err) provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("testmeter") diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 7210ea97a76..1cb96f5172f 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -65,19 +65,29 @@ func TestNewConfig(t *testing.T) { wantGatherer: prometheus.DefaultGatherer, }, { - name: "WithRegistry", + name: "WithGatherer", options: []Option{ - WithRegistry(registry), + WithGatherer(registry), }, wantReaderType: metric.NewManualReader(), - wantRegisterer: registry, + wantRegisterer: prometheus.DefaultRegisterer, wantGatherer: registry, }, + { + name: "WithRegisterer", + options: []Option{ + WithRegisterer(registry), + }, + wantReaderType: metric.NewManualReader(), + wantRegisterer: registry, + wantGatherer: prometheus.DefaultGatherer, + }, { name: "Multiple Options", options: []Option{ WithReader(metric.NewPeriodicReader(testExporter{})), - WithRegistry(registry), + WithGatherer(registry), + WithRegisterer(registry), }, wantReaderType: metric.NewPeriodicReader(testExporter{}), wantRegisterer: registry, @@ -87,7 +97,8 @@ func TestNewConfig(t *testing.T) { name: "nil options do nothing", options: []Option{ WithReader(nil), - WithRegistry(nil), + WithGatherer(nil), + WithRegisterer(nil), }, wantReaderType: metric.NewManualReader(), wantRegisterer: prometheus.DefaultRegisterer, diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 8de3afd133a..38452192162 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -135,9 +135,9 @@ func TestPrometheusExporter(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { ctx := context.Background() - registry := prometheus.NewRegistry() // This is the default behavior, this is used to manually gather. + registry := prometheus.NewRegistry() - exporter, err := New(WithRegistry(registry)) + exporter, err := New(WithGatherer(registry), WithRegisterer(registry)) require.NoError(t, err) provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("testmeter") From 406817dab0db3d17b0f6705751fae9201b249ba5 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 29 Sep 2022 16:52:30 +0000 Subject: [PATCH 4/8] Fix lint --- exporters/prometheus/confg_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 1cb96f5172f..0a409e07cd7 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -20,6 +20,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/metricdata" ) From cbb800151be1cc5db2414315bf1785019bfb0afc Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 29 Sep 2022 20:44:58 +0000 Subject: [PATCH 5/8] Removed reader config can be added after (#3244) --- exporters/prometheus/confg_test.go | 39 +----------------------------- exporters/prometheus/config.go | 17 ------------- exporters/prometheus/exporter.go | 7 ++++-- 3 files changed, 6 insertions(+), 57 deletions(-) diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 0a409e07cd7..fda6641bcaa 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -15,62 +15,33 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( - "context" "testing" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" - - "go.opentelemetry.io/otel/sdk/metric" - "go.opentelemetry.io/otel/sdk/metric/metricdata" ) -type testExporter struct{} - -func (e testExporter) Export(_ context.Context, _ metricdata.ResourceMetrics) error { - return nil -} - -func (e testExporter) ForceFlush(_ context.Context) error { - return nil -} - -func (e testExporter) Shutdown(_ context.Context) error { - return nil -} - func TestNewConfig(t *testing.T) { registry := prometheus.NewRegistry() testCases := []struct { name string options []Option - wantReaderType metric.Reader wantRegisterer prometheus.Registerer wantGatherer prometheus.Gatherer }{ { name: "Default", options: nil, - wantReaderType: metric.NewManualReader(), - wantRegisterer: prometheus.DefaultRegisterer, - wantGatherer: prometheus.DefaultGatherer, - }, - { - name: "WithReader", - options: []Option{ - WithReader(metric.NewPeriodicReader(testExporter{})), - }, - wantReaderType: metric.NewPeriodicReader(testExporter{}), wantRegisterer: prometheus.DefaultRegisterer, wantGatherer: prometheus.DefaultGatherer, }, + { name: "WithGatherer", options: []Option{ WithGatherer(registry), }, - wantReaderType: metric.NewManualReader(), wantRegisterer: prometheus.DefaultRegisterer, wantGatherer: registry, }, @@ -79,29 +50,24 @@ func TestNewConfig(t *testing.T) { options: []Option{ WithRegisterer(registry), }, - wantReaderType: metric.NewManualReader(), wantRegisterer: registry, wantGatherer: prometheus.DefaultGatherer, }, { name: "Multiple Options", options: []Option{ - WithReader(metric.NewPeriodicReader(testExporter{})), WithGatherer(registry), WithRegisterer(registry), }, - wantReaderType: metric.NewPeriodicReader(testExporter{}), wantRegisterer: registry, wantGatherer: registry, }, { name: "nil options do nothing", options: []Option{ - WithReader(nil), WithGatherer(nil), WithRegisterer(nil), }, - wantReaderType: metric.NewManualReader(), wantRegisterer: prometheus.DefaultRegisterer, wantGatherer: prometheus.DefaultGatherer, }, @@ -110,9 +76,6 @@ func TestNewConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := newConfig(tt.options...) - // If no reader is provided you should get a new ManualReader. - assert.IsType(t, tt.wantReaderType, cfg.reader) - // If no Registry is provided you should get the DefaultRegisterer and DefaultGatherer. assert.Equal(t, tt.wantRegisterer, cfg.registerer) assert.Equal(t, tt.wantGatherer, cfg.gatherer) diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index 37cae9a742d..65e85d675d6 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -16,12 +16,8 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "github.com/prometheus/client_golang/prometheus" - - "go.opentelemetry.io/otel/sdk/metric" ) // config is added here to allow for options expansion in the future. type config struct { - reader metric.Reader - registerer prometheus.Registerer gatherer prometheus.Gatherer } @@ -32,10 +28,6 @@ func newConfig(opts ...Option) config { cfg = opt.apply(cfg) } - if cfg.reader == nil { - cfg.reader = metric.NewManualReader() - } - if cfg.gatherer == nil { cfg.gatherer = prometheus.DefaultGatherer } @@ -57,15 +49,6 @@ func (fn optionFunc) apply(cfg config) config { return fn(cfg) } -// WithReader controls where the Exporter reader Collects() from. If no reader -// is passed a ManualReader will be used. -func WithReader(rdr metric.Reader) Option { - return optionFunc(func(cfg config) config { - cfg.reader = rdr - return cfg - }) -} - // WithRegisterer configures which prometheus Registerer the Exporter will // register with. If no registerer is used the prometheus DefaultRegisterer is // used. diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 7f48603ad88..47467468d44 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -52,9 +52,12 @@ type collector struct { func New(opts ...Option) (*Exporter, error) { cfg := newConfig(opts...) + // TODO (#????): Enable some way to configure the reader, but not change temporality. + reader := metric.NewManualReader() + handler := promhttp.HandlerFor(cfg.gatherer, promhttp.HandlerOpts{}) collector := &collector{ - reader: cfg.reader, + reader: reader, } if err := cfg.registerer.Register(collector); err != nil { @@ -62,7 +65,7 @@ func New(opts ...Option) (*Exporter, error) { } e := &Exporter{ - Reader: cfg.reader, + Reader: reader, Collector: collector, handler: handler, From c00a0fabf92f98ae10cae9cff3459ad613cc0de7 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 6 Oct 2022 18:25:23 +0000 Subject: [PATCH 6/8] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb99127ad4f..63675c8caf4 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 - Added an example of using metric views to customize instruments. (#3177) +- Prometheus exporter will register with a prometheus registerer on creation, there are options to control this (#3239) ### Changed From 0e3bbb920faf753fe34705524853bb301fb6d63b Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Fri, 7 Oct 2022 15:04:11 +0000 Subject: [PATCH 7/8] Update with PR feedback --- CHANGELOG.md | 3 ++- example/prometheus/go.mod | 2 +- example/prometheus/main.go | 8 +++++--- example/view/main.go | 16 ++++------------ exporters/prometheus/benchmark_test.go | 2 +- exporters/prometheus/confg_test.go | 23 ----------------------- exporters/prometheus/config.go | 18 +++--------------- exporters/prometheus/exporter.go | 19 ++++--------------- exporters/prometheus/exporter_test.go | 2 +- 9 files changed, 21 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63675c8caf4..e324643330c 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 - Added an example of using metric views to customize instruments. (#3177) -- Prometheus exporter will register with a prometheus registerer on creation, there are options to control this (#3239) +- Prometheus exporter will register with a prometheus registerer on creation, there are options to control this. (#3239) ### Changed @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - 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) +- Prometheus New() function now also returns an error. (#3239) ## [0.32.1] Metric SDK (Alpha) - 2022-09-22 diff --git a/example/prometheus/go.mod b/example/prometheus/go.mod index 1a597fc8d1a..1672cc93961 100644 --- a/example/prometheus/go.mod +++ b/example/prometheus/go.mod @@ -3,6 +3,7 @@ module go.opentelemetry.io/otel/example/prometheus go 1.18 require ( + github.com/prometheus/client_golang v1.13.0 go.opentelemetry.io/otel v1.10.0 go.opentelemetry.io/otel/exporters/prometheus v0.32.1 go.opentelemetry.io/otel/metric v0.32.1 @@ -16,7 +17,6 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect - github.com/prometheus/client_golang v1.13.0 // indirect github.com/prometheus/client_model v0.2.0 // indirect github.com/prometheus/common v0.37.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect diff --git a/example/prometheus/main.go b/example/prometheus/main.go index 7a93454d58e..e1d87693c22 100644 --- a/example/prometheus/main.go +++ b/example/prometheus/main.go @@ -22,6 +22,8 @@ import ( "os" "os/signal" + "github.com/prometheus/client_golang/prometheus/promhttp" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/exporters/prometheus" "go.opentelemetry.io/otel/metric/instrument" @@ -42,7 +44,7 @@ func main() { meter := provider.Meter("github.com/open-telemetry/opentelemetry-go/example/prometheus") // Start the prometheus HTTP server and pass the exporter Collector to it - go serveMetrics(exporter) + go serveMetrics() attrs := []attribute.KeyValue{ attribute.Key("A").String("B"), @@ -77,9 +79,9 @@ func main() { <-ctx.Done() } -func serveMetrics(exp *prometheus.Exporter) { +func serveMetrics() { log.Printf("serving metrics at localhost:2223/metrics") - http.Handle("/metrics", exp) + http.Handle("/metrics", promhttp.Handler()) err := http.ListenAndServe(":2223", nil) if err != nil { fmt.Printf("error serving http: %v", err) diff --git a/example/view/main.go b/example/view/main.go index 5e7eb8265a6..c8f1b246590 100644 --- a/example/view/main.go +++ b/example/view/main.go @@ -22,7 +22,6 @@ import ( "os" "os/signal" - "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "go.opentelemetry.io/otel/attribute" @@ -71,7 +70,7 @@ func main() { meter := provider.Meter(meterName) // Start the prometheus HTTP server and pass the exporter Collector to it - go serveMetrics(exporter.Collector) + go serveMetrics() attrs := []attribute.KeyValue{ attribute.Key("A").String("B"), @@ -97,17 +96,10 @@ func main() { <-ctx.Done() } -func serveMetrics(collector prometheus.Collector) { - registry := prometheus.NewRegistry() - err := registry.Register(collector) - if err != nil { - fmt.Printf("error registering collector: %v", err) - return - } - +func serveMetrics() { log.Printf("serving metrics at localhost:2222/metrics") - http.Handle("/metrics", promhttp.HandlerFor(registry, promhttp.HandlerOpts{})) - err = http.ListenAndServe(":2222", nil) + http.Handle("/metrics", promhttp.Handler()) + err := http.ListenAndServe(":2222", nil) if err != nil { fmt.Printf("error serving http: %v", err) return diff --git a/exporters/prometheus/benchmark_test.go b/exporters/prometheus/benchmark_test.go index be85665b951..6c6d67cae76 100644 --- a/exporters/prometheus/benchmark_test.go +++ b/exporters/prometheus/benchmark_test.go @@ -28,7 +28,7 @@ import ( func benchmarkCollect(b *testing.B, n int) { ctx := context.Background() registry := prometheus.NewRegistry() - exporter, err := New(WithGatherer(registry), WithRegisterer(registry)) + exporter, err := New(WithRegisterer(registry)) require.NoError(b, err) provider := metric.NewMeterProvider(metric.WithReader(exporter)) meter := provider.Meter("testmeter") diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index fda6641bcaa..29c63a97572 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -28,48 +28,26 @@ func TestNewConfig(t *testing.T) { name string options []Option wantRegisterer prometheus.Registerer - wantGatherer prometheus.Gatherer }{ { name: "Default", options: nil, wantRegisterer: prometheus.DefaultRegisterer, - wantGatherer: prometheus.DefaultGatherer, }, - { - name: "WithGatherer", - options: []Option{ - WithGatherer(registry), - }, - wantRegisterer: prometheus.DefaultRegisterer, - wantGatherer: registry, - }, { name: "WithRegisterer", options: []Option{ WithRegisterer(registry), }, wantRegisterer: registry, - wantGatherer: prometheus.DefaultGatherer, - }, - { - name: "Multiple Options", - options: []Option{ - WithGatherer(registry), - WithRegisterer(registry), - }, - wantRegisterer: registry, - wantGatherer: registry, }, { name: "nil options do nothing", options: []Option{ - WithGatherer(nil), WithRegisterer(nil), }, wantRegisterer: prometheus.DefaultRegisterer, - wantGatherer: prometheus.DefaultGatherer, }, } for _, tt := range testCases { @@ -78,7 +56,6 @@ func TestNewConfig(t *testing.T) { // If no Registry is provided you should get the DefaultRegisterer and DefaultGatherer. assert.Equal(t, tt.wantRegisterer, cfg.registerer) - assert.Equal(t, tt.wantGatherer, cfg.gatherer) }) } } diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index 65e85d675d6..faf09f9a188 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -16,10 +16,11 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "github.com/prometheus/client_golang/prometheus" -) // config is added here to allow for options expansion in the future. +) + +// config is added here to allow for options expansion in the future. type config struct { registerer prometheus.Registerer - gatherer prometheus.Gatherer } func newConfig(opts ...Option) config { @@ -28,9 +29,6 @@ func newConfig(opts ...Option) config { cfg = opt.apply(cfg) } - if cfg.gatherer == nil { - cfg.gatherer = prometheus.DefaultGatherer - } if cfg.registerer == nil { cfg.registerer = prometheus.DefaultRegisterer } @@ -58,13 +56,3 @@ func WithRegisterer(reg prometheus.Registerer) Option { return cfg }) } - -// WithRegisterer configures which prometheus Gatherer the Exporter will -// Gather from. If no gatherer is used the prometheus DefaultGatherer is -// used. -func WithGatherer(gatherer prometheus.Gatherer) Option { - return optionFunc(func(cfg config) config { - cfg.gatherer = gatherer - return cfg - }) -} diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 47467468d44..6f8c5ba7b21 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -17,14 +17,12 @@ package prometheus // import "go.opentelemetry.io/otel/exporters/prometheus" import ( "context" "fmt" - "net/http" "sort" "strings" "unicode" "unicode/utf8" "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promhttp" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -36,9 +34,6 @@ import ( // interface for easy instantiation with a MeterProvider. type Exporter struct { metric.Reader - Collector prometheus.Collector - - handler http.Handler } var _ metric.Reader = &Exporter{} @@ -52,10 +47,11 @@ type collector struct { func New(opts ...Option) (*Exporter, error) { cfg := newConfig(opts...) - // TODO (#????): Enable some way to configure the reader, but not change temporality. + // 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() - handler := promhttp.HandlerFor(cfg.gatherer, promhttp.HandlerOpts{}) collector := &collector{ reader: reader, } @@ -65,19 +61,12 @@ func New(opts ...Option) (*Exporter, error) { } e := &Exporter{ - Reader: reader, - Collector: collector, - - handler: handler, + Reader: reader, } return e, nil } -func (e *Exporter) ServeHTTP(w http.ResponseWriter, r *http.Request) { - e.handler.ServeHTTP(w, r) -} - // Describe implements prometheus.Collector. func (c *collector) Describe(ch chan<- *prometheus.Desc) { metrics, err := c.reader.Collect(context.TODO()) diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 38a7cfd245a..5a1a65ce58d 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -139,7 +139,7 @@ func TestPrometheusExporter(t *testing.T) { ctx := context.Background() registry := prometheus.NewRegistry() - exporter, err := New(WithGatherer(registry), WithRegisterer(registry)) + exporter, err := New(WithRegisterer(registry)) require.NoError(t, err) customBucketsView, err := view.New( From 0c412fc3183e520117c9d6ee5970ab42c51b2575 Mon Sep 17 00:00:00 2001 From: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com> Date: Thu, 13 Oct 2022 14:40:17 +0000 Subject: [PATCH 8/8] Updated comments --- exporters/prometheus/confg_test.go | 1 - exporters/prometheus/config.go | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/exporters/prometheus/confg_test.go b/exporters/prometheus/confg_test.go index 29c63a97572..91893d40f3a 100644 --- a/exporters/prometheus/confg_test.go +++ b/exporters/prometheus/confg_test.go @@ -54,7 +54,6 @@ func TestNewConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := newConfig(tt.options...) - // If no Registry is provided you should get the DefaultRegisterer and DefaultGatherer. assert.Equal(t, tt.wantRegisterer, cfg.registerer) }) } diff --git a/exporters/prometheus/config.go b/exporters/prometheus/config.go index faf09f9a188..6ee84732556 100644 --- a/exporters/prometheus/config.go +++ b/exporters/prometheus/config.go @@ -18,11 +18,12 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -// config is added here to allow for options expansion in the future. +// config contains options for the exporter. type config struct { registerer prometheus.Registerer } +// newConfig creates a validated config configured with options. func newConfig(opts ...Option) config { cfg := config{} for _, opt := range opts { @@ -36,7 +37,7 @@ func newConfig(opts ...Option) config { return cfg } -// Option may be used in the future to apply options to a Prometheus Exporter config. +// Option sets exporter option values. type Option interface { apply(config) config }