-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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]: Pass Meter to HttpClientHandler and SocketsHttpHandler #86961
Comments
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackground and motivationThis is part of #84978. For testability, we need to enable passing a custom API ProposalI'm proposing to expose a property to set the 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 Also, the ownership semantics are somewhat counterintuitive, since API Usageusing 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
|
Does this instead mean we have the layering wrong? |
Depends how deep do we want Moving |
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 |
We discussed two options in the past:
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 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. |
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 |
Thinking this over again I think the difference is insignificant from
@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? |
@tarekgh another question we should consider if there are any other BCL libraries where we expect passing a |
@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. |
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 it is matter of time till the package get released to the nuget.org. |
There are a lot of scenario based questions that were taking up a lot of time in the meeting and we weren't getting answers, so this is being sent back for followup. One particular question was what the expected behaviors are around a pipeline of delegating message handlers, and are they all supposed to use the same customized Meter, or different Meters, and if the same, how is it supposed to flow? As proposed, "forwarding" doesn't seem possible. https://github.com/Pixeval/Pixeval/blob/main/src/Pixeval.CoreApi/Net/RetryHttpClientHandler.cs was a randomly searched out example, if this type wanted to add metrics, how should it participate in the pipeline? namespace System.Net.Http;
public class HttpClientHandler
{
public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
}
public class SocketsHttpHandler
{
public Meter Meter { get; set; } // = DefaultGlobalMeterForHttpClient;
} |
The industry is standardizing on OpenTelemetry and its metrics capabilities. That standardization includes OTel's various SDKs (C#, Java, Go, etc), Prometheus (popular cloud-native metrics database), Grafana (popular cloud-native dashboard and alerting software). The .NET metrics API is based on OTel. I don't see a situation anytime soon that would cause this to change. "This time is different." he says confidently. @noahfalk is the best person to discuss the stability and longevity of metrics. Edit: I listened further into the video and people seemed satisfied that
I agree that there isn't an automatic way for other handlers to get access to the A
Scenario 1: Custom Scenario 2: Custom Note that the tldr; other |
I've updated the original issue body. The usage example now includes a scenario of a |
Sorry I probably should have been in the meeting to help answer questions. James answered some above and here are answers to some other questions that I heard: [Stephen]: Whats the behavior if diagnostics are disabled?These allocations occur in the expected common cases:
For the rare case where a user manually created their own Meter and passed it directly to HttpClient or HttpMessageInvoker then it could be as much as one set of objects per-client. It is up to that user whether they are passing in the same Meter to multiple clients or they allocate a new one every time. In terms of CPU latency, when diagnostics are disabled then the property Instrument.Enabled returns false. Think of this similar to ILogger where ILogger.IsEnabled(level) returns false. The code in Metrics handler checks for this and short-circuits to the next handler if nothing is being recorded. See: https://github.com/dotnet/runtime/pull/87319/files#diff-283460efa1becb9e2937c6cbc738e6c6d48ff9e4d268304c1e1652b5e44ac120R42 [Stephen] I would expect if this was disabled the Meter property would return null?Disabling metrics at a global/per-factory/per-client level wasn't considered because we assumed the cost of one extra virtual call and a few boolean property checks per-request wasn't a large enough cost to justify additional configuration complexity. If there is no disable mechanism then there is no need to signal that it is disabled. We also don't expect user-code to read this property back in general, it is intended for either user-code or HttpClientFactory to set the value and for MetricsHandler to read it. Many other types would do this type of configuration as a constructor parameter, but HttpClient appears to use a property injection pattern rather than constructor injection so we attempted to follow suit. [Jeremy] I don't feel like the diagnostics APIs are stable enough that we should commit to them being in important types like HttpClientWhile I don't have a crystal ball, I think the odds of us abandoning this API within at least the next decade is quite low. In .NET's entire history we've had three metrics APIs I am aware of, Windows Perf Counters (early 2000s), EventCounters (.NET 3), and now Meters(.NET 6). We stopped using the first when we needed to support multiple OSes, the 2nd was a hastily designed xplat replacement and we recognized its shortcomings fairly quickly. The Meter API was designed as part of the OpenTelemetry effort in a lengthy effort with industry experts in this space. That standardization is a notable marketing bullet for our product and it can't be discarded lightly. We are taking a bet on it in ASP.NET Core for .NET8 and likely will consume it elsewhere like EntityFramework in .NET 9. I see no sign anywhere on the horizon that we would want to walk ourselves back from this API. If we want to do sneaky things like define the type as object rather than Meter or bury it in some out-of-the-way corner of the API on a separate interface that you have to cast to that doesn't bother me. The only bit that is important IMO is getting a Meter object transported from HttpClientFactory down to HttpClient and it happens to cross a public API boundary in the middle. [Various folks] Given that this is only used for tests...I think this framing is incomplete. A fuller description of the work is that it allows segregating the telemetry data based on the DI container that it was produced from. In the short term the value of doing this does accrue primarily to testing. Over the longer term Fowler wants the freedom to pursue multi-DI-container-per-process application models in production use and having important telemetry be process-global impedes that effort. HttpClientFactory presents itself as a DI-aware API so it is natural that users would expect it to produce DI-segregated telemetry. [Stephen] If everyone else is injecting these via IMeterFactory, why does HttpHandler do it different?IMeterFactory is in Microsoft.Extensions.Diagnostics.Abstractions and the layering rules as far as I know prevent us from referencing that assembly in System.Net.Http.dll. Functionally the difference shouldn't matter and in terms of design aesthetics I expect very few developers will ever interact with this API directly. [Stephen] What else aside from System.Net.Http do we expect to use Meter in the .NET 8 timeframe?As a public API, nothing. As an implementation detail maybe System.Diagnostics.DiagnosticSource will implement a System.Runtime Meter. Looking forward I only expect Meter to show up in a System.* public API when these conditions are met:
[Paraphrasing Martin and others] How do we deal with the desire for different names?This is an area that is still under active development and discussion, but I expect there are a few different solutions:
My current best guess is that custom instrumentation is a short term solution for 1P needs and evolution around OTel views or similar APIs is a good long term solution. |
"Ok!" he says as he screenshots the response 😄
Ok. What about multiple terminating handlers, e.g. if someone else wrote their own SuperFastHttpHandler, what Meter instance should it be using? If library A is creating SocketHttpHandler that's using a I'm still not clear on what the purpose is of the Meter's name, why it needs to be "System.Net.Http" for a Meter explicitly provided.
It's not just CPU; if it's enabled, that leads to calling an async method, which will invariably need to suspend at some point, which results in an extra layer of state machine allocation:
Does it need a public getter then? Should it just be a SetMeter method or some such thing? Exposing the getter means we'll be exposing the global singleton Meter that's used by default; what happens if someone uses that instance? Can they mess anything up?
How does this jive with HttpClient requiring that the Meter be named "System.Net.Http"? What else about the Meter instance is unique such that the data produced by one instance can be discerned from data produced by another instance?
This is putting the cart before the horse. If the argument is "the right thing to do is to take a Meter because functionally that makes sense for XYZ reasons", great. But we control the layering, so saying we're doing this because of layering isn't a good reason. I've heard Tarek state that the right answer is to take a Meter rather than an IMeterFactory for reasons unrelated to layering; if others agree with that, I'll drop it.
I don't understand this. Why would a DI-aware wrapper be a requirement for using Meter? Isn't Meter intended as a 100% replacement for DiagnosticCounter, such that anywhere we're currently using DiagnosticCounter today we'd expect to use Meter tomorrow? And if testability with HttpClient + Meter is important, why wouldn't it be important in all of those places? Thanks. |
In that situation, if the person writing the library wants It's ok if different people create two meters with the same name. There isn't a rule against it. Comparing by
Think of the meter name like a namespace. It's a way to group counters together. Also, when counter values are exported to Prometheus (a cloud native metrics database), the meter name is included as a tag. The meter name is externally visible in the exported data. The OTel spec might provide insight of how things relate to each other: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md We want to restrict SocketsHttpHandler and friends to only allow "System.Net.Http" as the meter name to prevent people from changing the names of built-in counter values when they're exported and queried. It simplifies the eco-system if we lock it down.
Correct. There is always a meter. There are always counters. But if no one is listening to the counters (
I'll think more about whether a getter is necessary. But exposing the global singleton Meter won't mess anything up. It's wrapped to prevent
MeterListener gives you the instrument instance (which has a reference to its meter), and you can decide how you want to test whether a meter's instruement is listened to. You could listen to all meters called "System.Net.Http" or listen based on one particular meter instance. In a unit test you might know the instance because it was customized when the HttpClient was created. That's great because now you get metrics from the HttpClient instance used in your test and not clients in other tests that are running in parallel. Example of filtering to meter's based on their instance: Lines 68 to 70 in 0c77cbe
For example, the
+1. The API here should be Meter.
I disagree with this statement from Noah. I think Meter will show up if we want to allow people to test or gather metrics on something in isolation. Whether it has DI or not doesn't matter. For example:
A place where it doesn't make sense to expose a Meter is for global/static metrics like CPU usage or garbage collection counters. |
Yep, now I just have to make sure the next decade plays out how I expect... how hard could it be? :)
Yeah, thats fair. I would still consider this DI in principle, but it isn't specifically M.E.DependencyInjection which is what I had been thinking about as I wrote my earlier comment.
It isn't a requirement for using a Meter. I was trying to describe circumstances where I expected it to "show up in the public API". For example this code has no trouble using a Meter, but it doesn't expose it in the public API of the type: void DoSomething(int x)
{
// not a performant way of doing it, but it works fine
Meter m = new Meter("MyAwesomeMeter");
Counter<int> c = m.CreateCounter("TheBestestCounter");
c.Add(x);
// ...
} In order to show up in the public API the Meter is presumably being injected from elsewhere because the caller has some reason to care what instance of the Meter gets used.
Answer take two :) Functionally both options work and I think it makes little difference. If hypothetically layering was of absolutely no concern I would differ with others and pick IMeterFactory as the type, but I'm only saying that out of honesty on my current opinion, not because I have any interest in spending more time to reverse course. I think the time spent in a debate splitting hairs on pros and cons, in potential design changes, or in layering changes far, far outweigh any tiny value we might get from switching the type. I also think I'm the only one who has that preference and everyone else in the feature crew feels Meter is the preferred option. I am just fine moving ahead with Meter. |
I very much agree with this. There is little or no practical functional difference between injecting the meter instance or the "thing that creates the meter" here. We can achieve everything with both options, and we are protected from pitfalls in both cases. Exposing the |
I disagree. Getting layering set up correctly now and for the future is important. That said, I'm fine with the outcome in this case. |
For future extensibility it seems like the best answer is to take something like namespace System.Net.Http
{
public class HttpClientHandler
{
public IMeterFactory? MeterFactory { get; set; }
}
public class SocketsHttpHandler
{
public IMeterFactory? MeterFactory { get; set; }
}
}
namespace System.Diagnostics.Metrics
{
public interface IMeterFactory : IDisposable
{
Meter Create(MeterOptions options);
}
public static class MeterFactoryExtensions
{
public static Meter Create(this IMeterFactory, string name, string? version = null, IEnumerable<KeyValuePair<string,object?>> tags = null, object? scope = null);
}
} |
Background and motivation
This is part of #84978. For testability, we need to enable passing a custom
Meter
toSocketsHttpHandler
andHttpClientHandler
. SinceIMeterFactory
lives inMicrosoft.Extensions.Diagnostics.Abstractions
, we cannot use it inSystem.Net.Http
.API Proposal
I'm proposing to expose a property to set the
Meter
instance directly.The downside of this API is that it makes
SocketsHttpHandler
's andHttpClientHandler
'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'sMeter
instance.API Usage
Alternative Designs
Meter
directly, exposeFunc<MeterOptions, Meter>?
instead.IMeterFactory
toSystem.Diagnostics.Metrics
and exposeIMeterFactory
instead of exposing theMeter
.The text was updated successfully, but these errors were encountered: