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

Add an option to HttpClient's default DiagnosticHandler so that it does not create an Activity #41072

Closed
noahfalk opened this issue Aug 20, 2020 · 12 comments · Fixed by #55392
Closed
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@noahfalk
Copy link
Member

Some of our developers are very surprised that DiagnosticHandler creates an Activity by default and they would appreciate a way that they can opt-out of this behavior. See this thread for more background.

@noahfalk noahfalk added this to the 6.0.0 milestone Aug 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Aug 20, 2020
@ghost
Copy link

ghost commented Aug 20, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@karelz
Copy link
Member

karelz commented Aug 20, 2020

Seems to be related to #35337 and to #31261

@noahfalk is the key problem here that the activity breaks parent activities? Or something else?
cc @MihaZupan

@karelz karelz added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Aug 20, 2020
@scalablecory
Copy link
Contributor

It seems like we should expose DiagnosticsHandler as an API and could have settings on it.

@mikedoug
Copy link

@karelz There are a couple of issues for me. First, it is completely unexpected and the behavior of the code changes in an unstable manner. See my breakdown of this issue for more information on that.

Second, the automatic extra Activity on making an HTTP call should be the exception and not the rule. If you look at any documentation and any guides/blog posts/etc on using W3C Trace Context (including information created by Microsoft itself) the concept is that the context starts with some process, and a new span is created as you cross service lines (by the called service -- not the caller). So if I have Service A creating the context calling into Service B one time I should have two total SpanIds. It was altogether unexpected to have an intermediate Activity (causing a new span) being created by the HttpClient (through the DiagnosticDelegate) -- I was met with an inability to trace my distributed calls because none of my down-stream ParentIds referenced any previous SpanIds on the call.

I'd argue that it should be the EXCEPTIONAL case that people would want HttpClient to generate this intermediate/additional span -- perhaps if I'm doing an operation that is going to make thousands of calls I'd want to do that so that I have a distinct SpanId on the caller side to match to each individual call on the down-stream side. But, to be honest, if I want that then I should be the one who creates an Activity before making such a call. It should be an explicit decision to make a new child activity (causing a new span); it should not be forced upon me.

Primarily I'm thinking about what this will look like inside of a tracing program. For every HTTP call I will have the originating span for the trace and then I'll have 2 spans for every HTTP call made (one on calling side and one on caller side). I guess it is possible that whomever added this code did so thinking that they were creating the single child activity context for this call -- not realizing that the service it is calling will necessarily by the standards of tracing create a new span on the trace (through its own activity context in the .NET world). Not only am I going to have nearly 2x the number of spans in my log which means that these extraneous spans double the cost of logging services which are often volume base.

Here's a visual example of the current .NET Tracing (Trace, Span, Parent) of one operation calling three separate APIs.

  • TraceId-1, Span-Id1, 00000000 (the initiating span)
  • -- here the initiating service would log with this information.
  • TraceId-1, Span-Id2, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id3, Span-Id2 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.
  • TraceId-1, Span-Id4, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id5, Span-Id4 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.
  • TraceId-1, Span-Id6, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id7, Span-Id6 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.

You notice that Span-Id2, Span-Id4, and Span-Id6 are these intermediate spans that don't offer any value to me. Especially since they are inside the bowels of the code I don't know that I can even really log the Span information for this "instance" of a remote call. This is why I say it would better to allow ME to make that decision -- if I need to have that one-to-one tracing then I should create the Activity, log information I need for this instance, and then make the HTTP call.

I hope this information helps you further. I look forward to any fixes and/or changes you make to this area. For now the hacky work around is helping me maintain my sanity.

@gislikonrad
Copy link

@mikedoug Great breakdown of the issue. Let's hope we get a fix or official workaround soon. The hack makes me cringe, but it's necessary for us to be able to trace across service boundaries.

@jonhwong
Copy link

jonhwong commented Sep 10, 2020

There is a similar pattern in this example from OpenTelemetry:https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/examples/MicroserviceExample/Utils/Messaging/MessageSender.cs where a WebApi endpoint is sending a message to RabbitMQ and creating an Activity in the process. Two instances in reputable places makes me think I'm missing something. Does anyone know of documentation that supports this?

Edit: Found it: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#spankind. Though I still can't find the rationale for this.

@Trolldemorted
Copy link

Since dotnet/aspnetcore#27846 (comment) was closed: How can I tell a HttpClient to not send a traceparent header? I didn't manage to find out how to completely disable them.

@s-bauer
Copy link

s-bauer commented Nov 22, 2020

Same goes for me, it's quite frustrating that there is no opt-out for such a "feature"!

@JustinDroege
Copy link

@karelz There are a couple of issues for me. First, it is completely unexpected and the behavior of the code changes in an unstable manner. See my breakdown of this issue for more information on that.

Second, the automatic extra Activity on making an HTTP call should be the exception and not the rule. If you look at any documentation and any guides/blog posts/etc on using W3C Trace Context (including information created by Microsoft itself) the concept is that the context starts with some process, and a new span is created as you cross service lines (by the called service -- not the caller). So if I have Service A creating the context calling into Service B one time I should have two total SpanIds. It was altogether unexpected to have an intermediate Activity (causing a new span) being created by the HttpClient (through the DiagnosticDelegate) -- I was met with an inability to trace my distributed calls because none of my down-stream ParentIds referenced any previous SpanIds on the call.

I'd argue that it should be the EXCEPTIONAL case that people would want HttpClient to generate this intermediate/additional span -- perhaps if I'm doing an operation that is going to make thousands of calls I'd want to do that so that I have a distinct SpanId on the caller side to match to each individual call on the down-stream side. But, to be honest, if I want that then I should be the one who creates an Activity before making such a call. It should be an explicit decision to make a new child activity (causing a new span); it should not be forced upon me.

Primarily I'm thinking about what this will look like inside of a tracing program. For every HTTP call I will have the originating span for the trace and then I'll have 2 spans for every HTTP call made (one on calling side and one on caller side). I guess it is possible that whomever added this code did so thinking that they were creating the single child activity context for this call -- not realizing that the service it is calling will necessarily by the standards of tracing create a new span on the trace (through its own activity context in the .NET world). Not only am I going to have nearly 2x the number of spans in my log which means that these extraneous spans double the cost of logging services which are often volume base.

Here's a visual example of the current .NET Tracing (Trace, Span, Parent) of one operation calling three separate APIs.

  • TraceId-1, Span-Id1, 00000000 (the initiating span)
  • -- here the initiating service would log with this information.
  • TraceId-1, Span-Id2, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id3, Span-Id2 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.
  • TraceId-1, Span-Id4, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id5, Span-Id4 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.
  • TraceId-1, Span-Id6, Span-Id1 (the HttpClient span for the outbound HTTP call)
  • TraceId-1, Span-Id7, Span-Id6 (the span created by the down-stream API service)
  • -- here the downstream service would log with this information.

You notice that Span-Id2, Span-Id4, and Span-Id6 are these intermediate spans that don't offer any value to me. Especially since they are inside the bowels of the code I don't know that I can even really log the Span information for this "instance" of a remote call. This is why I say it would better to allow ME to make that decision -- if I need to have that one-to-one tracing then I should create the Activity, log information I need for this instance, and then make the HTTP call.

I hope this information helps you further. I look forward to any fixes and/or changes you make to this area. For now the hacky work around is helping me maintain my sanity.

Is there any fixes meanwhile? I have exactly the same problem, that two childrens have not the same parent id because the http client generates new spans and this is makes more difficult to generate a dependecy graph.

@gislikonrad
Copy link

gislikonrad commented Mar 16, 2021

@JustinDroege My original issue had a hack/workaround. #31862

@MihaZupan
Copy link
Member

MihaZupan commented Jul 13, 2021

There is some overlap with #35337 here. If you don't want context propagation (headers being injected), see:

For the actual problem discussed in this issue (DiagnosticHandler always creating an Activity):

Note that the propagator used is also configurable on a per-SocketsHttpHandler instance level (#55556).

@noahfalk
Copy link
Member Author

In addition to the options that @MihaZupan mentioned above there is also a new ActivityListener implemented in .NET 5 which can be used to observe all Activities that are created, including the one from HttpClient. This page includes example code and logging output.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

Successfully merging a pull request may close this issue.