[API Proposal]: pass custom tags for metrics enrichment in HttpClientHandler

[antonfirsov]

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));
}

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

Issue Details

---

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.
I'm assuming that enriching is a scenario that is common enough to be supported by an API, but we need to make sure we are doing the right thing.

API Proposal

namespace System.Net.Http;

public class HttpRequestMessage : IDisposable
{
    public ICollection<string, object?> CustomMetricsTags { get; }
}

See the prototype for more info about how the custom tags will be used.

API Usage

public class EnricherHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage req, CancellationToken cancellationToken)
    {
        KeyValuePair<string, object?> tag = new("org-name", req.Headers.GetValue("x-org-name"));
        req.CustomMetricTags.Add(tag);
        var response = await base.SendAsync(req, cancellationToken);
        return response;
    }
}

Alternative Designs

1. To simplify usage, we can consider a way to expose an Add(string, object?) method. This could be achieved by exposing CustomMetricsTags type, or by extension methods.
2. Store the custom tags in HttpRequestMessage.Options and expose extension methods. Since this solution would also introduce new public API-s, it feels like a complicated and unconventional way to expose a property. Nevertheless, we can consider adding such extensions to Microsoft.Extensions.Http, if expect enrichment use-cases to always build on top of Microsoft.Extensions.Http.
3. Store the custom tags in HttpRequestMessage.Options without exposing any APIs and let downstream libraries deal with the extension methods. This is reasonable if encrichment is an advanced/niche use-case we don't want to support at API level.

Risks

Not all handlers will support metrics. Whether the initial implementation would work only with SocketsHttpHandler or also with mobile/wasm handlers depends on the outcome of the prototyping work. It's unlikely that we will ever implement metrics for WinHtpHandler. If we think HttpRequestMessage APIs should be always handler-agnostic we should go with the alternative options 2. or 3.

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

[antonfirsov]

@noahfalk @JamesNK any thoughts? Where would usages of CustomMetricsTags live? Only R9 /other middleware or do we expect it to be directly used by customer applications?

[JamesNK]

Typo in the proposal. I'm guessing public ICollection<string, object?> CustomMetricsTags { get; } is wrong and you mean ICollection<KeyValuePair<string, object?>>

@noahfalk @JamesNK any thoughts? Where would usages of CustomMetricsTags live? Only R9 /other middleware or do we expect it to be directly used by customer applications?

Adding custom metrics would be something for power users. Potential users:

• Devs who are writing an SDK and wish to add extra metadata such as the request's route template, e.g. /products/{id}. This is likely added while creating the HttpRequestMessage.
• Microsoft.Extensions.Http integration. This will probably be in the form of an interface + 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.
• Libraries like R9 that want to add metadata to request metrics that are important for the company. This could come from DelegatingHandler. Because the handler pipeline for all HttpClient uses 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]

Moving to 8.0 to track this as part of #84978, feel free to bump back to Future

[antonfirsov]

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]

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]

a nice middle ground would be defining either a property or an extension method on the HttpRequestOptions

I had a same thought recently. Updated the proposal, thanks for the feedback!

[antonfirsov]

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?