From eb1a0115330965f6f1c7c9e44a795f2043db298d Mon Sep 17 00:00:00 2001 From: xxbtwxx Date: Fri, 5 Apr 2024 14:04:43 +0300 Subject: [PATCH 1/3] [ISSUE-5161] Fix panics if a `nil` context is provided to the `.Start()` method --- internal/global/trace.go | 5 +++++ trace/noop.go | 6 ++++++ trace/noop/noop.go | 5 +++++ 3 files changed, 16 insertions(+) diff --git a/internal/global/trace.go b/internal/global/trace.go index 596f716f40c..1f8bf0232b5 100644 --- a/internal/global/trace.go +++ b/internal/global/trace.go @@ -136,6 +136,11 @@ func (t *tracer) setDelegate(provider trace.TracerProvider) { // 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.SpanStartOption) (context.Context, trace.Span) { + if ctx == nil { + // Prevent trace.ContextWithSpan from panicking. + ctx = context.Background() + } + delegate := t.delegate.Load() if delegate != nil { return delegate.(trace.Tracer).Start(ctx, name, opts...) diff --git a/trace/noop.go b/trace/noop.go index ca20e9997ab..f2510dde7e4 100644 --- a/trace/noop.go +++ b/trace/noop.go @@ -43,6 +43,12 @@ func (t noopTracer) Start(ctx context.Context, name string, _ ...SpanStartOption // span is likely already a noopSpan, but let's be sure span = noopSpanInstance } + + if ctx == nil { + // Prevent trace.ContextWithSpan from panicking. + ctx = context.Background() + } + return ContextWithSpan(ctx, span), span } diff --git a/trace/noop/noop.go b/trace/noop/noop.go index 1dfa52c5216..6f2f8522479 100644 --- a/trace/noop/noop.go +++ b/trace/noop/noop.go @@ -54,6 +54,11 @@ type Tracer struct{ embedded.Tracer } func (t Tracer) Start(ctx context.Context, _ string, _ ...trace.SpanStartOption) (context.Context, trace.Span) { span := trace.SpanFromContext(ctx) + if ctx == nil { + // Prevent trace.ContextWithSpan from panicking. + ctx = context.Background() + } + // If the parent context contains a non-zero span context, that span // context needs to be returned as a non-recording span // (https://github.com/open-telemetry/opentelemetry-specification/blob/3a1dde966a4ce87cce5adf464359fe369741bbea/specification/trace/api.md#behavior-of-the-api-in-the-absence-of-an-installed-sdk). From 7298e2716e3f3f27b93a828c2806677c922b1a45 Mon Sep 17 00:00:00 2001 From: xxbtwxx Date: Fri, 5 Apr 2024 16:16:34 +0300 Subject: [PATCH 2/3] [ISSUE-5161] moved the safeguard to be the first op, added tests --- trace/noop.go | 10 +++++----- trace/noop/noop.go | 4 ++-- trace/noop/noop_test.go | 8 ++++++++ trace/noop_test.go | 10 ++++++++++ 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/trace/noop.go b/trace/noop.go index f2510dde7e4..06fa48be48a 100644 --- a/trace/noop.go +++ b/trace/noop.go @@ -38,17 +38,17 @@ var _ Tracer = noopTracer{} // Start carries forward a non-recording Span, if one is present in the context, otherwise it // creates a no-op Span. func (t noopTracer) Start(ctx context.Context, name string, _ ...SpanStartOption) (context.Context, Span) { + if ctx == nil { + // Prevent trace.ContextWithSpan from panicking. + ctx = context.Background() + } + span := SpanFromContext(ctx) if _, ok := span.(nonRecordingSpan); !ok { // span is likely already a noopSpan, but let's be sure span = noopSpanInstance } - if ctx == nil { - // Prevent trace.ContextWithSpan from panicking. - ctx = context.Background() - } - return ContextWithSpan(ctx, span), span } diff --git a/trace/noop/noop.go b/trace/noop/noop.go index 6f2f8522479..07809d67f02 100644 --- a/trace/noop/noop.go +++ b/trace/noop/noop.go @@ -52,13 +52,13 @@ type Tracer struct{ embedded.Tracer } // span context. If the span context in ctx is for a non-recording span, that // span instance will be returned directly. func (t Tracer) Start(ctx context.Context, _ string, _ ...trace.SpanStartOption) (context.Context, trace.Span) { - span := trace.SpanFromContext(ctx) - if ctx == nil { // Prevent trace.ContextWithSpan from panicking. ctx = context.Background() } + span := trace.SpanFromContext(ctx) + // If the parent context contains a non-zero span context, that span // context needs to be returned as a non-recording span // (https://github.com/open-telemetry/opentelemetry-specification/blob/3a1dde966a4ce87cce5adf464359fe369741bbea/specification/trace/api.md#behavior-of-the-api-in-the-absence-of-an-installed-sdk). diff --git a/trace/noop/noop_test.go b/trace/noop/noop_test.go index c228ad09918..cba206bf955 100644 --- a/trace/noop/noop_test.go +++ b/trace/noop/noop_test.go @@ -101,6 +101,14 @@ func TestTracerStartPropagatesSpanContext(t *testing.T) { assert.False(t, span.IsRecording(), "recording span returned") } +func TestStartSpanWithNilContext(t *testing.T) { + tp := NewTracerProvider() + tr := tp.Tracer("NoPanic") + + // nolint:staticcheck // no nil context, but that's the point of the test. + assert.NotPanics(t, func() { tr.Start(nil, "should-not-panic") }) +} + type recordingSpan struct{ Span } func (recordingSpan) IsRecording() bool { return true } diff --git a/trace/noop_test.go b/trace/noop_test.go index 7bf709e6de9..bd55c73829a 100644 --- a/trace/noop_test.go +++ b/trace/noop_test.go @@ -6,6 +6,8 @@ package trace import ( "context" "testing" + + "github.com/stretchr/testify/assert" ) func TestNewNoopTracerProvider(t *testing.T) { @@ -78,3 +80,11 @@ func TestNonRecordingSpanTracerStart(t *testing.T) { t.Errorf("SpanContext not carried by nonRecordingSpan. got %#v, want %#v", got, want) } } + +func TestStartSpanWithNilContext(t *testing.T) { + tp := NewNoopTracerProvider() + tr := tp.Tracer("NoPanic") + + // nolint:staticcheck // no nil context, but that's the point of the test. + assert.NotPanics(t, func() { tr.Start(nil, "should-not-panic") }) +} From 0f99c8a5c76754a601702402e2d15a4fb2abaa33 Mon Sep 17 00:00:00 2001 From: xxbtwxx Date: Mon, 8 Apr 2024 13:04:25 +0300 Subject: [PATCH 3/3] [ISSUE-5161] Added test for the internal tracer start method --- internal/global/trace_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/global/trace_test.go b/internal/global/trace_test.go index cc0fd73b526..b95b0352f51 100644 --- a/internal/global/trace_test.go +++ b/internal/global/trace_test.go @@ -235,3 +235,12 @@ func TestSpanContextPropagatedWithNonRecordingSpan(t *testing.T) { assert.Equal(t, sc, span.SpanContext()) assert.False(t, span.IsRecording()) } + +func TestStartSpanWithNilContext(t *testing.T) { + ResetForTest(t) + + tr := TracerProvider().Tracer("NoPanic") + + // nolint:staticcheck // no nil context, but that's the point of the test. + assert.NotPanics(t, func() { tr.Start(nil, "should-not-panic") }) +}