dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

HttpClient distributed tracing tag enrichment #96263

Open JamesNK opened 11 months ago

JamesNK commented 11 months ago

Description

HttpClient creates an Activity for each HTTP request. The activity is used to create a span in OTel distributed tracing.

OTel spans have attributes and HttpClient automatically adds information about the HTTP request to the span. Additional information can be added by calling Activity.AddTag. However, there isn't an easy way to enrich the span with custom information for an HTTP request. Usually the activity is accessed using Activity.Current, an async local, but HttpClient creates the activity in DiaganosticsHandler which is automatically added as the last handler in the pipeline. It's not possible to add your own handler to work with Activity.Current because it's created afterwards.

An alternative approach is to use a diagnostics listener and listen to System.Net.Http.HttpRequestOut.Start. It is called after the activity has been created. The problem with this approach:

  1. It's static across the app. Can't localize enrichment to one HttpClient or request.
  2. Accessing the HttpRequestMessage in the raised listener callback requires reflection.
  3. It's hard and inaccessible.

A scenario where this information is useful is this Aspire UI enhancement: https://github.com/dotnet/aspire/pull/1061

We want to identify the Browser Link HTTP request an ASP.NET Core app makes to Visual Studio. It would be great if we could enrich the activity to have an attribute to identify the call was for Browser Link.

Because that's not possible we were forced to look at the request URL to infer the request is for Browser Link.

Reproduction Steps

Try to enrich Activity for a HttpClient.

Expected behavior

There is an easy-to-use API for enriching the HttpClient. The API:

  1. Can be configured per HttpClient, or ideally, per HttpRequestMessage.
  2. Provides the Activity for HttpClient. Providing the activity is better than making people use Activity.Current, because there might be a child activity created, and using Activity.Current forces callers to check the name for each activity in the hierarchy until they find the one for the HttpClient.

Maybe something like:

var httpRequest = new HttpRequestMessage("https://contoso.com");
HttpActivityEnrichmentContext.AddCallback(httpRequest, context =>
{
    context.Activity.AddTag("http.is_browser_link", true);
});

Actual behavior

No easy way to enrich activity for HttpClient.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

ghost commented 11 months ago

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

Issue Details
### Description HttpClient creates an Activity for each HTTP request. The activity is used to create a span in OTel distributed tracing. OTel spans have attributes and HttpClient automatically adds information about the HTTP request to the span. Additional information can be added by calling [Activity.AddTag](https://github.com/dotnet/runtime/blob/0cf461b302f58c7add3f6dc405873fb2212b513f/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L461-L479). However, there isn't an easy way to enrich the span with custom information for an HTTP request. Usually the activity is accessed using `Activity.Current`, an async local, but HttpClient [creates the activity in DiaganosticsHandler](https://github.com/dotnet/runtime/blob/0cf461b302f58c7add3f6dc405873fb2212b513f/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L112) which is automatically added as the last handler in the pipeline. It's not possible to add your own handler to work with `Activity.Current` because it's created afterwards. An alternative approach is to use a diagnostics listener and listen to `System.Net.Http.HttpRequestOut.Start`. It is called after the activity has been created. The problem with this approach: 1. It's static across the app. Can't localize enrichment to one HttpClient or request. 2. Accessing the `HttpRequestMessage` in the raised listener callback requires reflection. 3. It's hard and inaccessible. --- A scenario where this information is useful is this Aspire UI enhancement: https://github.com/dotnet/aspire/pull/1061 We want to identify the Browser Link HTTP request an ASP.NET Core app makes to Visual Studio. It would be great if we could enrich the activity to have an attribute to identify the call was for Browser Link. Because that's not possible we were forced to look at the request URL to infer the request is for Browser Link. ### Reproduction Steps Try to enrich Activity for a HttpClient. ### Expected behavior There is an easy-to-use API for enriching the HttpClient. The API: 1. Can be configured per HttpClient, or ideally, per HttpRequestMessage. 2. Provides the Activity for HttpClient. Providing the activity is better than making people use Activity.Current, because there might be a child activity created, and using Activity.Current forces callers to check the name for each activity in the hierarchy until they find the one for the HttpClient. Maybe something like: ```cs var httpRequest = new HttpRequestMessage("https://contoso.com"); HttpActivityEnrichmentContext.AddCallback(httpRequest, context => { context.Activity.AddTag("http.is_browser_link", true); }); ``` ### Actual behavior No easy way to enrich activity for HttpClient. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: JamesNK
Assignees: -
Labels: `area-System.Net.Http`
Milestone: -
MihaZupan commented 10 months ago

How commonly do you think users would want to make use of such APIs?

There were also hints in the past that we could consider exposing the internal handler pipeline more, such that you could insert handlers at any point (e.g. behind the diagnostics handler).

For your use case, would a workaround like this be acceptable (yay AsyncLocal 😄)?

BrowserLinkHandler.cs ```c# public sealed class BrowserLinkHandler : DelegatingHandler { private static readonly AsyncLocal s_isBrowserLinkRequest = new(); static BrowserLinkHandler() { ActivitySource.AddActivityListener(new ActivityListener { ShouldListenTo = static source => source.Name == "System.Net.Http", ActivityStarted = static activity => { if (s_isBrowserLinkRequest.Value && activity.OperationName == "System.Net.Http.HttpRequestOut") { activity.AddTag("http.is_browser_link", true); } }, // Sample is only needed if there are no other ActivityListeners around Sample = static (ref ActivityCreationOptions options) => { if (s_isBrowserLinkRequest.Value && options.Name == "System.Net.Http.HttpRequestOut") { return ActivitySamplingResult.AllDataAndRecorded; } return ActivitySamplingResult.None; } }); } public BrowserLinkHandler(HttpMessageHandler innerHandler) : base(innerHandler) { } protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) { s_isBrowserLinkRequest.Value = true; return base.Send(request, cancellationToken); } protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { s_isBrowserLinkRequest.Value = true; return base.SendAsync(request, cancellationToken); } } ```
JamesNK commented 10 months ago

For your use case, would a workaround like this be acceptable (yay AsyncLocal 😄)?

As a hack, it might be ok. But I think the goal is to make something generally usable.

How commonly do you think users would want to make use of such APIs?

I don't think it was commonly wanted in the past, but I expect the desire to have to add this kind of feature will grow now that tools like Aspire make telemetry much more accessible.

antonfirsov commented 10 months ago

Triage: we should track and possibly implement this for 9.0.

antonfirsov commented 5 months ago

or ideally, per HttpRequestMessage

@JamesNK how important is to have this per-request? Are there any use-cases that strictly need this? Also, how important is the ability to register multiple callbacks? What would be the downside on if we would try to get away with a single HttpClientHandler.ActivityEnrichmentCallback?

We invested a lot of energy to have sophisticated, fine-grained metrics enrichment feature in .NET 8, yet it looks like HttpMetricsEnrichmentContext has virtually no users. This makes me question if we made the right design choices back in the days.

Maybe we need a simpler, more coarse-grained, but more discoverable API for both Metrics and Activity enrichment?

julealgon commented 5 months ago

Also, how important is the ability to register multiple callbacks? What would be the downside on if we would try to get away with a single HttpClientHandler.ActivityEnrichmentCallback?

Why not just make it IObservable instead which naturally supports N handlers? Is there even a reason to expose "callbacks" in modern code if there is an abstraction that supports event processing without the downfall of original events?

I keep seeing these proposals with manual Func/Action "callbacks" and I just don't understand why one would go that route instead of just leveraging IObservable.

antonfirsov commented 5 months ago

@julealgon one can implement a callback inline, while implementing IObserver<T> needs a separate class. People like the simplicity of callbacks.

More imporantly, subscribers don't fit the IObserver<T> model. According to the docs , IObserver<T>

Provides a mechanism for receiving push-based notifications.

Enricment callbacks are not "push-based notifications", they are hooks to modify built-in telemetry behavior. It's not clear what would be the role of OnCompleted (why would anyone implement it in practice?) and when an error occurrs, it's insufficient to observe the exception only, the enrichment code also needs to see the HttpRequestMessage causing the error.

aeb-dev commented 3 weeks ago

It seems like this is not gonna make it to .net 9. Is there any workaround?

CodeBlanch commented 3 days ago

@aeb-dev If you are using OpenTelemetry .NET SDK, you could look at https://github.com/Macross-Software/core/blob/develop/ClassLibraries/Macross.OpenTelemetry.Extensions/README.md#opentelemetry-activity-enrichment-scope.

Something I did a few years ago to enable this type of scenario. Basically uses an AsyncLocal to store the state and registers an OTel processor to invoke the callback when the Activity\span is ended.

Should work for HttpClient or any child operation type of thing.