Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE-5161] Fix panics if a nil context is provided to the .Start() method #5162

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/global/trace.go
Expand Up @@ -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...)
Expand Down
9 changes: 9 additions & 0 deletions internal/global/trace_test.go
Expand Up @@ -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") })
}
6 changes: 6 additions & 0 deletions trace/noop.go
Expand Up @@ -38,11 +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
}

return ContextWithSpan(ctx, span), span
}

Expand Down
5 changes: 5 additions & 0 deletions trace/noop/noop.go
Expand Up @@ -52,6 +52,11 @@ 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) {
if ctx == nil {
xxbtwxx marked this conversation as resolved.
Show resolved Hide resolved
// Prevent trace.ContextWithSpan from panicking.
ctx = context.Background()
}

span := trace.SpanFromContext(ctx)

// If the parent context contains a non-zero span context, that span
Expand Down
8 changes: 8 additions & 0 deletions trace/noop/noop_test.go
Expand Up @@ -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 }
10 changes: 10 additions & 0 deletions trace/noop_test.go
Expand Up @@ -6,6 +6,8 @@ package trace
import (
"context"
"testing"

"github.com/stretchr/testify/assert"
)

func TestNewNoopTracerProvider(t *testing.T) {
Expand Down Expand Up @@ -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") })
}