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

Document that HttpClient, by default, creates a new Activity #20195

Open
noahfalk opened this issue Aug 20, 2020 · 9 comments
Open

Document that HttpClient, by default, creates a new Activity #20195

noahfalk opened this issue Aug 20, 2020 · 9 comments
Labels
doc-enhancement Improve the current content [org][type][category] Pri3

Comments

@noahfalk
Copy link
Member

Some of our developers are surprised by the behavior of DiagnosticHandler (part of HttpClient) when it creates an Activity by default. We should document this. See this thread for more background.

@mikedoug
Copy link

Please make sure to cover the complexity of the cases properly. I posted about it here, but will repeat it here with increased specificity.

THE FIRST STATEMENT WHICH MATCHES WINS:

  • If there are ZERO listeners on System.Net.Http.DiagnosticsHandler, a new Activity is created (and thus a new span).
  • If there is AT LEAST ONE listener on System.Net.Http.DiagnosticsHandler and ANY listener returns True for handling System.Net.Http.HttpRequestOut, a new Activity is created (and thus a new span).
  • If there is AT LEAST ONE listener on System.Net.Http.DiagnosticsHandler and ALL listeners returns False for handling System.Net.Http.HttpRequestOut, there is NO new Activity created (and thus NO new span).

As stated in my previous post, this is incredibly unstable because I can have my application happily working in the third case where I don't get these extraneous Activities and then I incorporate some third party SDK that listens on System.Net.Http.DiagnosticsHandler and returns true for System.Net.Http.HttpRequestOut and the behavior of my HTTP calls changes instantly to creating these intermediate Activities. The Documentation should be really clear around the brittle nature of these code paths.

@BillWagner
Copy link
Member

adding @scottaddie

Where should this fix be made?

@scottaddie
Copy link
Member

@BillWagner There's an IHttpClientFactory doc over in the ASP.NET Core docs. See https://docs.microsoft.com/aspnet/core/fundamentals/http-requests. We could add some details there; however, this detail is probably better off in a dedicated HttpClient doc. I say that because IHttpClientFactory simply creates HttpClient instances. I'd expect the proposed HttpClient doc to exist in the .NET docs.

@BillWagner
Copy link
Member

Adding @gewarren @tdykstra

Should this be in our conceptual docs for fundamentals, or in the API reference for HttpClient?

@tdykstra
Copy link
Contributor

I would put it in API reference for HttpClient.

@PRMerger6 PRMerger6 added the Pri3 label Nov 11, 2020
@J-Yen
Copy link

J-Yen commented Dec 8, 2020

I found it very confusing that running two instances (a client project that makes a call to an API service) with 'Visual Studio Multiple startup projects' or without debugger results in the creation of a new Activity (because no listeners) but when running the client with debugger separately/standalone results in not creating a new Activity (listener that returns false for System.Net.Http.HttpRequestOut).

  1. Running client project with debugger (separately/standalone):
    image

  2. Running client project and API project with 'Visual Studio Multiple startup projects' or without debugger:
    image

Also confusing that you need to create your own activity in order to make the DiagnosticHandler to create it's own System.Net.Http.HttpRequestOut activity (internal DiagnosticsHandler.IsEnabled is checking Activity.Current != null) so removing the creation of my own custom activity and depending on the activity created by DiagnosticHandler is also not an option to avoid duplicate activity creations. All these different cases exists without documentation or logging which is very difficult to understand what's happening and how I should use it.

@tdykstra
Copy link
Contributor

@scottaddie Does my suggestion to put this content in the API ref make sense to you? If so, we should transfer this issue to the API docs repo.

@scottaddie
Copy link
Member

I support Tom's suggestion to put the content in the API ref page.

@tdykstra tdykstra added doc-enhancement Improve the current content [org][type][category] and removed ⌚ Not Triaged Not triaged labels May 3, 2021
@nosalan
Copy link

nosalan commented Jul 1, 2022

I'm surprised that is going to be fixed by documentation. I would expect to be the default behavior that HttpClient propagates existing activity (span). Can't see why HttpClient creates a new span by default. There are workarounds like this but I'd prefer it to be default behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] Pri3
Projects
None yet
Development

No branches or pull requests

9 participants