search

open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.19k stars 755 forks source link

TraceContextPropagator uses ActivityContext in input PropagationContext as override instead of fallback #2807

Open Oberon00 opened 2 years ago

Oberon00 commented 2 years ago

Affected code: https://github.com/open-telemetry/opentelemetry-dotnet/blob/aad309b9445e19c437f05df150af92227e1b9eb3/src/OpenTelemetry.Api/Context/Propagation/TraceContextPropagator.cs#L53-L59

This will not extract a Context from the carrier if there already is one in the input context.

This is against the base class documentation which says:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/aad309b9445e19c437f05df150af92227e1b9eb3/src/OpenTelemetry.Api/Context/Propagation/TextMapPropagator.cs#L45-L53

The default context to be used if Extract fails.

The base class docs conform to the OTel spec (or at least my understanding of it and the Java implementation), the TraceContextPropagator implementation does not. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#extract

stevejgordon commented 3 months ago

@Oberon00 - This appears to be by design (added in #1048) since ASP.NET and ASP.NET Core automatically extract the W3C trace context when handling incoming requests, so there's no need to parse the headers a second time, adding a small amount of overhead. It, therefore, handles cases when an ActivityContext hasn't already been parsed (i.e. when ASP.NET (Core) is not involved). This seems reasonable, and I don't think the spec prohibits this. @martinjt, if @CodeBlanch agrees, I think we can close this.