ASP.Net Core endpoint response time collecting middleware uses actual path as labels

[kodfodrasz]

Nexogen.Libraries.Metrics.Prometheus.AspCore.HttpMetricsMiddleware measures endpoint response times and collects them labelled by http methods, response codes and endpoint paths.

However these paths are actual paths (eg.: order/321item/123), not logical ones (eg.: order/{orderId}/item/{itemId}) identifiing the controller in MVC, thus if [PathParam] is used on a controller this causes an explosion of labels by each actual call. This has storage and performance costs in Prometheus.

The middleware level approach currently used doesn't have acces to MVC routing, so the logical path is not available, only actual HTTP request path.

This is usually not desired, so this measurement should either be moved to MVC level, or an MVC level alternative needs to be added, so the consumers can weigh the costs and benefits of each approach.

[gideonkorir]

From what I see, mvc uses routing for mapping requests to controllers. Look at the code at RoutingMiddleware I was able to figure out a hacky way of doing this something along this:

            app.Use(next =>
            {
                RequestDelegate requestDelegate = async ctx =>
                   {
                        await next(ctx);
                       var feature = ctx.Features.Get<IRoutingFeature>();
                       var lastRouter = feature?.RouteData?.Routers.FindLast();
                       string path = ctx.Request.Path;

                       if (lastRouter != null)
                       {
                           await ctx.Response.WriteAsync("Router found");
                            path = lastRouter.ParsedTemplate.TemplateText;
                       }
                       else
                       {
                           await ctx.Response.WriteAsync("Router not found");
                       }
                       await ctx.Response.WriteAsync($"Settled on prom path: {path} for http path: {ctx.Request.Path}");

                   };
                return requestDelegate;
            });

                   //find last extension method
        public static RouteBase FindLast(this IList<IRouter> routers)
        {
            if (routers == null || routers.Count == 0)
                return null;
            for(int i=routers.Count - 1; i >= 0; i--)
            {
                if (routers[i] is RouteBase)
                    return routers[i] as RouteBase;
            }
            return null;
        }

Need to test this more but it works with mvc & vanilla routing. I can put in a PR with the fix if this sounds sensible

[kodfodrasz]

The solution provided by @gideonkorir does work correctly when using manual routing, but when using MVC AttributeRouting the problem still exists.

Version 2.6.0 contains the partial solution already.

After some investigation it seems that it is not possible to Access the served path template used by MVC attribute routing from a middleware outside MVC. If a solution is possible it seems it must be implemented as a plugin to MVC. I think that in this case a new type of metric collector should be introduced with MVC dependencies, and a different middleware without mvc dependencies also needs to be provided.

It must be investigated how the metric collection setup API should be modified to properly communicate this. Probably the metric collection and the metric exposal.

[gideonkorir]

@kodfodrasz would it be useful to add a PrometheusMvcActionFilter that would put the actual route information in HttpContext.Items using a known key e.g. prometheus.mvcpath? The middleware would check for the key in items and use that instead of the actual Http path?

My initial investigations show that I can access the AttributeRouteInfo using the path `ActionExecutingContext.ActionDescriptor.AttributeRouteInfo. I now need to check that I can access routes based on conventional based mapping.

I've dug through the Mvc code, the main action occurring in MvcRouteHandler and there is no way to do this transparently, we'll need an extension method on AddMvc() and/or AddMvcCore() in order to make metrics better.

[kodfodrasz]

@gideonkorir if I understand correctly, then this approach would also have the benefit that non-mvc and mvc routing based collectors could be used simultenously and they could collaborate in providing the best available path for reporting in case somebody uses both in a single application.

[gideonkorir]

@kodfodrasz yes that is the idea. As an example, Asp.Net core is Owin compatible and that means I can use it to host NancyFx and/or other frameworks. Instead of us changing our CollectMetricsMiddleware they'd add an integration point specific to their framework.

In this specific scenario, I'd propose we have a Nexogen.Libraries.Metrics.Prometheus.AspCore.Mvc integration package & that would have Mvc specific integration.

What do you think?

[gideonkorir]

Done some simple investigations and so far:

1. Conventional based routing in Mvc uses Microsoft.AspnetCore.Routing.Route to handle the dispatch to the controller. We can get that off the IRoutingFeature similar to what I'm currently doing with vanilla routing. I just need to check for the Route object in RouteData.Routers collection.
2. Attribute based routing doesn't expose the route template and I haven't found a way to extract it without using an action filter and extracting the information from there.

[jbrekle]

I use 2.6 release, and still see all requests with full URLs, but that might be due to the fact that I don't use ASP routing at all, but a middleware only, that does some redirects to other services. Anyways, could there be a parameter or a separate UsePrometheusAspCollection method, so that i can avoid ASP integration somehow? I could make a pull request.

[gideonkorir]

@jbrekle I'm a bit confused, you are using asp middleware so why do you need to avoid asp integration? I already did a PR that allows you to create the metrics object outside the AddPrometheus extension method.

[jbrekle]

I just want to avoid the request collection but not the endpoint serving. The AddPrometheus method is fine, Just the UsePrometheus method should not couple the CollectMetricsMiddleware with the ServeMetricsMiddleware. Or ServeMetricsMiddleware becomes public and I can configure it myself.

[ahoka]

@jbrekle We plan to do that, but it will be a breaking change calling for 3.0. I'm enjoying my PTO, so I think I can do this change soon.

[jbrekle]

i dont think it needs to be breaking. can you have a look at PR #27 ?

[ahoka]

I think that potentially causing a time series "explosion" by generating an endless number of labels is a serious enough bug to disable it by default.

From the documentation[1]:

Remember that every unique combination of key-value label pairs represents a new time series, which can dramatically increase the amount of data stored. Do not use labels to store dimensions with high cardinality (many different label values), such as user IDs, email addresses, or other unbounded sets of values.

[1] - https://prometheus.io/docs/practices/naming/