open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.37k stars 1.09k forks source link

NewNoopTracerProvider when used with WithTracerProvider resets the trace id for inner traces #4027

Open RangelReale opened 1 year ago

RangelReale commented 1 year ago

Description

NewNoopTracerProvider creates a TraceProvider that isn't really noop, it returns a non-recording span that returns a "not-valid" trace id, making inner traces creating a new traceID.

Environment

Steps To Reproduce

Given an http server:

When the http client code reaches traces.newSpan, the trace.SpanContextFromContext call will return the noop tracer, which will return false for TraceID().IsValid(), and this will create a new traceID instead of using the one from otelmux.

Expected behavior

The Noop trace provider should ne really noop, but it is changing things. Maybe some other name, and a real noop implementation should be created.

Aneurysm9 commented 1 year ago

The code you've linked is from the SDK TracerProvider which is not a no-op. The no-op provider lives in the API and will forward a non-recording span, if one exists in the context, or return a new no-op span if one does not. The non-recording span exists to ensure that context propagation can continue to work even if the application is not creating new spans of its own. The no-op provider is working as intended and is not intended to "disable" tracing in an instrumentation while other instrumentations are creating spans in the same context. If it did work the way you appear to be expecting it to then the otelhttp instrumentation would operate on the span created by the otelmux instrumentation and add duplicate or conflicting attributes.

RangelReale commented 1 year ago

The code you've linked is from the SDK TracerProvider which is not a no-op.

Yes I know, the code I linked it the code that is executed in the otelhttp.NewTransport handler, just to show the after adding a NoopTracerProvider between tracer starts, the trace id would be reset.

The non-recording span exists to ensure that context propagation can continue to work even if the application is not creating new spans of its own.

So to me it looks like the only place this noop tracer provider is useful is being the first one to cover this case, in what other use case I, a user of the (exported) function, would use it?

If it did work the way you appear to be expecting it to then the otelhttp instrumentation would operate on the span created by the otelmux instrumentation and add duplicate or conflicting attributes.

By the name Noop my expectation is that it would do nothing, and just pass on the unmodified context forward.

This is the implementation I expected (and I tested on my code), by something called "Noop":

func (t noopTracer) Start(ctx context.Context, name string, _ ...trace.SpanStartOption) (context.Context, trace.Span) {
    return ctx, &noopSpan{}
}

This implementation will send a "noop" span to otelhttp.NewHandler, while sending forward the trace id from otelmux to otelhttp.NewTransport, instead of making it be reset.

So maybe this should not be exported, and not called "noop", because in the end it does change something.

pellared commented 1 year ago

SIG meeting notes: After a discussion, it looks like this it is a bug with how the noop providers handles context propagation. Using noop provider may be a clean solution for explicitly disabling traces/metrics instrumentation when using an instrumentation library and when global providers are set.

@Aneurysm9 @MrAlias