opentelemetry-dotnet-contrib
opentelemetry-dotnet-contrib copied to clipboard
[AspNetCoreInstrumentation] Incorrect value for `http.route` when using conventional routing
Bug
The http.route attribute is one of the more important HTTP attributes because users commonly wish to facet their metric and span data on http.route in order to understand the performance characteristics of specific endpoints in their applications.
ASP.NET Core apps that use conventional routing are not well supported. With conventional routing, it is common that a single route template matches many distinct routes. For these applications the instrumentation sets http.route to the value of the route template and not to a value that represents the actual route invoked. In turn, this means users lose the ability to understand the performance of specific endpoints.
This bug was introduced in open-telemetry/opentelemetry-dotnet#5026 to conform with the behavior of ASP.NET Core 8 which natively emits the http.server.request.duration metric. We wanted to avoid a surprise change in behavior for users migrating from earlier versions of .NET to .NET 8. Fixing this bug should be done in coordination with the ASP.NET Core team.
Reproduce
Run:
dotnet new mvc
The following is abbreviated, but Program.cs will look something like:
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllersWithViews();
var app = builder.Build();
app.UseRouting();
app.MapControllerRoute(
name: "default",
pattern: "{controller=Home}/{action=Index}/{id?}");
app.Run();
The application's HomeController contains multiple actions - e.g., Index, Privacy, etc. Invoking any of these endpoints will result in a metric and span with the http.route attribute set to {controller=Home}/{action=Index}/{id?}.
Suggested fix
Well-known route parameters (e.g., controller, action) should be replaced by their actual values. Using the HttpContext.GetRouteData() method is one way to get the actual route parameter values.
One potential option would be to apply the fix for .NET 6 and .NET 7 users gated by an environment variable.
I hit this today. Sharing some more concrete examples:
I configured my test app same as Alan:
app.MapControllerRoute(
name: "default",
pattern: "{controller=Home}/{action=Index}/{id?}");
My test app has a HomeController and CustomerController.
Regardless which controller is invoked, The url.path shows the correct Controller/Action. The http.route shows the configured pattern.
-
https://localhost:44311/Home/Index
Activity.ActivitySourceName: Microsoft.AspNetCore Activity.DisplayName: GET {controller=Home}/{action=Index}/{id?} Activity.Kind: Server Activity.Tags: server.address: localhost server.port: 44311 http.request.method: GET url.scheme: https url.path: / network.protocol.version: 2 http.route: {controller=Home}/{action=Index}/{id?} http.response.status_code: 200 -
https://localhost:44311/Customer/Index
Activity.ActivitySourceName: Microsoft.AspNetCore Activity.DisplayName: GET {controller=Home}/{action=Index}/{id?} Activity.Kind: Server Activity.Tags: server.address: localhost server.port: 44311 http.request.method: GET url.scheme: https url.path: /Customer/Index network.protocol.version: 2 http.route: {controller=Home}/{action=Index}/{id?} http.response.status_code: 200
I am facing the exact same issue for all requests in net8. As a workaround I found the following:
.AddAspNetCoreInstrumentation(
o => {
o.EnrichWithHttpResponse = (activity, response) => {
var template = response.HttpContext.Request.Path;
activity.DisplayName = template;
activity.SetTag("http.route", template);
};
}
)
This regression needs some attention. I agree with the OP that the http.route attribute is very important.
Thanks for the workaround @stevenmccormack
@DanielLaberge @stevenmccormack
The mentioned workaround is bad and shouldn't be used in production.
Not only is the full path formed from a combination of PathBase and Path, but the Path itself can contain any values.
Say, if some bots start going through your admin panel, all their /wp-admin.php, /login.php, /admin/login and other queries in attempts to attack some CMS will happily end up in the label values of the metrics.
This leads to an uncontrollable growth in cardinality.
In practice, this will result in the database of something like Prometheus not being able to handle such high cardinality of metrics and will consequently fail.
Any update on this?
Using both conventional routing and Route attribute works fine but then what's the point of using conventional routing
This is my fix until an official solution is released.
tracing.AddAspNetCoreInstrumentation(options =>
{
options.EnrichWithHttpResponse = (activity, response) =>
{
var endpoint = response.HttpContext.GetEndpoint();
if (endpoint is RouteEndpoint routeEndpoint)
{
var descriptor = routeEndpoint.Metadata.GetMetadata<ControllerActionDescriptor>();
if (descriptor is not null)
{
var controller = descriptor.ControllerName;
var action = descriptor.ActionName;
var pathParameters = descriptor.Parameters
.Where(p => p.BindingInfo is null || p.BindingInfo.BindingSource?.Id == "Path")
.Select(p => $"{{{p.Name}}}");
var route = string.Join("/", [controller, action, .. pathParameters]);
activity.DisplayName = route;
activity.SetTag("http.route", route);
}
}
};
});
This will include path parameters in the http.route but intentionally exclude query parameters as these would add to much overhead. Parameters are included by name and does not use the actual value.
So for instance, the URL https://mydomain.com/Products/Overview/123?order=desc&search=GPU will update display name and http.route to be Products/Overview/{id}.
This is my fix until an official solution is released.
tracing.AddAspNetCoreInstrumentation(options => { options.EnrichWithHttpResponse = (activity, response) => { var endpoint = response.HttpContext.GetEndpoint(); if (endpoint is RouteEndpoint routeEndpoint) { var descriptor = routeEndpoint.Metadata.GetMetadata<ControllerActionDescriptor>(); if (descriptor is not null) { var controller = descriptor.ControllerName; var action = descriptor.ActionName;
var pathParameters = descriptor.Parameters .Where(p => p.BindingInfo is null || p.BindingInfo.BindingSource?.Id == "Path") .Select(p => $"{{{p.Name}}}"); var route = string.Join("/", [controller, action, .. pathParameters]); activity.DisplayName = route; activity.SetTag("http.route", route); } } };}); This will include path parameters in the
http.routebut intentionally exclude query parameters as these would add to much overhead. Parameters are included by name and does not use the actual value.So for instance, the URL
https://mydomain.com/Products/Overview/123?order=desc&search=GPUwill update display name andhttp.routeto beProducts/Overview/{id}.
Please give me your full code in program.cs and list of installed libraries. I tried your code sample but no success. Thank you!
@aminotran It goes in the AddOpenTelemetry service registration, so in Program.cs you would put it here:
using Microsoft.AspNetCore.Mvc.Controllers;
using OpenTelemetry.Trace;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenTelemetry().WithTracing(tracing =>
{
tracing.AddAspNetCoreInstrumentation(options =>
{
options.EnrichWithHttpResponse = (activity, response) =>
{
var endpoint = response.HttpContext.GetEndpoint();
if (endpoint is RouteEndpoint routeEndpoint)
{
var descriptor = routeEndpoint.Metadata.GetMetadata<ControllerActionDescriptor>();
if (descriptor is not null)
{
var controller = descriptor.ControllerName;
var action = descriptor.ActionName;
var pathParameters = descriptor.Parameters
.Where(p => p.BindingInfo is null || p.BindingInfo.BindingSource?.Id == "Path")
.Select(p => $"{{{p.Name}}}");
var route = string.Join("/", [controller, action, .. pathParameters]);
activity.DisplayName = route;
activity.SetTag("http.route", route);
}
}
};
});
});
+ other service registrations and middleware configurations.
Order doesn't really matter since it is part of a service registration. Also, I would probably move this code into a service defaults project, particularly if you are running .NET Aspire.
📝 NOTE: I am using some C# 12.0 features, so if you can't use that for whatever reason, you have to replace the collection expressions and spread syntax with something compatible with your code.
@aminotran It goes in the
AddOpenTelemetryservice registration, so inProgram.csyou would put it here:using Microsoft.AspNetCore.Mvc.Controllers; using OpenTelemetry.Trace;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenTelemetry().WithTracing(tracing => { tracing.AddAspNetCoreInstrumentation(options => { options.EnrichWithHttpResponse = (activity, response) => { var endpoint = response.HttpContext.GetEndpoint(); if (endpoint is RouteEndpoint routeEndpoint) { var descriptor = routeEndpoint.Metadata.GetMetadata<ControllerActionDescriptor>(); if (descriptor is not null) { var controller = descriptor.ControllerName; var action = descriptor.ActionName;
var pathParameters = descriptor.Parameters .Where(p => p.BindingInfo is null || p.BindingInfo.BindingSource?.Id == "Path") .Select(p => $"{{{p.Name}}}"); var route = string.Join("/", [controller, action, .. pathParameters]); activity.DisplayName = route; activity.SetTag("http.route", route); } } }; });});
- other service registrations and middleware configurations. Order doesn't really matter since it is part of a service registration. Also, I would probably move this code into a service defaults project, particularly if you are running .NET Aspire.
📝 NOTE: I am using some C# 12.0 features, so if you can't use that for whatever reason, you have to replace the collection expressions and spread syntax with something compatible with your code.
I'm try but it not worked. Slution of me:
- Use attribute Route for Controller/Action example => [Route("[area]/[controller]/[action]")] or [Route("[controller]/[action]")] but it will config all controller/Action and between action default Index of controller Thank you!