Skip to content

Add IRouteDiagnosticsMetadata #47492

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

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

Add IRouteDiagnosticsMetadata #47492

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

The Open Telemetry HTTP server semantic conventions records required and optional data to include with metrics measurements. A recommended tag is the route match by the HTTP request. For example, an HTTP request to app.MapGet("product/{id}", () => ...) would have the route product/{id}.

The route is available in ASP.NET Core on the matched endpoint in the RouteEndpoint.RoutePattern. However, HTTP metrics are run in Microsoft.AspNetCore.Hosting. That code doesn't have access to RouteEndpoint, which is in Microsoft.AspNetCore.Routing, because of layering.

Two possible solutions:

  • Move RouteEndpoint, RoutePattern, and all its dependencies to Microsoft.AspNetCore.Http.Abstractions and type forward. Then hosting can check if the matched endpoint is a RouteEndpoint and access the route. However, the route pattern is quite a lot of code to move.
  • Or, add new diagnostics metadata to Microsoft.AspNetCore.Http.Abstractions. The metadata has a property with a string version of the pattern. Hosting diagnostics can then get the metadata from the endpoint, and use whatever text value is present.

Proposed API

namespace Microsoft.AspNetCore.Http.Metadata;

+ public interface IRouteDiagnosticsMetadata
+ {
+     string Route { get; }
+ }

Usage Examples

if (context.MetricsEnabled)
{
    var route = httpContext.GetEndpoint()?.Metadata.GetMetadata<IRouteDiagnosticsMetadata>()?.Route;

    _metrics.RequestEnd(
        httpContext.Request.Protocol,
        httpContext.Request.Scheme,
        httpContext.Request.Method,
        httpContext.Request.Host,
        route,
        httpContext.Response.StatusCode,
        startTimestamp,
        currentTimestamp);
}

Alternative Designs

See description.

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.

@halter73
Copy link
Member

halter73 commented Mar 30, 2023

API Review Note:

  • Would we ever consider moving RouteEndpoint and RoutePattern down into Http.Abstractions.dll?
    • RoutePattern might be too big when all we need is the string for stuff like diagnostics and logging.
  • Microsoft.AspNetCore.Http.Metadata or Microsoft.AspNetCore.Routing? Let's hide it in Microsoft.AspNetCore.Http.Metadata.
  • Can we just have Routing.dll add metadata with the actual RoutePattern? If you have a reference to routing, you can just cast the Endpoint to a RouteEndpoint.
    • var route = ((RouteEndpoint)httpContext.GetEndpoint()).RoutePattern
    • Hosting needs it.
  • Do we like the name Route for the property? What about RoutePattern?
    • We don't want to get peoples hopes up that they're getting the actual RoutePattern type.
  • What about the IRouteDiagnosticsMetadata type name? There could be a lot of "Diagnostics". Would we add more to it in the future? Probably not.

API Approved!

// Microsoft.AspNetCore.Http.Abstractions.dll

namespace Microsoft.AspNetCore.Http.Metadata;

+ public interface IRouteDiagnosticsMetadata
+ {
+     string Route { get; }
+ }

@halter73 halter73 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 Mar 30, 2023
@amcasey
Copy link
Member

amcasey commented Apr 15, 2023

@JamesNK was this fixed by #46834?

@JamesNK JamesNK closed this as completed Apr 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 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

4 participants
@halter73 @JamesNK @amcasey and others