[API Proposal]: Pass Meter to HttpClientHandler and SocketsHttpHandler

[antonfirsov]

Background and motivation

This is part of #84978. For testability, we need to enable passing a custom Meter to SocketsHttpHandler and HttpClientHandler. Since IMeterFactory lives in Microsoft.Extensions.Diagnostics.Abstractions, we cannot use it in System.Net.Http.

API Proposal

I'm proposing to expose a property to set the Meter instance directly.

namespace System.Net.Http;

public class HttpClientHandler
{
    public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
}

public class SocketsHttpHandler
{
    public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
}

The downside of this API is that it makes SocketsHttpHandler's and HttpClientHandler's responsibility to enforce the correct meter name in the property setter:

https://github.com/antonfirsov/runtime/blob/5abf04854dbb6c2fca458663a4affb580e727ec8/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs#L458-L468

Also, the ownership semantics are somewhat counterintuitive, since SocketsHttpHandler.Dispose() should not dispose it's Meter instance.

API Usage

using Meter meter = new Meter("System.Net.Http");
using HttpClient client = new(new HttpClientHandler()
{
    Meter = meter
});

// Use the client with the custom meter
// Because the recorder has the meter instance we care about, only values from this meter are captured.
var instrumentRecorder = new InstrumentRecorder<double>(meter, "http-client-request-duration");

// There is another InstrumentRecorder constructor that takes the meter name. If the meter name was used, "System.Net.Http",
// then all counter values with that meter name would be collected.
// var globalInstrumentRecorder = new InstrumentRecorder<double>(System.Net.Http", "http-client-request-duration");

// Make HTTP request.
var response = await client.GetAsync("https://www.bing.com");
var data = await response.Content.ReadAsBytesAsync();

// Assert recorded "http-client-request-duration" value and check the "status-code" tag.
Assert.Collection(instrumentRecorder.GetMeasurement(),
    m => Assert.Equals(200, (int)m.Tags.ToArray().Single(t => t.Name == "status-code"));

Alternative Designs

• Instead of exposing the Meter directly, expose Func<MeterOptions, Meter>? instead.
• Move IMeterFactory to System.Diagnostics.Metrics and expose IMeterFactory instead of exposing the Meter.

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

Issue Details

---

Background and motivation

This is part of #84978. For testability, we need to enable passing a custom Meter to SocketsHttpHandler and HttpClientHandler. Since IMeterFactory lives in Microsoft.Extensions.Diagnostics.Abstractions, we cannot use it in System.Net.Http.

API Proposal

I'm proposing to expose a property to set the Meter instance directly.

namespace System.Net.Http;

public class HttpClientHandler
{
    public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
}

public class SocketsHttpHandler
{
    public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
}

The downside of this API is that it makes SocketsHttpHandler's and HttpClientHandler's responsibility to enforce the correct meter name in the property setter:

https://github.com/antonfirsov/runtime/blob/5abf04854dbb6c2fca458663a4affb580e727ec8/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs#L458-L468

Also, the ownership semantics are somewhat counterintuitive, since SocketsHttpHandler.Dispose() should not dispose it's Meter instance.

API Usage

using Meter meter = new Meter("System.Net.Http");
using HttpClient client = new(new HttpClientHandler()
{
    Meter = meter
});

// Use the client with the custom meter
// Observe instrument events on the custom meter.

Alternative Designs

• Instead of exposing the Meter directly, expose Func<MeterOptions, Meter>? instead.
• Move IMeterFactory to System.Diagnostics.Metrics and expose IMeterFactory instead of exposing the Meter.

Author:	antonfirsov
Assignees:	-
Labels:	api-suggestion, area-System.Net.Http
Milestone:	8.0.0

[stephentoub]

Since IMeterFactory lives in Microsoft.Extensions.Diagnostics.Abstractions, we cannot use it in System.Net.Http.

Does this instead mean we have the layering wrong?

[antonfirsov]

Does this instead mean we have the layering wrong?

Depends how deep do we want IMeterFactory to live. Historically, we didn't care much about DI and testability in System.* API-s, which made it more natural to put IMeterFactory into Microsoft.Extensions.*, however as we can see, this makes meter injection to HttpClient look weird. Did the final rounds of discussions on #77514 take into account that we need to pass Meter instances to HttpClient(Handler)? If no, we should reopen the layering discussion.

Moving IMeterFactory to System.Diagnostics.Metrics (and as a result getting rid of Microsoft.Extensions.Diagnostics.Abstractions) is listed in the "Alternative Designs", in fact now I'm in favor of that option, if we can get consensus.

/cc @tarekgh @noahfalk

[tarekgh]

however as we can see, this makes meter injection to HttpClient look weird

Are you expecting HttpClient to create the Meter? or is this something passed from the HttpClient consumer? Think about it if someone is using HttpClient without a DI at all, what do you expect users will do?

I expect the layer Microsoft.Extensions.Http would be the right place to get the IMeterFactory from the DI and then create the meter and pass it to the HttpClient. It would be good if clarify how things expected to work with meters in the http client.

[noahfalk]

Does this instead mean we have the layering wrong?

We discussed two options in the past:

1. IMeterFactory lives in System.Diagnostics.DiagnosticSource.dll and HttpClient takes a dependency on it
2. IMeterFactory lives in Microsoft.Extensions.* and HttpClient takes a dependency only on Meter (the proposal @antonfirsov outlines above)

I was in favor of the first approach, but BCL design review was strongly opposed to having IMeterFactory defined as an interface go into S.D.DiagnosticSource, so I abandoned that plan and assumed we would do option (2) here.

I expect the layer Microsoft.Extensions.Http would be the right place to get the IMeterFactory from the DI and then create the meter and pass it to the HttpClient

I agree, but I don't think it conflicts with what is bring proposed here. I think of it as two layering boundaries we need to pass through in that scenario: DI container <-> HttpClientFactory <-> HttpClient. At the first boundary the factory can consume IMeterFactory, at the 2nd boundary being discussed here factory needs to instantiate a Meter and pass it to HttpClient. Its also possible (though probably uncommon) that users who create HttpClient directly would configure the Meter themsevles.

[stephentoub]

Thanks for the info.

We shouldn't design HttpClient's surface area based on a disagreement about in which assembly an abstraction should live because of the shape of that abstraction :)

Let's come to an agreement about what's ideal for HttpClient first. In an ideal world, does it make more sense for it to be handed a Meter or to be handed the thing that creates the Meter? What is HttpClient actually going to do with it? If it makes more sense for it to be handed a Meter, great, we can keep IMeterFactory where and how it is (assuming we're not going to be having this discussion tomorrow about something else in the core libs). If it makes more sense for it to be handed the thing that creates the Meter, then let's fix the layering. I don't want to put the cart before the horse.

cc: @terrajobst

[antonfirsov]

In an ideal world, does it make more sense for it to be handed a Meter or to be handed the thing that creates the Meter?

Thinking this over again I think the difference is insignificant from HttpClientHandler/SocketsHttpHandler perspective:

• If we hand a Meter (before starting the client!) we should guard the name.
• If we hand a factory, it would create (a potentially user-provided) Meter at startup time based on the meter name passed in MeterOptions, which the factory implementation could ignore, so it's still worth to guard the resulting Meter.

@JamesNK I wonder how does the difference feel like from user (test-implementer) perspective, and is it just a convenience difference or a fundamental thing?

[antonfirsov]

@tarekgh another question we should consider if there are any other BCL libraries where we expect passing a Meter for testability, and if yes would it be better to pass the factory instead of passing the meter? If the answer is yes for both questions, it's probably worth to move IMeterFactory, otherwise no.

[tarekgh]

@antonfirsov could you please explain more about your testability scenario? What exactly do you want to do? providing a meter different than the default used one in the library? That can be achieved without the factory anyway. The factory is interesting only in the DI scenarios which users want to have isolation inside the containers and want to override the factory to control the meter creation.

[noahfalk]

In an ideal world, does it make more sense for it ...

I agree with @antonfirsov that the difference in approaches feels minor. If there were no other considerations I'd give a small edge to IMeterFactory, but that is a personal opinion rather than a consensus. I expect the number of users who write code directly interacting with the new APIs will be quite small (most either will use the default global Meter, or HttpClientFactory will configure it on their behalf). In terms of precedent I'm also not expecting many other System.* libraries to make use of it, maybe a couple over the next 5-10 years? If they did the situation would probably be similar to HttpClient where most usage would either use a default value or some Microsoft.Extensions.* thing would create a higher level abstraction and configure a DI container specific Meter as an implementation detail the developer doesn't tend to see.

[excme]

https://github.com/dotnet/runtime/blob/ce689d9189d549e249f4bb5e3d9aeef322464bfb/src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/README.md have link to nuget.
Why https://www.nuget.org/packages/Microsoft.Extensions.Diagnostics.Abstractions 404?

[tarekgh]

@excme it is matter of time till the package get released to the nuget.org.