Don't panic when setting a provider to itself

CHANGELOG.md

@@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Changed
+
+- Don't panic anymore when setting a (Tracer|Meter)Provider or TextMapPropagator to itself (#2749)
+
 ### Removed
 
 - Removed module the `go.opentelemetry.io/otel/sdk/export/metric`.

internal/global/state.go

@@ -50,14 +50,12 @@ func SetTracerProvider(tp trace.TracerProvider) {
 	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")
+			// Setting the provider to the prior default results in a noop. Return
+			// early.
+			return
 		} else if def, ok := current.(*tracerProvider); ok {
 			def.setDelegate(tp)
 		}
-
 	})
 	globalTracer.Store(tracerProviderHolder{tp: tp})
 }
@@ -73,10 +71,9 @@ func SetTextMapPropagator(p propagation.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")
+			// Setting the provider to the prior default results in a noop. Return
+			// early.
+			return
 		} else if def, ok := current.(*textMapPropagator); ok {
 			def.SetDelegate(p)
 		}

internal/global/state_test.go

@@ -12,36 +12,99 @@
 // 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()
-	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.Cleanup(ResetForTest)
+
+	t.Run("Set With default is a noop", func(t *testing.T) {
+		ResetForTest()
+		SetTracerProvider(TracerProvider())
+
+		tp, ok := TracerProvider().(*tracerProvider)
+		if !ok {
+			t.Fatal("Global Tracer Provider should be the default tracer provider")
+			return
+		}
+
+		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()
+
+		SetTracerProvider(trace.NewNoopTracerProvider())
+
+		_, ok := TracerProvider().(*tracerProvider)
+		if ok {
+			t.Fatal("Global Tracer Provider was not changed")
+			return
+		}
+	})
+
+	t.Run("Set() should delegate existing Tracer Providers", func(t *testing.T) {
+		ResetForTest()
+
+		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.Cleanup(ResetForTest)
+
+	t.Run("Set With default is a noop", func(t *testing.T) {
+		ResetForTest()
+		SetTextMapPropagator(TextMapPropagator())
+
+		tmp, ok := TextMapPropagator().(*textMapPropagator)
+		if !ok {
+			t.Fatal("Global TextMap Propagator should be the default propagator")
+			return
 		}
-	}()
 
-	f()
+		if tmp.delegate != nil {
+			t.Fatal("TextMap propagator should not delegate when setting itself")
+		}
+	})
+
+	t.Run("First Set() should replace the delegate", func(t *testing.T) {
+		ResetForTest()
+
+		SetTextMapPropagator(propagation.TraceContext{})
+
+		_, ok := TextMapPropagator().(*textMapPropagator)
+		if ok {
+			t.Fatal("Global TextMap Propagator was not changed")
+			return
+		}
+	})
+
+	t.Run("Set() should delegate existing propagators", func(t *testing.T) {
+		ResetForTest()
+
+		p := TextMapPropagator()
+		SetTextMapPropagator(propagation.TraceContext{})
+
+		np := p.(*textMapPropagator)
+
+		if np.delegate == nil {
+			t.Fatal("The delegated TextMap propagators should have a delegate")
+		}
+	})
 }

metric/internal/global/state.go

@@ -41,10 +41,9 @@ func SetMeterProvider(mp metric.MeterProvider) {
 	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")
+			// Setting the provider to the prior default results in a noop. Return
+			// early.
+			return
 		} else if def, ok := current.(*meterProvider); ok {
 			def.setDelegate(mp)
 		}

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,19 @@ 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 Meter Provider should be the default meter provider")
+			return
+		}
 
+		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,7 +51,7 @@ func TestSetMeterProvider(t *testing.T) {
 
 		_, ok := MeterProvider().(*meterProvider)
 		if ok {
-			t.Error("Global Meter Provider was not changed")
+			t.Fatal("Global Meter Provider was not changed")
 			return
 		}
 	})
@@ -62,7 +66,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")
 		}
 	})
 }