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

HttpClient distributed tracing tag enrichment #96263

Open
JamesNK opened this issue Dec 22, 2023 · 12 comments
Open

HttpClient distributed tracing tag enrichment #96263

JamesNK opened this issue Dec 22, 2023 · 12 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Dec 22, 2023

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: dotnet/aspire#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
Copy link

ghost commented Dec 22, 2023

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. 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: dotnet/aspire#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

Author: JamesNK
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 22, 2023
@MihaZupan
Copy link
Member

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
public sealed class BrowserLinkHandler : DelegatingHandler
{
    private static readonly AsyncLocal<bool> 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<ActivityContext> 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<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        s_isBrowserLinkRequest.Value = true;
        return base.SendAsync(request, cancellationToken);
    }
}

@JamesNK
Copy link
Member Author

JamesNK commented Jan 8, 2024

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.

@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed enhancement Product code improvement that does NOT require public API changes/additions labels Jan 9, 2024
@antonfirsov
Copy link
Member

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

@antonfirsov antonfirsov removed the untriaged New issue has not been triaged by the area owner label Jan 10, 2024
@antonfirsov antonfirsov added this to the 9.0.0 milestone Jan 10, 2024
@antonfirsov antonfirsov self-assigned this May 21, 2024
@antonfirsov
Copy link
Member

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
Copy link

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
Copy link
Member

antonfirsov commented Jun 13, 2024

@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
Copy link

aeb-dev commented Oct 30, 2024

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

@CodeBlanch
Copy link
Contributor

@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.

@evgenyfedorov2
Copy link

is there any action on this? is it on track to make it to .NET 10?

@antonfirsov
Copy link
Member

antonfirsov commented Feb 7, 2025

This is not being actively worked on now, but let's chat offline!

@antonfirsov
Copy link
Member

Since the OTel SDK does have the feature, this is about bringing it-in box. We should be very careful with the API design (we made mistakes when designing MetricsEnrichmentContext IMO it shouldn't have been shipped in its' current form). There are several API requests in this area and ideally it would be great to unify them to avoid API friction, see #103130.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests

8 participants