Add support for endpoint routing in route template detection

[tillig]

Feature Request

For a while now (since ~3.0) ASP.NET Core has been pretty flexible about how it allows you to define routes/patterns, much of which boils down internally to endpoint routing. For example, that's what the default WebApplicationBuilder does and it's how attribute routing (and other routing) gets added to the route table.

Currently the HttpInListener specifically gets the attribute route info from the current request and uses that to set the display name and route information. While this works fine for attribute routing, it misses cases like...

app
  .UseRouting()
  .UseEndpoints(endpoints =>
  {
    endpoints.MapControllers();
    endpoints.Map("/demo/{id}", ctx =>
    {
      ctx.Response.StatusCode = 200;
      return ctx.Response.WriteAsJsonAsync("missed");
    });
  });

The way to get routes out for endpoint routing is to get the IEndpointFeature out of the current request context:

if (context.Features.Get<IEndpointFeature>()?.Endpoint is RouteEndpoint endpoint)
{
  // endpoint.RoutePattern is the pattern - from attribute or otherwise
  // endpoint.RoutePattern.RawText is probably what the activity DisplayName should be
}
else
{
  // Fallback to path or something that isn't route pattern.
}

I noticed in the code that the MVC DiagnosticListener setup was referenced (which has actually moved to a new location since the old MVC repo is deprecated) and the current context does look like it's attached to the event so it should be available.

I also noticed the code mentioned...

// Reflection accessing: ActionDescriptor.AttributeRouteInfo.Template
// The reason to use reflection is to avoid a reference on MVC package.
// This package can be used with non-MVC apps and this logic simply wouldn't run.
// Taking reference on MVC will increase size of deployment for non-MVC apps.

...which doesn't make much sense as the OpenTelemetry.Instrumentation.AspNetCore library already references the MVC framework and is useless outside of an MVC context.

Is your feature request related to a problem?

Endpoint routing and other non-attribute route patterns are not caught by the activity naming logic.

Describe the solution you'd like:

I'd like the code to be updated to support endpoint routing, which provides a wider array of options in development.

Describe alternatives you've considered.

Right now I have to manually set the enricher to handle it, like:

services.AddOpenTelemetryTracing((builder) =>
  builder
    .AddAspNetCoreInstrumentation(b => b.Enrich = (activity, eventName, data) =>
    {
      HttpContext context;
      if (data is HttpRequest request)
      {
        context = request.HttpContext;
      }
      else if (data is HttpResponse response)
      {
        context = response.HttpContext;
      }
      else
      {
        return;
      }

      if (context.Features.Get<IEndpointFeature>()?.Endpoint is RouteEndpoint endpoint)
      {
        // We like display name like: GET /normalized-lower-case-route
        // so it's easier to differentiate and sort, but that's just our pref. You can
        // see how we get the route pattern, though.
        activity.DisplayName = $"{context.Request.Method.ToUpperInvariant()} {endpoint.RoutePattern?.RawText?.ToLowerInvariant()}";
      }
      else
      {
        // Fall back to path.
        activity.DisplayName = $"{context.Request.Method.ToUpperInvariant()} {context.Request.Path.ToString().ToLowerInvariant()}";
      }
    })
    .AddHttpClientInstrumentation()
    .AddConsoleExporter());

Additional Context

It would be nice if the enrichment mechanism was easier to wire up. Having to cast the data and check types like this is sort of painful given the ASP.NET Core instrumentation generally already knows what kind of data it's passing for each event type. Having something more strongly typed would be nice.

services.AddOpenTelemetryTracing((builder) =>
  builder
    .AddAspNetCoreInstrumentation(b =>
      {
        b.Enrich.OnStartActivity = (activity, httpRequest) =>
        {
          // Use the already-cast HttpRequest object to enrich the activity.
        }
      });

[suraciii]

Hi, is there a plan for this? Resolving span name (activity.DisplayName) from request path may produce a huge amount of span names which may break the tracing infrastructure (indexing, aggregating, usage limiting, etc.) I think we should avoid producing unbounded span name values as possible as we can, in OpenTracing, it uses HTTP {method} as the span name (see https://github.com/opentracing-contrib/csharp-netcore/blob/master/src/OpenTracing.Contrib.NetCore/AspNetCore/HostingOptions.cs#L58)

Also, IMO, it may be better if span names can respect route name from RouteNameMetadata(configured by .WithName({name})) and endpoint.DisplayName (configured by .WithDisplayName("{name}"), which has a default value as '{protocal}: {method} {pattern}')

Any ideas?

[stilettk]

The issue is still there.

We use minimal APIs, and this leads to bloating with thousands of operations like "/api/orders/xxxxxxx" instead of single "/api/orders/{id}".

[cijothomas]

@vishweshbankwar Lets take a look at this.

[stilettk]

BTW I have tried to use a workaround using Enrich from the original post, but it didn't work for me because Endpoint is always null.

I know that middlewares which want to use .GetEndpoint() muse be registered between .UseRouting() and .UseEndpoints(), but here I don't have any control over it.

[hacst]

Given with how many cases in modern ASP.NET Core this leads to high cardinality traces it would be great to have this as part of the upcoming release. I just stumbled over this when trying the RC and it unfortunately makes the instrumentation unusable for my use-case right now. Even in some existing projects with MVC I encountered things like customized error handling triggering this which resulted in high cardinality traces for failing requests. I would expect many new users trying the release for the first time once it comes out to encounter the same problem resulting in bug reports.

The PR by @vishweshbankwar looks like it would fix this but it is still marked as a draft and "stale" so I am not sure if that is blocked on something.

[vishweshbankwar]

fixed in https://github.com/open-telemetry/opentelemetry-dotnet/pull/5026