HttpClient & ASP.NET Core instrumentation invokes Propagator.Inject/Extract when it should not

[alanwest]

I just spotted this in the HTTP instrumentation

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

Lines 113 to 116 in dd4bd3e

	if (!(this.httpClientSupportsW3C && this.options.Propagator is TextMapPropagator))
	{
	this.options.Propagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageHeaderValueSetter);
	}

This seems to suggest that we do not call Inject when the version of HttpClient being used is one that already propagates W3C trace context (.NET Core 3.0+ I believe). However, recent work was done defaulting to using the CompositePropagator (which propagates both trace context and baggage). So, by default, this instrumentation always invokes Inject now which is desirable for propagating baggage but redundant for propagating W3C trace context.

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationOptions.cs

Lines 39 to 43 in dd4bd3e

	public IPropagator Propagator { get; set; } = new CompositePropagator(new IPropagator[]
	{
	new TextMapPropagator(),
	new BaggagePropagator(),
	});

@eddynaka @CodeBlanch I imagine one of you may have a bit more context here since you both recently worked on propagating baggage in #1048.

[eddynaka]

In the "past" we had a method called IsInjected and we would only inject in CompositePropagator if we didn't have injected yet. But, that was removed. so I think that is a know "issue". Don't say it's a bug, because it would inject something that it already has.

[CodeBlanch]

@alanwest Your write-up is correct. We used to let .NET handle it, but when we added support for Baggage we switched to the CompositePropagator so it will now be firing by default where it didn't before.

It looks like .NET has an existence check:
https://github.com/dotnet/runtime/blob/632cdcab09fda363e460296a77be0be32096f512/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L290

I'm not sure which fires first, us or them. Are you seeing duplicates? That would for sure be a bug. I think it's just a perf issue right now.

We could tinker with it and make the default just Baggage on .NET Core 3+? Basically to let .NET handle W3C traceparent+tracestate propagation and we can handle the extra stuff?

PS: I removed IsInjected because it was basically impossible to determine for something like Baggage which isn't necessarily present but the context overall could have still been injected. To say that another way, IsInjected made sense for TraceContext but not so much for CompositePropagator scenarios. It isn't part of the propagation spec, probably for that reason.

[alanwest]

Are you seeing duplicates?

Nope. I just saw the code and thought it looked fishy, so figured I'd ask.

I think it's just a perf issue right now.

Yes, seems like a possible minor perf issue if the DiagnosticsHandler does it work first, then the work we do is redundant.

We could tinker with it and make the default just Baggage on .NET Core 3+?

This seems like a decent thing to explore.

At a minimum, I think we should do away with the if (!(this.httpClientSupportsW3C && this.options.Propagator is TextMapPropagator)) check since it no longer really serves a purpose (and is a little confusing if you're not aware of the history 😄).

[alanwest]

@eddynaka @CodeBlanch

Similar thing in ASP.NET Core instrumentation. Though I think the effect is a little worse in this case because an additional activity is created.

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Lines 81 to 103 in 5e41c62

	if (!this.hostingSupportsW3C || !(this.options.Propagator is TextMapPropagator))
	{
	var ctx = this.options.Propagator.Extract(default, request, HttpRequestHeaderValuesGetter);
	if (ctx.ActivityContext.IsValid() && ctx.ActivityContext != activity.Context)
	{
	// Create a new activity with its parent set from the extracted context.
	// This makes the new activity as a "sibling" of the activity created by
	// Asp.Net Core.
	Activity newOne = new Activity(ActivityNameByHttpInListener);
	newOne.SetParentId(ctx.ActivityContext.TraceId, ctx.ActivityContext.SpanId, ctx.ActivityContext.TraceFlags);
	newOne.TraceStateString = ctx.ActivityContext.TraceState;
	// Starting the new activity make it the Activity.Current one.
	newOne.Start();
	activity = newOne;
	}
	if (ctx.Baggage != default)
	{
	Baggage.Current = ctx.Baggage;
	}
	}

[CodeBlanch]

@alanwest FYI it should only create that additional Activity if the extracted ActivityContext is different than the running one. That should only happen if someone uses something like B3 which ASP.NET Core won't understand. Also, if you look in the Extract implementations, I added a short-circuit that quits out if the ActivityContext is already valid:

opentelemetry-dotnet/src/OpenTelemetry.Api/Context/Propagation/TextMapPropagator.cs

Lines 48 to 52 in 768eaa3

	if (context.ActivityContext.IsValid())
	{
	// If a valid context has already been extracted, perform a noop.
	return context;
	}

Basically that was to reduce the perf hit on these double-extractions.

I'm not saying we can't improve it, just want to add all the relevant info 😄

[cijothomas]

Looks like we are doing the right things and there is no bug. Perhaps some room for minor perf optimizations. Removing bug tag.

[alanwest]

Looks like we are doing the right things and there is no bug. Perhaps some room for minor perf optimizations. Removing bug tag.

Agreed, not a bug based on conversation. I opened an issue with the spec a while back (open-telemetry/opentelemetry-specification#954) which may help facilitate any optimizations we might want to make.