From 6b669b7581bd9424478f146b1aab1a5a3215e14e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 25 Feb 2021 16:43:56 -0800 Subject: [PATCH 1/3] Add TracerProvider tests to oteltest harness --- internal/matchers/expectation.go | 6 ++++ oteltest/harness.go | 51 ++++++++++++++++++++++++++++++++ sdk/trace/trace_test.go | 13 ++++---- 3 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/matchers/expectation.go b/internal/matchers/expectation.go index 806d787fe67..625ee017bfe 100644 --- a/internal/matchers/expectation.go +++ b/internal/matchers/expectation.go @@ -83,6 +83,12 @@ func (e *Expectation) ToBeFalse() { } } +func (e *Expectation) NotToPanic() { + if actual := recover(); actual != nil { + e.fail(fmt.Sprintf("Expected panic\n\t%v\nto have not been raised", actual)) + } +} + func (e *Expectation) ToSucceed() { switch actual := e.actual.(type) { case error: diff --git a/oteltest/harness.go b/oteltest/harness.go index 428094dcdab..e5f7c6494bd 100644 --- a/oteltest/harness.go +++ b/oteltest/harness.go @@ -16,6 +16,7 @@ package oteltest // import "go.opentelemetry.io/otel/oteltest" import ( "context" + "fmt" "sync" "testing" "time" @@ -39,6 +40,56 @@ func NewHarness(t *testing.T) *Harness { } } +// TestTracer runs validation tests for an implementation of the OpenTelemetry +// TracerProvider API. +func (h *Harness) TestTracerProvider(subjectFactory func() trace.TracerProvider) { + h.t.Run("#Start", func(t *testing.T) { + t.Run("allow creating an arbitrary number of TracerProvider instances", func(t *testing.T) { + t.Parallel() + + e := matchers.NewExpecter(t) + + tp1 := subjectFactory() + tp2 := subjectFactory() + + e.Expect(tp1).NotToEqual(tp2) + }) + t.Run("all methods are safe to be called concurrently", func(t *testing.T) { + t.Parallel() + + runner := func(tp trace.TracerProvider) <-chan struct{} { + done := make(chan struct{}) + go func(tp trace.TracerProvider) { + var wg sync.WaitGroup + for i := 0; i < 20; i++ { + wg.Add(1) + go func(name, version string) { + _ = tp.Tracer(name, trace.WithInstrumentationVersion(version)) + wg.Done() + }(fmt.Sprintf("tracer %d", i%5), fmt.Sprintf("%d", i)) + } + wg.Wait() + done <- struct{}{} + }(tp) + return done + } + + matchers.NewExpecter(t).Expect(func() { + // Run with multiple TracerProvider to ensure they encapsulate + // their own Tracers. + tp1 := subjectFactory() + tp2 := subjectFactory() + + done1 := runner(tp1) + done2 := runner(tp2) + + <-done1 + <-done2 + }).NotToPanic() + }) + }) +} + // TestTracer runs validation tests for an implementation of the OpenTelemetry // Tracer API. func (h *Harness) TestTracer(subjectFactory func() trace.Tracer) { diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index bf04c2f2c84..93dde22147c 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -68,13 +68,16 @@ func init() { } func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) { - tp := NewTracerProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) harness := oteltest.NewHarness(t) - subjectFactory := func() trace.Tracer { - return tp.Tracer("") - } - harness.TestTracer(subjectFactory) + harness.TestTracerProvider(func() trace.TracerProvider { + return NewTracerProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) + }) + + tp := NewTracerProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) + harness.TestTracer(func() trace.Tracer { + return tp.Tracer("") + }) } type testExporter struct { From 8ce7ec8beab716e1d805c2637de2acb4ebc911b5 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 25 Feb 2021 16:49:56 -0800 Subject: [PATCH 2/3] Update Tracer method docs --- sdk/trace/provider.go | 9 +++++++-- trace/trace.go | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go index 64012c86670..6955d0e6c6f 100644 --- a/sdk/trace/provider.go +++ b/sdk/trace/provider.go @@ -85,8 +85,13 @@ func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider { return tp } -// Tracer with the given name. If a tracer for the given name does not exist, -// it is created first. If the name is empty, DefaultTracerName is used. +// Tracer returns a Tracer with the given name and options. If a Tracer for +// the given name and options does not exist it is created first, otherwise +// the existing matching Tracer is returned. +// +// If the name is empty, DefaultTracerName is used. +// +// This method is safe to be called concurrently. func (p *TracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.Tracer { c := trace.NewTracerConfig(opts...) diff --git a/trace/trace.go b/trace/trace.go index 9b390082299..70606387ac9 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -558,5 +558,7 @@ type TracerProvider interface { // only if that code provides built-in instrumentation. If the // instrumentationName is empty, then a implementation defined default // name will be used instead. + // + // This method must be concurrency safe. Tracer(instrumentationName string, opts ...TracerOption) Tracer } From 2820050446d0a2aee1ceb674b5c69c50f5f58956 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 25 Feb 2021 16:51:34 -0800 Subject: [PATCH 3/3] Fix grammar --- sdk/trace/provider.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/trace/provider.go b/sdk/trace/provider.go index 6955d0e6c6f..135e79ed02c 100644 --- a/sdk/trace/provider.go +++ b/sdk/trace/provider.go @@ -86,10 +86,10 @@ func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider { } // Tracer returns a Tracer with the given name and options. If a Tracer for -// the given name and options does not exist it is created first, otherwise -// the existing matching Tracer is returned. +// the given name and options does not exist it is created, otherwise the +// existing Tracer is returned. // -// If the name is empty, DefaultTracerName is used. +// If name is empty, DefaultTracerName is used instead. // // This method is safe to be called concurrently. func (p *TracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.Tracer {