Skip to content

Commit

Permalink
Allow non-comparable global types (#2773)
Browse files Browse the repository at this point in the history
* 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.

* Add changes to changelog

* No lint unused field for testing
  • Loading branch information
MrAlias committed Apr 7, 2022
1 parent 376c23c commit 9838bba
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 24 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -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
Expand Down
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
25 changes: 25 additions & 0 deletions internal/global/state_test.go
Expand Up @@ -17,10 +17,18 @@ 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() //nolint:structcheck,unused // This is not called.
}

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 +67,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 +115,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
17 changes: 17 additions & 0 deletions metric/internal/global/state_test.go
Expand Up @@ -18,6 +18,9 @@ import (
"sync"
"testing"

"github.com/stretchr/testify/assert"

"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/metric/nonrecording"
)

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

type nonComparableMeterProvider struct {
metric.MeterProvider

nonComparable func() //nolint:structcheck,unused // This is not called.
}

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

Expand Down Expand Up @@ -67,4 +76,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 9838bba

Please sign in to comment.