diff --git a/CHANGELOG.md b/CHANGELOG.md index fdc74307538..da42684fc25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm span's `SpanContext`. (#1655) - Jaeger Exporter: Ensure mapping between OTEL and Jaeger span data complies with the specification. (#1626) - The `otel-collector` example now correctly flushes metric events prior to shutting down the exporter. (#1678) +- Synchronization issues in global trace delegate implementation. (#1686) ### Fixed - Do not set span status message in `SpanStatusFromHTTPStatusCode` if it can be inferred from `http.status_code`. (#1681) diff --git a/internal/global/trace.go b/internal/global/trace.go index 7b6993153fe..a23df914f93 100644 --- a/internal/global/trace.go +++ b/internal/global/trace.go @@ -34,6 +34,7 @@ SDK prior to any Tracer creation. import ( "context" "sync" + "sync/atomic" "go.opentelemetry.io/otel/internal/trace/noop" "go.opentelemetry.io/otel/trace" @@ -44,9 +45,8 @@ import ( // All TracerProvider functionality is forwarded to a delegate once // configured. type tracerProvider struct { - mtx sync.Mutex - tracers []*tracer - + mtx sync.Mutex + tracers []*tracer delegate trace.TracerProvider } @@ -60,13 +60,8 @@ var _ trace.TracerProvider = &tracerProvider{} // All Tracers provided prior to this function call are switched out to be // Tracers provided by provider. // -// Delegation only happens on the first call to this method. All subsequent -// calls result in no delegation changes. +// It is guaranteed by the caller that this happens only once. func (p *tracerProvider) setDelegate(provider trace.TracerProvider) { - if p.delegate != nil { - return - } - p.mtx.Lock() defer p.mtx.Unlock() @@ -97,11 +92,10 @@ func (p *tracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.T // All Tracer functionality is forwarded to a delegate once configured. // Otherwise, all functionality is forwarded to a NoopTracer. type tracer struct { - once sync.Once name string opts []trace.TracerOption - delegate trace.Tracer + delegate atomic.Value } // Compile-time guarantee that tracer implements the trace.Tracer interface. @@ -112,17 +106,17 @@ var _ trace.Tracer = &tracer{} // // All subsequent calls to the Tracer methods will be passed to the delegate. // -// Delegation only happens on the first call to this method. All subsequent -// calls result in no delegation changes. +// It is guaranteed by the caller that this happens only once. func (t *tracer) setDelegate(provider trace.TracerProvider) { - t.once.Do(func() { t.delegate = provider.Tracer(t.name, t.opts...) }) + t.delegate.Store(provider.Tracer(t.name, t.opts...)) } // Start implements trace.Tracer by forwarding the call to t.delegate if // set, otherwise it forwards the call to a NoopTracer. func (t *tracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) { - if t.delegate != nil { - return t.delegate.Start(ctx, name, opts...) + delegate := t.delegate.Load() + if delegate != nil { + return delegate.(trace.Tracer).Start(ctx, name, opts...) } return noop.Tracer.Start(ctx, name, opts...) } diff --git a/internal/global/trace_test.go b/internal/global/trace_test.go index 8eca6001f18..10652611943 100644 --- a/internal/global/trace_test.go +++ b/internal/global/trace_test.go @@ -16,7 +16,9 @@ package global_test import ( "context" + "sync/atomic" "testing" + "time" "github.com/stretchr/testify/assert" @@ -71,6 +73,14 @@ func (fn fnTracerProvider) Tracer(instrumentationName string, opts ...trace.Trac return fn.tracer(instrumentationName, opts...) } +type fnTracer struct { + start func(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span) +} + +func (fn fnTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span) { + return fn.start(ctx, spanName, opts...) +} + func TestTraceProviderDelegates(t *testing.T) { global.ResetForTest() @@ -91,3 +101,97 @@ func TestTraceProviderDelegates(t *testing.T) { gtp.Tracer("abc", trace.WithInstrumentationVersion("xyz")) assert.True(t, called, "expected configured TraceProvider to be called") } + +func TestTraceProviderDelegatesConcurrentSafe(t *testing.T) { + global.ResetForTest() + + // Retrieve the placeholder TracerProvider. + gtp := otel.GetTracerProvider() + + done := make(chan struct{}) + quit := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-time.After(1 * time.Millisecond): + gtp.Tracer("abc", trace.WithInstrumentationVersion("xyz")) + case <-quit: + return + } + } + }() + + // Wait for the goroutine to make some calls before installing the provider. + <-time.After(100 * time.Millisecond) + + // Configure it with a spy. + called := int32(0) + otel.SetTracerProvider(fnTracerProvider{ + tracer: func(name string, opts ...trace.TracerOption) trace.Tracer { + newVal := atomic.AddInt32(&called, 1) + assert.Equal(t, "abc", name) + assert.Equal(t, []trace.TracerOption{trace.WithInstrumentationVersion("xyz")}, opts) + if newVal == 10 { + // Signal the goroutine to finish. + close(quit) + } + return trace.NewNoopTracerProvider().Tracer("") + }, + }) + + // Wait for the go routine to finish + <-done + + assert.LessOrEqual(t, int32(10), atomic.LoadInt32(&called), "expected configured TraceProvider to be called") +} + +func TestTracerDelegatesConcurrentSafe(t *testing.T) { + global.ResetForTest() + + // Retrieve the placeholder TracerProvider. + gtp := otel.GetTracerProvider() + tracer := gtp.Tracer("abc", trace.WithInstrumentationVersion("xyz")) + + done := make(chan struct{}) + quit := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-time.After(1 * time.Millisecond): + tracer.Start(context.Background(), "name") + case <-quit: + return + } + } + }() + + // Wait for the goroutine to make some calls before installing the provider. + <-time.After(100 * time.Millisecond) + + // Configure it with a spy. + called := int32(0) + otel.SetTracerProvider(fnTracerProvider{ + tracer: func(name string, opts ...trace.TracerOption) trace.Tracer { + assert.Equal(t, "abc", name) + assert.Equal(t, []trace.TracerOption{trace.WithInstrumentationVersion("xyz")}, opts) + return fnTracer{ + start: func(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span) { + newVal := atomic.AddInt32(&called, 1) + assert.Equal(t, "name", spanName) + if newVal == 10 { + // Signal the goroutine to finish. + close(quit) + } + return trace.NewNoopTracerProvider().Tracer("").Start(ctx, spanName) + }, + } + }, + }) + + // Wait for the go routine to finish + <-done + + assert.LessOrEqual(t, int32(10), atomic.LoadInt32(&called), "expected configured TraceProvider to be called") +}