-
Notifications
You must be signed in to change notification settings - Fork 760
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
HttpClient & ASP.NET Core instrumentation invokes Propagator.Inject/Extract when it should not #1242
Comments
In the "past" we had a method called |
@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: 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. |
Nope. I just saw the code and thought it looked fishy, so figured I'd ask.
Yes, seems like a possible minor perf issue if the
This seems like a decent thing to explore. At a minimum, I think we should do away with the |
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
|
@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
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 😄 |
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. |
I just spotted this in the HTTP instrumentation
opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
Lines 113 to 116 in dd4bd3e
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 theCompositePropagator
(which propagates both trace context and baggage). So, by default, this instrumentation always invokesInject
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
@eddynaka @CodeBlanch I imagine one of you may have a bit more context here since you both recently worked on propagating baggage in #1048.
The text was updated successfully, but these errors were encountered: