Skip to content
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

Add IHttpMetricsTagsFeature and IConnectionMetricsTagsFeature #47493

Closed
JamesNK opened this issue Mar 30, 2023 · 3 comments
Closed

Add IHttpMetricsTagsFeature and IConnectionMetricsTagsFeature #47493

JamesNK opened this issue Mar 30, 2023 · 3 comments
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@JamesNK
Copy link
Member

JamesNK commented Mar 30, 2023

Background and Motivation

ASP.NET Core hosting counters record the number of current requests and duration. There isn't a way for a framework, middleware or app to enrich these counters with tags per-request. For example, ASP.NET Core might want to add a tag to the HTTP server metrics that specifies whether there was an unhandled exception with that request.

This issue outlines an ASP.NET Core feature for adding tags to metrics. It's similar to the existing IHttpActivityFeature. For an example of enriching the request activity with tags, see Activity Enrichment in ASP.NET Core 6.0.

Proposed API

namespace Microsoft.AspNetCore.Http.Features;

+ public interface IHttpMetricsTagsFeature
+ {
+     IList<KeyValuePair<string, object?> Tags { get; }
+ }

namespace Microsoft.AspNetCore.Connections.Features;

+ public interface IConnectionMetricsTagsFeature
+ {
+     IList<KeyValuePair<string, object?>> Tags { get; }
+ }

Implementations are internal. Features are automatically set on the HttpContext or TransportConnection if metrics is enabled (aka someone is listening).

Usage Examples

The feature is used by frameworks, middleware or apps when they want to enrich HTTP metrics counters (or connection counters) with extra tags. In the sample below, middleware catches unhandled exceptions, then uses the feature to add the exception type name to metrics.

app.Use(async (context, next) =>
{
    try
    {
        await next.Invoke();
    }
    catch (Exception ex)
    {
        var tagsFeature = context.Features.Get<IHttpMetricsTagsFeature>();
        if (tagsFeature != null)
        {
            tagsFeature.Tags.Add(new KeyValuePair<string, object?>("exception-type", ex.GetType().Name));
        }
    }
});

ASP.NET Core hosting uses the feature when recording counters.

public class HostingMetrics
{
    public void RequestEnd(HttpContext context, long startTimestamp, long currentTimestamp)
    {
        var duration = new TimeSpan((long)(HostingApplicationDiagnostics.TimestampToTicks * (currentTimestamp - startTimestamp)));

        var tags = new TagList();
        // Known tag values.
        tags.Add("scheme", context.Request.Scheme);
        tags.Add("method", context.Request.Method);
        
        // Framework, middleware or app define tags.
        var tagsFeature = context.Features.Get<IHttpMetricsTagsFeature>();
        if (tagsFeature != null)
        {
            foreach (var tag in tagsFeatures.Tags)
            {
                tags.Add(tag);
            }
        }

        _requestDuration.Record(duration.TotalMilliseconds, tags);
    }
}

Alternative Designs

One feature could be used for HTTP request and connection.

I believe that approach means it wouldn't be possible to get the connection metrics tags feature from a request, because the request feature of the same type would always be returned.

Risks

@JamesNK JamesNK added area-runtime api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 30, 2023
@ghost
Copy link

ghost commented Mar 30, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@JamesNK
Copy link
Member Author

JamesNK commented Apr 11, 2023

API Review Note:

  • Why add two similar feature interfaces? Could one be reused? Two are added because the feature collection uses the type as the key. Different types are required to be able to access connection tags from a request.
  • IList<KeyValuePair<string, object?> vs ICollection<KeyValuePair<string, object?> vs AddTag/SetTag/ClearTag methods
    • If we need add/set/remove/clear methods then the feature is basically a collection. Might as well expose a collection.
    • Final choice is ICollection<...> because accessing via index isn't required.
  • Indicating metrics is disabled: IsEnabled property with false vs a null feature.
    • Final choice is a null feature to be consistent with IHttpActivityFeature.

API Approved!

namespace Microsoft.AspNetCore.Http.Features;

+ public interface IHttpMetricsTagsFeature
+ {
+     ICollection<KeyValuePair<string, object?> Tags { get; }
+ }

namespace Microsoft.AspNetCore.Connections.Features;

+ public interface IConnectionMetricsTagsFeature
+ {
+     ICollection<KeyValuePair<string, object?>> Tags { get; }
+ }

@JamesNK JamesNK added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 11, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Apr 13, 2023

Added in #46834

@JamesNK JamesNK closed this as completed Apr 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

3 participants
@JamesNK @amcasey and others