dotnet / runtime

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

[API Proposal]: HttpClientFactory AddMetricsEnrichment #88460

Open JamesNK opened 1 year ago

JamesNK commented 1 year ago

Background and motivation

.NET 8 adds metrics and enrichment support to HttpClient. See https://github.com/dotnet/runtime/issues/86281

This API proposal is for a new HttpClientFactory API. It's designed to make it easier to combine enrichment with the factory.

Right now, there isn't an ask for this feature. However:

  1. It is a low-cost change that makes enrichment easy to use with the client factory.
  2. People who want to combine enrichment with HttpClientFactory could use this extension method. Reusing this method would save dotnet/extensions from having to implement this feature internally.

I expect this feature will be implemented by registering an internal http handler with the client factory. The handler just calls HttpMetricsEnrichmentContext.AddCallback with the callback passed to the extension method.

API Proposal

namespace Microsoft.Extensions.DependencyInjection;

public static class HttpClientBuilderExtensions
{
    public static IHttpClientBuilder AddMetricsEnrichment(
        this IHttpClientBuilder builder,
        Action<HttpMetricsEnrichmentContext> callback);
}

API Usage

services.AddHttpClient("my-cool-client")
    .AddHttpMessageHandler<CustomHttpHandler>()
    .AddMetricsEnrichment(context =>
    {
        var status = context.Response.Headers.GetValue("x-status");
        context.AddCustomTag("x-status", status);
    });

Alternative Designs

No response

Risks

No response

ghost commented 1 year ago

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

Issue Details
### Background and motivation .NET 8 adds metrics and enrichment support to HttpClient. See https://github.com/dotnet/runtime/issues/86281 This API proposal is for a new HttpClientFactory API. It's designed to make it easier to combine enrichment with the factory. Right now, there isn't an ask for this feature. However, it is a low-cost change that makes enrichment easy to use with the client factory. I expect this feature would be implemented by registering an internal http handler with the client factory. The handler just calls `HttpMetricsEnrichmentContext.AddCallback` with the callback passed to the extension method. ### API Proposal ```csharp namespace Microsoft.Extensions.DependencyInjection; public static class HttpClientBuilderExtensions { public static IHttpClientBuilder AddMetricsEnrichment(this IHttpClientBuilder builder, Action callback); } ``` ### API Usage ```csharp services.AddHttpClient("my-cool-client") .AddHttpMessageHandler() .AddMetricsEnrichment(context => { var status = context.Response.Headers.GetValue("x-status"); context.AddCustomTag("x-status", status); }); ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: JamesNK
Assignees: -
Labels: `api-suggestion`, `area-Extensions-HttpClientFactory`
Milestone: -
JamesNK commented 1 year ago

cc @antonfirsov @CarnaViire @noahfalk @tarekgh

CarnaViire commented 1 year ago

Triage: as discussed with @JamesNK offline, this is a nice-to-have and not critical for .NET 8. Even if dotnet/extensions would need to implement this for enrichment on their side, it is going to be straightforward and wouldn't need a public API. Moving to Future.

davidroth commented 10 months ago

Will it be possible to also disable metrics on individual named http clients?

I have a use case where I am only interested in metrics on a specific named http client of my application. I would like to not see the metrics of other http clients.

CarnaViire commented 10 months ago

@davidroth that's a good point. @antonfirsov is it even possible to do with existing metrics APIs? I can see that enrichment can only add the tags, but not remove them. And SocketsHttpHandler will use some default shared meter even if the meter factory was set to null 🤔

antonfirsov commented 10 months ago

When it comes to the .NET libraries, we do provide the infrastructure to enable this: it is possible to initialize SocketsHttpHandler and HttpClientHandler to use custom IMeterFactory instances eg. by using ConfigurePrimaryMessageHandler, so you can take control of your Meter instances for the clients you do care about. On the listener side, the originating Meter and IMeterFactory == meter.Scope instances can then be observed and used for filtering in MeterListener.InstrumentPublished callbacks.

However, to make this useful in practice, collection/exporter libaries (the stuff that subscribes to these callbacks) need to expose APIs for filtering (eg. to drop recordings for a specific meter or factory). @davidroth what do you use for collection? @vishweshbankwar is there such a filter mechanism in opentelemetry-dotnet?

CarnaViire commented 10 months ago

it is possible to initialize SocketsHttpHandler and HttpClientHandler to use custom IMeterFactory instances

That was my first thought, but what should such "no-op" factory return upon the meterFactory.Create call? if it returns null, the code will just use SharedMeter.Instance once again:

https://github.com/dotnet/runtime/blob/7269f90cb075cee9fd99fa88060bcf9d83457584/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs#L22

It seems that the returned meter instance should be some kind of mock as well? But Meter is a class, not an interface. I didn't look further, but it already seemed too complicated 🤔

antonfirsov commented 10 months ago

That was my first thought, but what should such "no-op" factory return upon the meterFactory.Create call?

It should return a functional Meter, not a no-op. Listening is enabled per-instrument, typically in MeterListener.InstrumentPublished, therefore the instruments a user wants to ignore should be filtered out (not enabled) in MeterListener.InstrumentPublished eg. based on the Meter instance referenced by the Instrument passed to the callback. (Also note that Meter.Scope typically references the IMeterFactory.)

IMO this should be implemented by the consumers of that event, eg. in collection libraries like Prometheus Exporter.

CarnaViire commented 10 months ago

Is this correct that the nesting goes as: MeterFactory -> Meter -> Instrument? And -- judging by the implementation -- Meter is always System.Net.Http, and Instruments are always http.client.active_requests and http.client.request.duration, regardless of the MeterFactory implementation?..

Then I really don't understand how can I differentiate between several HttpClients and say "only include this one"... 🤔 Because I don't have any controls over neither Meter name, nor Instrument names... So when I enable the instrument, e.g. http.client.request.duration, I enable listening to all of the HttpClients? Can I even tell them apart in the results? Am I missing something?

For comparison -- I know it's a completely different thing, but e.g. default HttpClientFactory logging includes name of the client into the logger name (e.g. "System.Net.Http.HttpClient.foo.ClientHandler"), so you can filter out (or in) specific clients by name in the logging config, like

{
  "Logging": {
    "LogLevel": {
      "Default": "Warning",
      "System.Net.Http.HttpClient": "None",
      "System.Net.Http.HttpClient.foo": "Information"
    }
}
antonfirsov commented 10 months ago

Given an Instrument, the IMeterFactory instance is accessible using instrument.Meter.Scope (== theMeterFactory). This enables comparing the factory instances by reference. For the HttpClient(s) which should not be metered, a user can override the IMeterFactory assigned to the handler with a custom one in ConfigurePrimaryMessageHandler. If the collection library (ie. OpenTelemetry.NET's Prometheus Exporter) has a way to hook user code into their InstrumentPublished implementation, it's possible then to check whether instrument.Meter.Scope == services.GetRequiredService<IMeterFactory>(), and enable the instrument only if the comparison returns true. Another way is to make the custom IMeterFactory add a tag to the Meter the custom factory creates, which can be then checked in the callback.

Ofc, this is complicated, and assumes that Prometheus Exporter (or other collection libraries) have ways to customize the filtering for their InstrumentPublished implementations. If filtering metrics by originating HttpClient instance is a popular feature request, we may need to have a discussion about making the user experience simpler. Since this issue is about enrichment, I would prefer to have that discussion in a separate one. @davidroth do you mind opening a new issue, if this is important for you?