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

[API Proposal]: HttpClientFactory AddMetricsEnrichment #88460

Open
JamesNK opened this issue Jul 6, 2023 · 10 comments
Open

[API Proposal]: HttpClientFactory AddMetricsEnrichment #88460

JamesNK opened this issue Jul 6, 2023 · 10 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 6, 2023

Background and motivation

.NET 8 adds metrics and enrichment support to HttpClient. See #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

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory labels Jul 6, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 6, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

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 #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

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

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-Extensions-HttpClientFactory

Milestone: -

@JamesNK
Copy link
Member Author

JamesNK commented Jul 6, 2023

@CarnaViire
Copy link
Member

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

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

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

antonfirsov commented Jan 4, 2024

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

CarnaViire commented Jan 4, 2024

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:

meter = meterFactory?.Create("System.Net.Http") ?? SharedMeter.Instance;

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

antonfirsov commented Jan 8, 2024

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

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

antonfirsov commented Jan 8, 2024

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?

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-Extensions-HttpClientFactory
Projects
None yet
Development

No branches or pull requests

4 participants