[ISSUE-5161] moved the safeguard to be the first op, added tests

Author: htemelski

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
 }
 

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).

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 }

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") })
+}