Don't panic when setting a provider to itself (#2749)

* don't panic when setting a provider to itself * check for the presence of a delegate in all tests * use t.Fatal instead of t.Error * remove unneeded return * Update CHANGELOG.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * log errors when skipping providers * check for current provider outside of the run once * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update metric/internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update metric/internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update internal/global/state_test.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
open-telemetry · Mar 31, 2022 · 625d76d · 625d76d
1 parent 60041d2
commit 625d76d
Show file tree

Hide file tree

Showing 5 changed files with 128 additions and 52 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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")
 		}
 	})
 }