From 29787dac73147804d6a665b16846dcfbd05bed69 Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Fri, 20 Oct 2023 20:09:52 +0000 Subject: [PATCH 1/2] prometheus exporter no longer collects metrics after shutdown --- CHANGELOG.md | 4 ++++ exporters/prometheus/exporter.go | 3 +++ exporters/prometheus/exporter_test.go | 34 +++++++++++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bb7a96733e..903a2288e5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm Implementors need to update their implementations based on what they want the default behavior of the interface to be. See the "API Implementations" section of the `go.opentelemetry.io/otel/trace` package documentation for more informatoin about how to accomplish this. (#4620) +### Fixed + +- In `go.opentelemetry.op/otel/exporters/prometheus`, the exporter no longer `Collect`s metrics after `Shutdown` is invoked. (#4648) + ## [1.19.0/0.42.0/0.0.7] 2023-09-28 This release contains the first stable release of the OpenTelemetry Go [metric SDK]. diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 8973b2028e6..b525b3e14c1 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -148,6 +148,9 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { metrics := metricdata.ResourceMetrics{} err := c.reader.Collect(context.TODO(), &metrics) if err != nil { + if err == metric.ErrReaderShutdown { + return + } otel.Handle(err) if err == metric.ErrReaderNotRegistered { return diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 0f211d8318e..bd31657825e 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -16,6 +16,7 @@ package prometheus import ( "context" + "errors" "io" "os" "sync" @@ -835,3 +836,36 @@ func TestIncompatibleMeterName(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(errs)) } + +func TestShutdownExporter(t *testing.T) { + var handledError error + eh := otel.ErrorHandlerFunc(func(e error) { handledError = errors.Join(handledError, e) }) + otel.SetErrorHandler(eh) + + ctx := context.Background() + registry := prometheus.NewRegistry() + + for i := 0; i < 3; i++ { + exporter, err := New(WithRegisterer(registry)) + require.NoError(t, err) + provider := metric.NewMeterProvider( + metric.WithResource(resource.Default()), + metric.WithReader(exporter)) + meter := provider.Meter("testmeter") + cnt, err := meter.Int64Counter("foo") + require.NoError(t, err) + cnt.Add(ctx, 100) + + // verify that metrics added to a previously shutdown MeterProvider + // do not conflict with metrics added in this loop. + _, err = registry.Gather() + require.NoError(t, err) + + // Shutdown should cause future prometheus Gather() calls to no longer + // include metrics from this loop's MeterProvider. + err = provider.Shutdown(ctx) + require.NoError(t, err) + } + // ensure we aren't unnecessarily logging errors from the shutdown MeterProvider + require.NoError(t, handledError) +} From fd16ce32972197eb1725685396e1654c55f2cf6c Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Mon, 23 Oct 2023 13:38:23 +0000 Subject: [PATCH 2/2] use errors.Is --- exporters/prometheus/exporter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index b525b3e14c1..92651c38cec 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -148,11 +148,11 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { metrics := metricdata.ResourceMetrics{} err := c.reader.Collect(context.TODO(), &metrics) if err != nil { - if err == metric.ErrReaderShutdown { + if errors.Is(err, metric.ErrReaderShutdown) { return } otel.Handle(err) - if err == metric.ErrReaderNotRegistered { + if errors.Is(err, metric.ErrReaderNotRegistered) { return } }