Repository navigation
[API Proposal]: pass custom tags for metrics enrichment in HttpClientHandler #86281
Description
Latest update: switch to callback-based API to observe HttpResponseMessage for enrichment. Starting a new round of API discussion.
Background and motivation
As part of #84978, we need to enable enriching metrics logs with additional custom tags injected by the application/middleware. These tags need to be injected on per-request basis meaning that they should be provided in HttpRequestMessage. Enrichment is an advanced scenario, and metrics will be only implemented in HttpClientHandler/SocketsHttpHandler, therefore the value of exposing a property on HttpRequestMessage would be questionable. Instead, we can pass the custom tags using HttpRequestMessage.Options. The problem is that there are no good established practices to document and it's hard to discover discover it. Defining a key alone would also leave the door open to mistakes. As a compromise, I'm proposing an API that works via an extension method over HttpRequestOptions using an internal key.
The API should provide an ability inject tags based on HttpResponseMessage. Since the metrics implementation will live within HttpClientHandler/SocketsHttpHandler, it would be too late to do this in a DelegatingHandler. To handle this, the proposed API is passing user-provided callbacks to the metrics implementation which can the custom tags to be added for enrichment.
API Proposal
namespace System.Net.Http.Metrics;
public sealed class HttpMetricsEnrichmentContext
{
public HttpRequestMessage Request { get; }
public HttpResponseMessage? Response { get; } // null when an exception is thrown by the innermost handler
public Exception? Exception { get; } // null when no exception is thrown
public void AddCustomTag(string name, object? value);
// Alternatively we can expose a collection to allow enumerating, removing in the future, but such functionality is not needed today.
// public ICollection<KeyValuePair<string, object?>> CustomTags { get; }
}
// Or should it be MetricsHttpRequestOptionsExtensions?
// runtime/libraries is not consistent on this, see HttpClientJsonExtensions vs OptionsServiceCollectionExtensions
public static class HttpRequestOptionsMetricsExtensions
{
public static void AddMetricsEnrichmentCallback(this HttpRequestOptions options, Action<HttpMetricsEnrichmentContext> callback));
}API Usage
public class EnricherHandler : DelegatingHandler
{
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
request.Options.AddMetricsEnrichmentCallback(
static ctx => {
ctx.AddCustomTag("org-name", ctx.Request.Headers.GetValue("x-org-name"));
ctx.AddCustomTag("result-category", GetResultCategory(ctx.Response.StatusCode));
}
);
return await base.SendAsync(request, cancellationToken);
}
private static string GetResultCategory(HttpStatusCode statusCode) => statusCode >= 200 && statusCode < 300 ? "Good" : "Bad";
}Alternative Designs
Attempt to reduce heap allocations by working with value types
Note that the prerequisite for such an attempt to be successful is to resolve #83701, otherwise TagList would allocate in almost all R9 scenarios.
namespace System.Net.Http.Metrics;
public readonly struct HttpMetricsEnrichmentContext
{
public readonly HttpRequestMessage Request { get; }
public readonly HttpResponseMessage? Response { get; }
public readonly Exception? Exception { get; }
}
public delegate void HttpMetricsEnrichmentCallback(in HttpMetricsEnrichmentContext context, ref TagList customTags);
public static class HttpRequestOptionsMetricsExtensions
{
public static void AddMetricsEnrichmentCallback(this HttpRequestOptions options, HttpMetricsEnrichmentCallback callback));
}
Activity
antonfirsov commentedon May 15, 2023
@noahfalk @JamesNK any thoughts? Where would usages of
CustomMetricsTagslive? Only R9 /other middleware or do we expect it to be directly used by customer applications?JamesNK commentedon May 17, 2023
Typo in the proposal. I'm guessing
public ICollection<string, object?> CustomMetricsTags { get; }is wrong and you meanICollection<KeyValuePair<string, object?>>Adding custom metrics would be something for power users. Potential users:
/products/{id}. This is likely added while creating theHttpRequestMessage.DelegatingHandler. The interface would be configured for a client on the client factory. The interface would then be called by the delegating handler during the handler pipeline. The result would be stored on the request's tags collection.DelegatingHandler. Because the handler pipeline for allHttpClientuses can be configured by a library, we'd want to test that someone can hook into the HTTP DiagnosticsSource events and add tags from there.MihaZupan commentedon May 22, 2023
Moving to 8.0 to track this as part of #84978, feel free to bump back to Future
antonfirsov commentedon May 24, 2023
Note that we have a precedent for downstream libraries defining extension methods to help using well-known keys:
https://github.com/dotnet/aspnetcore/blob/b441d4e816ef3bc43b72e91ba8793f252ada2c5a/src/Components/WebAssembly/WebAssembly/src/Http/WebAssemblyHttpRequestMessageExtensions.cs#L144C38-L151
noahfalk commentedon May 26, 2023
I agree with James that the scenario for arbitrary key/value pairs is aimed at power users and SDK/library authors, not the general audience that uses HttpClient. I don't think it needs to be overtly discoverable as a top-level property would be. Defining a convention for Options key name could work for R9, though it feels quite hard to discover for anyone else and leaves the door open for mistakes or weird incompatibilities like different library authors assuming they can store their own uniquely typed ICollection in there. I think a nice middle ground would be defining either a property or an extension method on the HttpRequestOptions.
It looks like code elsewhere assumes that HttpRequestOptions can be reliably copied by enumerating and copying all the dictionary keys (https://source.dot.net/#Microsoft.AspNetCore.Mvc.Testing/Handlers/RedirectHandler.cs,160) so unless you planned to change that (including where user code might do the same thing) then I think we'd want the property backed by a dictionary entry rather than a field.
I think the suggestion is close to your option (2) above but I am suggesting that the property/extension method be defined on the Options type, not on HttpRequestMessage to reduce its visibility. If the implementation of MetricsHandler is going to be in System.Net.Http then I'd put the API there too, not in M.E.Http, because I don't see any benefit in limiting it that way.
All of this is just a suggestion though. If networking team or BCL design review believe one of the other options is better then go for it. I'll sleep fine with any of options you proposed. Hope that helps!
antonfirsov commentedon May 28, 2023
I had a same thought recently. Updated the proposal, thanks for the feedback!
[-][API Proposal]: HttpRequestMessage.CustomMetricsTags[/-][+][API Proposal]: pass custom tags for metrics enrichment in HttpClientHandler[/+]antonfirsov commentedon Jun 1, 2023
One concern that came up in todays discussion that the cost of using
HttpRequestOptions(enforcing the dictionary allocation) may end up being significant.@noahfalk @JamesNK @dpk83 do you have any insights whether R9 or other middleware use-cases would come with enrichment handlers wired-in by default or in the majority of use-cases?
20 remaining items
JamesNK commentedon Jul 3, 2023
The add callback method is no longer an extension method. Part of the reason to choose
HttpRequestOptionsas thethisargument for it to hang off was to hide it away. That's no longer a concern.I propose a small change to make the
AddCallbackmethod takeHttpRequestMessageas an argument instead ofHttpRequestOptions:Reasons:
HttpRequestMessagemakes it slightly more obvious what to pass to the method (more people are familiar withHttpRequestMessagethanHttpRequestOptions)HttpRequestMessagereference.HttpRequestMessage.Optionsautomatically allocates the collection if it wasn't already present. There would be no way to ever optimize that away with the current shape.stephentoub commentedon Jul 3, 2023
+1 to James' comment
JamesNK commentedon Jul 4, 2023
Removing API approved tag and putting back api-ready-for-review. Change to consider: #86281 (comment)
I don't know the runtime API review system. Hopefully this is the right way to do it.
This isn't urgent. We can proceed with the current approved API and easily change it based on feedback. As long as it is considered before .NET 8 locks down.
karelz commentedon Jul 4, 2023
@antonfirsov perhaps we can do the incremental change review over email?
Implementation should be unblocked per @JamesNK as the difference is apparently very small.
antonfirsov commentedon Jul 4, 2023
Email sent. If it doesn't work out, I will open a new issue, since I don't want #87319 to be blocked on this.