From 966ea46206740cf6935179d2cf9eab43af301aab Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 7 Apr 2022 08:17:04 -0700 Subject: [PATCH 1/3] Fix #2772: allow non-comparable global types The global MeterProvider, TracerProvider, and TextMapPropagator should not panic when they are set to a non-comparable implementation of each. --- internal/global/state.go | 38 ++++++++++++++++------------ internal/global/state_test.go | 24 ++++++++++++++++++ metric/internal/global/state.go | 18 +++++++------ metric/internal/global/state_test.go | 16 ++++++++++++ 4 files changed, 72 insertions(+), 24 deletions(-) diff --git a/internal/global/state.go b/internal/global/state.go index c7119cc4134..837d3c6c45e 100644 --- a/internal/global/state.go +++ b/internal/global/state.go @@ -50,14 +50,17 @@ func TracerProvider() trace.TracerProvider { // SetTracerProvider is the internal implementation for global.SetTracerProvider. func SetTracerProvider(tp trace.TracerProvider) { current := TracerProvider() - if current == tp { - // Setting the provider to the prior default results in a noop. Return - // early. - Error( - errors.New("no delegate configured in tracer provider"), - "Setting tracer provider to it's current value. No delegate will be configured", - ) - return + + if _, cOk := current.(*tracerProvider); cOk { + if _, tpOk := tp.(*tracerProvider); tpOk && current == tp { + // Do not assign the default delegating TracerProvider to delegate + // to itself. + Error( + errors.New("no delegate configured in tracer provider"), + "Setting tracer provider to it's current value. No delegate will be configured", + ) + return + } } delegateTraceOnce.Do(func() { @@ -76,14 +79,17 @@ func TextMapPropagator() propagation.TextMapPropagator { // SetTextMapPropagator is the internal implementation for global.SetTextMapPropagator. func SetTextMapPropagator(p propagation.TextMapPropagator) { current := TextMapPropagator() - if current == p { - // Setting the provider to the prior default results in a noop. Return - // early. - Error( - errors.New("no delegate configured in text map propagator"), - "Setting text map propagator to it's current value. No delegate will be configured", - ) - return + + if _, cOk := current.(*textMapPropagator); cOk { + if _, pOk := p.(*textMapPropagator); pOk && current == p { + // Do not assign the default delegating TextMapPropagator to + // delegate to itself. + Error( + errors.New("no delegate configured in text map propagator"), + "Setting text map propagator to it's current value. No delegate will be configured", + ) + return + } } // For the textMapPropagator already returned by TextMapPropagator diff --git a/internal/global/state_test.go b/internal/global/state_test.go index 395f6c5e864..ebdf47ff680 100644 --- a/internal/global/state_test.go +++ b/internal/global/state_test.go @@ -17,10 +17,17 @@ package global import ( "testing" + "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) +type nonComparableTracerProvider struct { + trace.TracerProvider + + nonComparable func() +} + func TestSetTracerProvider(t *testing.T) { t.Run("Set With default is a noop", func(t *testing.T) { ResetForTest(t) @@ -59,6 +66,14 @@ func TestSetTracerProvider(t *testing.T) { t.Fatal("The delegated tracer providers should have a delegate") } }) + + t.Run("non-comparable types should not panic", func(t *testing.T) { + ResetForTest(t) + + tp := nonComparableTracerProvider{} + SetTracerProvider(tp) + assert.NotPanics(t, func() { SetTracerProvider(tp) }) + }) } func TestSetTextMapPropagator(t *testing.T) { @@ -99,4 +114,13 @@ func TestSetTextMapPropagator(t *testing.T) { t.Fatal("The delegated TextMapPropagators should have a delegate") } }) + + t.Run("non-comparable types should not panic", func(t *testing.T) { + ResetForTest(t) + + // A composite TextMapPropagator is not comparable. + prop := propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}) + SetTextMapPropagator(prop) + assert.NotPanics(t, func() { SetTextMapPropagator(prop) }) + }) } diff --git a/metric/internal/global/state.go b/metric/internal/global/state.go index 0f0f0b11730..47c0d787d8a 100644 --- a/metric/internal/global/state.go +++ b/metric/internal/global/state.go @@ -41,14 +41,16 @@ func MeterProvider() metric.MeterProvider { // SetMeterProvider is the internal implementation for global.SetMeterProvider. func SetMeterProvider(mp metric.MeterProvider) { current := MeterProvider() - if current == mp { - // Setting the provider to the prior default results in a noop. Return - // early. - global.Error( - errors.New("no delegate configured in meter provider"), - "Setting meter provider to it's current value. No delegate will be configured", - ) - return + if _, cOk := current.(*meterProvider); cOk { + if _, mpOk := mp.(*meterProvider); mpOk && current == mp { + // Do not assign the default delegating MeterProvider to delegate + // to itself. + global.Error( + errors.New("no delegate configured in meter provider"), + "Setting meter provider to it's current value. No delegate will be configured", + ) + return + } } delegateMeterOnce.Do(func() { diff --git a/metric/internal/global/state_test.go b/metric/internal/global/state_test.go index bda1e57da19..51145e5e29c 100644 --- a/metric/internal/global/state_test.go +++ b/metric/internal/global/state_test.go @@ -18,6 +18,8 @@ import ( "sync" "testing" + "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/nonrecording" ) @@ -26,6 +28,12 @@ func resetGlobalMeterProvider() { delegateMeterOnce = sync.Once{} } +type nonComparableMeterProvider struct { + metric.MeterProvider + + nonComparable func() +} + func TestSetMeterProvider(t *testing.T) { t.Cleanup(resetGlobalMeterProvider) @@ -67,4 +75,12 @@ func TestSetMeterProvider(t *testing.T) { t.Fatal("The delegated meter providers should have a delegate") } }) + + t.Run("non-comparable types should not panic", func(t *testing.T) { + resetGlobalMeterProvider() + + mp := nonComparableMeterProvider{} + SetMeterProvider(mp) + assert.NotPanics(t, func() { SetMeterProvider(mp) }) + }) } From 9b5f798178adfce4c84c8b0732a7eecfcf2dbfc7 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 7 Apr 2022 08:23:02 -0700 Subject: [PATCH 2/3] Add changes to changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d0cb191431..7339afa2f05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Upgrade `go.opentelemetry.io/proto/otlp` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric` from `v0.12.1` to `v0.15.0`. This replaces the use of the now deprecated `InstrumentationLibrary` and `InstrumentationLibraryMetrics` types and fields in the proto library with the equivalent `InstrumentationScope` and `ScopeMetrics`. (#2748) +### Fixed + +- Allow non-comparable global `MeterProvider`, `TracerProvider`, and `TextMapPropagator` types to be set. (#2772, #2773) + ## [1.6.2] - 2022-04-06 ### Changed From cff9da6d440084f1879e09b1d505e3906d92eeba Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 7 Apr 2022 08:29:45 -0700 Subject: [PATCH 3/3] No lint unused field for testing --- internal/global/state_test.go | 3 ++- metric/internal/global/state_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/global/state_test.go b/internal/global/state_test.go index ebdf47ff680..f26f9d8c5a4 100644 --- a/internal/global/state_test.go +++ b/internal/global/state_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" ) @@ -25,7 +26,7 @@ import ( type nonComparableTracerProvider struct { trace.TracerProvider - nonComparable func() + nonComparable func() //nolint:structcheck,unused // This is not called. } func TestSetTracerProvider(t *testing.T) { diff --git a/metric/internal/global/state_test.go b/metric/internal/global/state_test.go index 51145e5e29c..28b9ea1645a 100644 --- a/metric/internal/global/state_test.go +++ b/metric/internal/global/state_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/nonrecording" ) @@ -31,7 +32,7 @@ func resetGlobalMeterProvider() { type nonComparableMeterProvider struct { metric.MeterProvider - nonComparable func() + nonComparable func() //nolint:structcheck,unused // This is not called. } func TestSetMeterProvider(t *testing.T) {