opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

Noop tracer panics when calling `.Start()` with `nil` context

Open htemelski opened this issue 1 year ago • 2 comments

Description

Currently the .Start method of the noop tracer looks like this

// 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) {
	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
}

Which throws a panic if a nil context is provided

Comparing it to the .Start method of the SDK tracer

func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanStartOption) (context.Context, trace.Span) {
	config := trace.NewSpanStartConfig(options...)

	if ctx == nil {
		// Prevent trace.ContextWithSpan from panicking.
		ctx = context.Background()
	}

        ...

	return trace.ContextWithSpan(ctx, s), s
}

We can see that there's a safeguard for a nil context as agreed here

Expected behavior

Do not throw panic

htemelski avatar Apr 05 '24 10:04 htemelski

This seems like standard Go behavior. The context package explicitly states nil context should not be used:

20240405_072833

MrAlias avatar Apr 05 '24 14:04 MrAlias

This seems like standard Go behavior. The context package explicitly states nil context should not be used:

20240405_072833

This was previously discussed and agreed upon here

Here's a link to the spec, that says:

API methods MUST NOT throw unhandled exceptions when used incorrectly by end users. The API and SDK SHOULD provide safe defaults for missing or invalid arguments. For instance, a name like empty may be used if the user passes in null as the span name argument during Span construction.

htemelski avatar Apr 06 '24 09:04 htemelski