Skip to content

Commit

Permalink
Fix open-telemetry#2772: allow non-comparable global types
Browse files Browse the repository at this point in the history
The global MeterProvider, TracerProvider, and TextMapPropagator should
not panic when they are set to a non-comparable implementation of each.
  • Loading branch information
MrAlias committed Apr 7, 2022
1 parent 4bbf8d6 commit 966ea46
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 24 deletions.
38 changes: 22 additions & 16 deletions internal/global/state.go
Expand Up @@ -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() {
Expand All @@ -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
Expand Down
24 changes: 24 additions & 0 deletions internal/global/state_test.go
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) })
})
}
18 changes: 10 additions & 8 deletions metric/internal/global/state.go
Expand Up @@ -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() {
Expand Down
16 changes: 16 additions & 0 deletions metric/internal/global/state_test.go
Expand Up @@ -18,6 +18,8 @@ import (
"sync"
"testing"

"github.com/stretchr/testify/assert"
"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/metric/nonrecording"
)

Expand All @@ -26,6 +28,12 @@ func resetGlobalMeterProvider() {
delegateMeterOnce = sync.Once{}
}

type nonComparableMeterProvider struct {
metric.MeterProvider

nonComparable func()
}

func TestSetMeterProvider(t *testing.T) {
t.Cleanup(resetGlobalMeterProvider)

Expand Down Expand Up @@ -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) })
})
}

0 comments on commit 966ea46

Please sign in to comment.