[AspNetCoreInstrumentation] Incorrect value for http.route when using conventional routing

[alanwest]

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.

[TimothyMothra]

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
  

[stevenmccormack]

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

[DanielLaberge]

This regression needs some attention. I agree with the OP that the http.route attribute is very important.

Thanks for the workaround @stevenmccormack

[vanbukin]

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

[timohermans]

Any update on this?

[Slepoyi]

Using both conventional routing and Route attribute works fine but then what's the point of using conventional routing