diff --git a/CHANGELOG.md b/CHANGELOG.md index 5533d07f33f..2c8bdfa80b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed +- Don't panic anymore when setting a global (Tracer|Meter)Provider or TextMapPropagator to itself. (#2749) - 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) - Upgrade `go.opentelemetry.io/proto/otlp` in `go.opentelemetry.io/otel/exporters/otlp/otlptrace` from `v0.12.1` to `v0.15.0`. diff --git a/internal/global/state.go b/internal/global/state.go index a0e66918332..c7119cc4134 100644 --- a/internal/global/state.go +++ b/internal/global/state.go @@ -15,6 +15,7 @@ package global // import "go.opentelemetry.io/otel/internal/global" import ( + "errors" "sync" "sync/atomic" "testing" @@ -48,17 +49,21 @@ 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 + } + delegateTraceOnce.Do(func() { - current := TracerProvider() - if current == tp { - // Setting the provider to the prior default is nonsense, panic. - // Panic is acceptable because we are likely still early in the - // process lifetime. - panic("invalid TracerProvider, the global instance cannot be reinstalled") - } else if def, ok := current.(*tracerProvider); ok { + if def, ok := current.(*tracerProvider); ok { def.setDelegate(tp) } - }) globalTracer.Store(tracerProviderHolder{tp: tp}) } @@ -70,15 +75,21 @@ 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 + } + // For the textMapPropagator already returned by TextMapPropagator // delegate to p. delegateTextMapPropagatorOnce.Do(func() { - if current := TextMapPropagator(); current == p { - // Setting the provider to the prior default is nonsense, panic. - // Panic is acceptable because we are likely still early in the - // process lifetime. - panic("invalid TextMapPropagator, the global instance cannot be reinstalled") - } else if def, ok := current.(*textMapPropagator); ok { + if def, ok := current.(*textMapPropagator); ok { def.SetDelegate(p) } }) diff --git a/internal/global/state_test.go b/internal/global/state_test.go index 3f497cce61b..395f6c5e864 100644 --- a/internal/global/state_test.go +++ b/internal/global/state_test.go @@ -12,36 +12,91 @@ // See the License for the specific language governing permissions and // limitations under the License. -package global_test +package global import ( "testing" - "go.opentelemetry.io/otel/internal/global" + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/trace" ) -func TestResetsOfGlobalsPanic(t *testing.T) { - global.ResetForTest(t) - tests := map[string]func(){ - "SetTextMapPropagator": func() { - global.SetTextMapPropagator(global.TextMapPropagator()) - }, - "SetTracerProvider": func() { - global.SetTracerProvider(global.TracerProvider()) - }, - } - - for name, test := range tests { - shouldPanic(t, name, test) - } +func TestSetTracerProvider(t *testing.T) { + t.Run("Set With default is a noop", func(t *testing.T) { + ResetForTest(t) + SetTracerProvider(TracerProvider()) + + tp, ok := TracerProvider().(*tracerProvider) + if !ok { + t.Fatal("Global TracerProvider should be the default tracer provider") + } + + if tp.delegate != nil { + t.Fatal("tracer provider should not delegate when setting itself") + } + }) + + t.Run("First Set() should replace the delegate", func(t *testing.T) { + ResetForTest(t) + + SetTracerProvider(trace.NewNoopTracerProvider()) + + _, ok := TracerProvider().(*tracerProvider) + if ok { + t.Fatal("Global TracerProvider was not changed") + } + }) + + t.Run("Set() should delegate existing TracerProviders", func(t *testing.T) { + ResetForTest(t) + + tp := TracerProvider() + SetTracerProvider(trace.NewNoopTracerProvider()) + + ntp := tp.(*tracerProvider) + + if ntp.delegate == nil { + t.Fatal("The delegated tracer providers should have a delegate") + } + }) } -func shouldPanic(t *testing.T, name string, f func()) { - defer func() { - if r := recover(); r == nil { - t.Errorf("calling %s with default global did not panic", name) +func TestSetTextMapPropagator(t *testing.T) { + t.Run("Set With default is a noop", func(t *testing.T) { + ResetForTest(t) + SetTextMapPropagator(TextMapPropagator()) + + tmp, ok := TextMapPropagator().(*textMapPropagator) + if !ok { + t.Fatal("Global TextMapPropagator should be the default propagator") + } + + if tmp.delegate != nil { + t.Fatal("TextMapPropagator should not delegate when setting itself") + } + }) + + t.Run("First Set() should replace the delegate", func(t *testing.T) { + ResetForTest(t) + + SetTextMapPropagator(propagation.TraceContext{}) + + _, ok := TextMapPropagator().(*textMapPropagator) + if ok { + t.Fatal("Global TextMapPropagator was not changed") } - }() + }) + + t.Run("Set() should delegate existing propagators", func(t *testing.T) { + ResetForTest(t) - f() + p := TextMapPropagator() + SetTextMapPropagator(propagation.TraceContext{}) + + np := p.(*textMapPropagator) + + if np.delegate == nil { + t.Fatal("The delegated TextMapPropagators should have a delegate") + } + }) } diff --git a/metric/internal/global/state.go b/metric/internal/global/state.go index 29a67c5dbe4..0f0f0b11730 100644 --- a/metric/internal/global/state.go +++ b/metric/internal/global/state.go @@ -15,9 +15,11 @@ package global // import "go.opentelemetry.io/otel/metric/internal/global" import ( + "errors" "sync" "sync/atomic" + "go.opentelemetry.io/otel/internal/global" "go.opentelemetry.io/otel/metric" ) @@ -38,14 +40,19 @@ 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 + } + delegateMeterOnce.Do(func() { - current := MeterProvider() - if current == mp { - // Setting the provider to the prior default is nonsense, panic. - // Panic is acceptable because we are likely still early in the - // process lifetime. - panic("invalid MeterProvider, the global instance cannot be reinstalled") - } else if def, ok := current.(*meterProvider); ok { + if def, ok := current.(*meterProvider); ok { def.setDelegate(mp) } }) diff --git a/metric/internal/global/state_test.go b/metric/internal/global/state_test.go index 69cb9b917d6..bda1e57da19 100644 --- a/metric/internal/global/state_test.go +++ b/metric/internal/global/state_test.go @@ -18,8 +18,6 @@ import ( "sync" "testing" - "github.com/stretchr/testify/assert" - "go.opentelemetry.io/otel/metric/nonrecording" ) @@ -31,13 +29,18 @@ func resetGlobalMeterProvider() { func TestSetMeterProvider(t *testing.T) { t.Cleanup(resetGlobalMeterProvider) - t.Run("Set With default panics", func(t *testing.T) { + t.Run("Set With default is a noop", func(t *testing.T) { resetGlobalMeterProvider() + SetMeterProvider(MeterProvider()) - assert.Panics(t, func() { - SetMeterProvider(MeterProvider()) - }) + mp, ok := MeterProvider().(*meterProvider) + if !ok { + t.Fatal("Global MeterProvider should be the default meter provider") + } + if mp.delegate != nil { + t.Fatal("meter provider should not delegate when setting itself") + } }) t.Run("First Set() should replace the delegate", func(t *testing.T) { @@ -47,8 +50,7 @@ func TestSetMeterProvider(t *testing.T) { _, ok := MeterProvider().(*meterProvider) if ok { - t.Error("Global Meter Provider was not changed") - return + t.Fatal("Global MeterProvider was not changed") } }) @@ -62,7 +64,7 @@ func TestSetMeterProvider(t *testing.T) { dmp := mp.(*meterProvider) if dmp.delegate == nil { - t.Error("The delegated meter providers should have a delegate") + t.Fatal("The delegated meter providers should have a delegate") } }) }