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

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

Open
xxbtwxx opened this issue Apr 5, 2024 · 2 comments
Open

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

xxbtwxx opened this issue Apr 5, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@xxbtwxx
Copy link

xxbtwxx commented Apr 5, 2024

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

@MrAlias
Copy link
Contributor

MrAlias commented Apr 5, 2024

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

20240405_072833

@xxbtwxx
Copy link
Author

xxbtwxx commented Apr 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants