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

[ghost]

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?

[JamesNK]

This API needs to support multiple places adding tags:

1. The person creating the HttpRequestMessage
2. DelegatingHandler instances in the pipeline
3. HttpOut activity start diagnostic event

It should have a way to add and remove tags.

[antonfirsov]

@JamesNK I have updated the proposal to support these scenarios.

[JamesNK]

• TryGetCustomMetricsTags should be GetMetricsTags. It lazily creates a collection if one isn't already present.
• Remove SetCustomMetricsTags.

Look at how much simpler it is:

public class EnricherHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var tags = request.Options.GetMetricsTags();
        tags.Add(new KeyValuePair<string, object>(name, value));
        return await base.SendAsync(request, cancellationToken);
    }
}

Almost as simple as if there were a collection property on HttpRequestMessage:

public class EnricherHandler : DelegatingHandler
{
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        request.MetricsTags.Add(new KeyValuePair<string, object>(name, value));
        return await base.SendAsync(request, cancellationToken);
    }
}

[antonfirsov]

Then the method would be responsible for choosing the underlying collection type which I wanted to avoid, but I will reconsider this option. I wonder what do others think?

[JamesNK]

Why do you want to avoid that? List<KeyValuePair<string, object?>> is fine.

[antonfirsov]

I wanted HttpRequestOptionsExtensions in System.Net.Http to do the bare minimum: get/set the keys. Using these methods middlewares could implement their GetMetricsTags() extension with the semantics they prefer. Note that this would be a very clean and simple set of responsibilities that could be applied to other key-value pairs in the future making HttpRequestOptionsExtensions a universal central utility to manage well known pairs in a straightforward and safe way.

Since I received a lot of pushback on this approach, I updated the proposal to expose a single GetCustomMetricsTags() method as you are suggesting.

[bartonjs]

Video

Generally looks good as proposed. Ideally, this method will apply these tags to all metrics that are produced with "the request" (duration, size, etc) but probably not with "the client" (total number of requests, etc). If it's just cherry-picking individual metrics then it probably shouldn't be public API, as 3rd party callers wouldn't know when they can/can't add things sensibly.

namespace System.Net.Http;

// Consider a Metrics-specific HttpRequestOptionsMetricsExtensions
public static class HttpRequestOptionsExtensions
{
    // Returns the value assigned to the well known key if it's present:
    // HttpRequestOptionsKey<ICollection<KeyValuePair<string, object?>>>("CustomMetricsTags")
    // Creates a new collection if it doesn't. Throws if a collection is present but readonly.
    // Alternative name ideas: GetMetricsEnrichmentTags, GetEnrichmentTags
    public static ICollection<KeyValuePair<string, object?>> GetCustomMetricsTags(this HttpRequestOptions options);
}

[bartonjs]

Video

We moved the extension method to a static AddCallback on the context type. Otherwise, looks good as proposed.

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

    public static void AddCallback(HttpRequestOptions options, Action<HttpMetricsEnrichmentContext> callback);
}

[JamesNK]

public static void AddCallback(HttpRequestOptions options, Action<HttpMetricsEnrichmentContext> callback);

The add callback method is no longer an extension method. Part of the reason to choose HttpRequestOptions as the this argument for it to hang off was to hide it away. That's no longer a concern.

I propose a small change to make the AddCallback method take HttpRequestMessage as an argument instead of HttpRequestOptions:

public static void AddCallback(HttpRequestMessage message, Action<HttpMetricsEnrichmentContext> callback);

Reasons:

1. It's more user-friendly. HttpRequestMessage makes it slightly more obvious what to pass to the method (more people are familiar with HttpRequestMessage than HttpRequestOptions)
2. In the future, if you decide to change how callbacks are stored, such as to a field on the request message, it is easy to do because you already have the HttpRequestMessage reference.
3. Related to the point above, accessing HttpRequestMessage.Options automatically allocates the collection if it wasn't already present. There would be no way to ever optimize that away with the current shape.

[stephentoub]

+1 to James' comment

[JamesNK]

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]

@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]

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.