From 7c4aa325a31008940f03b6dd770e3f0ba7e3a3d0 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 31 Dec 2019 11:35:04 -0800 Subject: [PATCH 1/6] Global trace forwarding implementation Initial concept. --- api/global/internal/state.go | 16 +++++++- api/global/internal/trace.go | 80 ++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 api/global/internal/trace.go diff --git a/api/global/internal/state.go b/api/global/internal/state.go index a26492d7c4a..32425b9aecb 100644 --- a/api/global/internal/state.go +++ b/api/global/internal/state.go @@ -23,6 +23,7 @@ var ( globalMeter = defaultMeterValue() delegateMeterOnce sync.Once + delegateTraceOnce sync.Once ) // TraceProvider is the internal implementation for global.TraceProvider. @@ -32,6 +33,18 @@ func TraceProvider() trace.Provider { // SetTraceProvider is the internal implementation for global.SetTraceProvider. func SetTraceProvider(tp trace.Provider) { + delegateTraceOnce.Do(func() { + current := TraceProvider() + 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 Provider, the global instance cannot be reinstalled") + } else if def, ok := current.(*traceProvider); ok { + def.setDelegate(tp) + } + + }) globalTracer.Store(traceProviderHolder{tp: tp}) } @@ -59,7 +72,7 @@ func SetMeterProvider(mp metric.Provider) { func defaultTracerValue() *atomic.Value { v := &atomic.Value{} - v.Store(traceProviderHolder{tp: trace.NoopProvider{}}) + v.Store(traceProviderHolder{tp: &traceProvider{}}) return v } @@ -74,4 +87,5 @@ func ResetForTest() { globalTracer = defaultTracerValue() globalMeter = defaultMeterValue() delegateMeterOnce = sync.Once{} + delegateTraceOnce = sync.Once{} } diff --git a/api/global/internal/trace.go b/api/global/internal/trace.go new file mode 100644 index 00000000000..db96d3c6c4d --- /dev/null +++ b/api/global/internal/trace.go @@ -0,0 +1,80 @@ +package internal + +import ( + "context" + "sync" + + "go.opentelemetry.io/otel/api/trace" +) + +type traceProvider struct { + lock sync.Mutex + tracers []*tracer + + delegate trace.Provider +} + +var _ trace.Provider = &traceProvider{} + +func (p *traceProvider) setDelegate(provider trace.Provider) { + if p.delegate != nil { + return + } + + p.lock.Lock() + defer p.lock.Unlock() + + p.delegate = provider + for _, t := range p.tracers { + t.setDelegate(provider) + } + + p.tracers = nil +} + +// Tracer creates a trace.Tracer with the given name. When a delegate is set, +// all previously returned trace.Tracers will be swapped to equivalent +// trace.Tracers created from the delegate. +func (p *traceProvider) Tracer(name string) trace.Tracer { + p.lock.Lock() + defer p.lock.Unlock() + + if p.delegate != nil { + return p.delegate.Tracer(name) + } + + t := &tracer{name: name} + p.tracers = append(p.tracers, t) + return t +} + +type tracer struct { + trace.NoopTracer + + once sync.Once + name string + + delegate trace.Tracer +} + +var _ trace.Tracer = &tracer{} + +func (t *tracer) setDelegate(provider trace.Provider) { + t.once.Do(func() { t.delegate = provider.Tracer(t.name) }) +} + +// WithSpan wraps around execution of func with delegated Tracer. +func (t *tracer) WithSpan(ctx context.Context, name string, body func(context.Context) error) error { + if t.delegate != nil { + return t.delegate.WithSpan(ctx, name, body) + } + return t.NoopTracer.WithSpan(ctx, name, body) +} + +// Start starts a span from the delegated tracer. +func (t *tracer) Start(ctx context.Context, name string, opts ...trace.StartOption) (context.Context, trace.Span) { + if t.delegate != nil { + return t.delegate.Start(ctx, name, opts...) + } + return t.NoopTracer.Start(ctx, name, opts...) +} From 69d6e78d37e69eb627f849f5db80a2fcb342fe92 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 31 Dec 2019 12:36:39 -0800 Subject: [PATCH 2/6] Add test for trace initialization with default SDK --- api/global/internal/trace_test.go | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 api/global/internal/trace_test.go diff --git a/api/global/internal/trace_test.go b/api/global/internal/trace_test.go new file mode 100644 index 00000000000..81b9a0df644 --- /dev/null +++ b/api/global/internal/trace_test.go @@ -0,0 +1,64 @@ +package internal_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/otel/api/global" + "go.opentelemetry.io/otel/api/global/internal" + export "go.opentelemetry.io/otel/sdk/export/trace" + sdktrace "go.opentelemetry.io/otel/sdk/trace" +) + +type testSpanProcesor struct { + // Names of Spans started. + spansStarted []string + // Names of Spans ended. + spansEnded []string +} + +func (t *testSpanProcesor) OnStart(s *export.SpanData) { + t.spansStarted = append(t.spansStarted, s.Name) +} + +func (t *testSpanProcesor) OnEnd(s *export.SpanData) { + t.spansEnded = append(t.spansEnded, s.Name) +} + +func (t *testSpanProcesor) Shutdown() {} + +func TestTraceDefaultSDK(t *testing.T) { + internal.ResetForTest() + + ctx := context.Background() + gtp := global.TraceProvider() + tracer1 := gtp.Tracer("pre") + _, span1 := tracer1.Start(ctx, "span1") + + tp, err := sdktrace.NewProvider(sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()})) + if err != nil { + t.Fatal(err) + } + tsp := &testSpanProcesor{} + tp.RegisterSpanProcessor(tsp) + + global.SetTraceProvider(tp) + + // This span was started before initialization, it is expected to be dropped. + span1.End() + + // The existing Tracer should have been configured to now use the configured SDK. + _, span2 := tracer1.Start(ctx, "span2") + span2.End() + + // The global trace Provider should now create Tracers that also use the newly configured SDK. + tracer2 := gtp.Tracer("post") + _, span3 := tracer2.Start(ctx, "span3") + span3.End() + + expected := []string{"pre/span2", "post/span3"} + require.Equal(t, tsp.spansStarted, expected) + require.Equal(t, tsp.spansEnded, expected) +} From 61d0b6bb401c5d1c6bccead7f00802c68eb56ea1 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 31 Dec 2019 14:44:03 -0800 Subject: [PATCH 3/6] Update test to cover the WithSpan functionality --- api/global/internal/trace_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/api/global/internal/trace_test.go b/api/global/internal/trace_test.go index 81b9a0df644..2eee87e7ed8 100644 --- a/api/global/internal/trace_test.go +++ b/api/global/internal/trace_test.go @@ -37,6 +37,11 @@ func TestTraceDefaultSDK(t *testing.T) { tracer1 := gtp.Tracer("pre") _, span1 := tracer1.Start(ctx, "span1") + // This should be dropped. + if err := tracer1.WithSpan(ctx, "withSpan1", func(context.Context) error { return nil }); err != nil { + t.Errorf("failed to wrap function with span prior to initialization: %v", err) + } + tp, err := sdktrace.NewProvider(sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sdktrace.AlwaysSample()})) if err != nil { t.Fatal(err) @@ -52,13 +57,19 @@ func TestTraceDefaultSDK(t *testing.T) { // The existing Tracer should have been configured to now use the configured SDK. _, span2 := tracer1.Start(ctx, "span2") span2.End() + if err := tracer1.WithSpan(ctx, "withSpan2", func(context.Context) error { return nil }); err != nil { + t.Errorf("failed to wrap function with span post initialization: %v", err) + } // The global trace Provider should now create Tracers that also use the newly configured SDK. tracer2 := gtp.Tracer("post") _, span3 := tracer2.Start(ctx, "span3") span3.End() + if err := tracer2.WithSpan(ctx, "withSpan3", func(context.Context) error { return nil }); err != nil { + t.Errorf("failed to wrap function with span post initialization with new tracer: %v", err) + } - expected := []string{"pre/span2", "post/span3"} + expected := []string{"pre/span2", "pre/withSpan2", "post/span3", "post/withSpan3"} require.Equal(t, tsp.spansStarted, expected) require.Equal(t, tsp.spansEnded, expected) } From d36f57068870fc4855f312b5aa93873838ab5a4e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 31 Dec 2019 14:46:08 -0800 Subject: [PATCH 4/6] Add benchmark test --- api/global/internal/benchmark_test.go | 41 +++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/api/global/internal/benchmark_test.go b/api/global/internal/benchmark_test.go index ee2edc155b7..5eaf8cf65f7 100644 --- a/api/global/internal/benchmark_test.go +++ b/api/global/internal/benchmark_test.go @@ -9,12 +9,14 @@ import ( "go.opentelemetry.io/otel/api/global/internal" "go.opentelemetry.io/otel/api/key" "go.opentelemetry.io/otel/api/metric" + "go.opentelemetry.io/otel/api/trace" export "go.opentelemetry.io/otel/sdk/export/metric" sdk "go.opentelemetry.io/otel/sdk/metric" "go.opentelemetry.io/otel/sdk/metric/aggregator/counter" "go.opentelemetry.io/otel/sdk/metric/aggregator/ddsketch" "go.opentelemetry.io/otel/sdk/metric/aggregator/gauge" "go.opentelemetry.io/otel/sdk/metric/aggregator/minmaxsumcount" + sdktrace "go.opentelemetry.io/otel/sdk/trace" ) // benchFixture is copied from sdk/metric/benchmark_test.go. @@ -100,3 +102,42 @@ func BenchmarkGlobalInt64CounterAddWithSDK(b *testing.B) { cnt.Add(ctx, 1, labs) } } + +func BenchmarkStartEndSpan(b *testing.B) { + // Comapare with BenchmarkStartEndSpan() in ../../sdk/trace/benchmark_test.go + traceBenchmark(b, func(b *testing.B) { + t := global.TraceProvider().Tracer("Benchmark StartEndSpan") + ctx := context.Background() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, span := t.Start(ctx, "/foo") + span.End() + } + }) +} + +func traceBenchmark(b *testing.B, fn func(*testing.B)) { + internal.ResetForTest() + b.Run("No SDK", func(b *testing.B) { + b.ReportAllocs() + fn(b) + }) + b.Run("Default SDK (AlwaysSample)", func(b *testing.B) { + b.ReportAllocs() + global.SetTraceProvider(traceProvider(b, sdktrace.AlwaysSample())) + fn(b) + }) + b.Run("Default SDK (NeverSample)", func(b *testing.B) { + b.ReportAllocs() + global.SetTraceProvider(traceProvider(b, sdktrace.NeverSample())) + fn(b) + }) +} + +func traceProvider(b *testing.B, sampler sdktrace.Sampler) trace.Provider { + tp, err := sdktrace.NewProvider(sdktrace.WithConfig(sdktrace.Config{DefaultSampler: sampler})) + if err != nil { + b.Fatalf("Failed to create trace provider with sampler: %v", err) + } + return tp +} From cb93e2865e98e81b1810f5cb806bda2bfc72ba8c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 2 Jan 2020 10:46:03 -0800 Subject: [PATCH 5/6] Construct NoopTracer as needed instead of embed --- api/global/internal/trace.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/api/global/internal/trace.go b/api/global/internal/trace.go index db96d3c6c4d..c97b1b2890e 100644 --- a/api/global/internal/trace.go +++ b/api/global/internal/trace.go @@ -49,8 +49,6 @@ func (p *traceProvider) Tracer(name string) trace.Tracer { } type tracer struct { - trace.NoopTracer - once sync.Once name string @@ -68,7 +66,7 @@ func (t *tracer) WithSpan(ctx context.Context, name string, body func(context.Co if t.delegate != nil { return t.delegate.WithSpan(ctx, name, body) } - return t.NoopTracer.WithSpan(ctx, name, body) + return trace.NoopTracer{}.WithSpan(ctx, name, body) } // Start starts a span from the delegated tracer. @@ -76,5 +74,5 @@ func (t *tracer) Start(ctx context.Context, name string, opts ...trace.StartOpti if t.delegate != nil { return t.delegate.Start(ctx, name, opts...) } - return t.NoopTracer.Start(ctx, name, opts...) + return trace.NoopTracer{}.Start(ctx, name, opts...) } From 7f26628d71aa36ba23192e987d0ebffbe668811a Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 2 Jan 2020 12:59:21 -0800 Subject: [PATCH 6/6] Address feedback and add documentation. --- api/global/internal/trace.go | 60 ++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/api/global/internal/trace.go b/api/global/internal/trace.go index c97b1b2890e..4ed66856186 100644 --- a/api/global/internal/trace.go +++ b/api/global/internal/trace.go @@ -1,5 +1,22 @@ package internal +/* +This file contains the forwarding implementation of the trace.Provider used as +the default global instance. Prior to initialization of an SDK, Tracers +returned by the global Provider will provide no-op functionality. This means +that all Span created prior to initialization are no-op Spans. + +Once an SDK has been initialized, all provided no-op Tracers are swapped for +Tracers provided by the SDK defined Provider. However, any Span started prior +to this initialization does not change its behavior. Meaning, the Span remains +a no-op Span. + +The implementation to track and swap Tracers locks all new Tracer creation +until the swap is complete. This assumes that this operation is not +performance-critical. If that assumption is incorrect, be sure to configure an +SDK prior to any Tracer creation. +*/ + import ( "context" "sync" @@ -7,22 +24,33 @@ import ( "go.opentelemetry.io/otel/api/trace" ) +// traceProvider is a placeholder for a configured SDK Provider. +// +// All Provider functionality is forwarded to a delegate once configured. type traceProvider struct { - lock sync.Mutex + mtx sync.Mutex tracers []*tracer delegate trace.Provider } +// Compile-time guarantee that traceProvider implements the trace.Provider interface. var _ trace.Provider = &traceProvider{} +// setDelegate configures p to delegate all Provider functionality to provider. +// +// 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. func (p *traceProvider) setDelegate(provider trace.Provider) { if p.delegate != nil { return } - p.lock.Lock() - defer p.lock.Unlock() + p.mtx.Lock() + defer p.mtx.Unlock() p.delegate = provider for _, t := range p.tracers { @@ -32,12 +60,10 @@ func (p *traceProvider) setDelegate(provider trace.Provider) { p.tracers = nil } -// Tracer creates a trace.Tracer with the given name. When a delegate is set, -// all previously returned trace.Tracers will be swapped to equivalent -// trace.Tracers created from the delegate. +// Tracer implements trace.Provider. func (p *traceProvider) Tracer(name string) trace.Tracer { - p.lock.Lock() - defer p.lock.Unlock() + p.mtx.Lock() + defer p.mtx.Unlock() if p.delegate != nil { return p.delegate.Tracer(name) @@ -48,6 +74,10 @@ func (p *traceProvider) Tracer(name string) trace.Tracer { return t } +// tracer is a placeholder for a trace.Tracer. +// +// 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 @@ -55,13 +85,22 @@ type tracer struct { delegate trace.Tracer } +// Compile-time guarantee that tracer implements the trace.Tracer interface. var _ trace.Tracer = &tracer{} +// setDelegate configures t to delegate all Tracer functionality to Tracers +// created by provider. +// +// 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. func (t *tracer) setDelegate(provider trace.Provider) { t.once.Do(func() { t.delegate = provider.Tracer(t.name) }) } -// WithSpan wraps around execution of func with delegated Tracer. +// WithSpan implements trace.Tracer by forwarding the call to t.delegate if +// set, otherwise it forwards the call to a NoopTracer. func (t *tracer) WithSpan(ctx context.Context, name string, body func(context.Context) error) error { if t.delegate != nil { return t.delegate.WithSpan(ctx, name, body) @@ -69,7 +108,8 @@ func (t *tracer) WithSpan(ctx context.Context, name string, body func(context.Co return trace.NoopTracer{}.WithSpan(ctx, name, body) } -// Start starts a span from the delegated tracer. +// 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.StartOption) (context.Context, trace.Span) { if t.delegate != nil { return t.delegate.Start(ctx, name, opts...)