DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.45k stars 337 forks source link

OpenTelemetry ASP.NET Core Instrumentation Compatability #1575

Open qhris opened 3 months ago

qhris commented 3 months ago

Relevant dependencies

Name Version
.NET 8.0
Duende.IdentityServer 7.0.5
OpenTelemetry.Instrumentation.AspNetCore 1.8.1
OpenTelemetry.Extensions.Hosting 1.8.1

Description

We use the OpenTelemetry ASP.NET Core Instrumentation to collect traces and metrics for http server spans. Specifically the http.server.request.duration metric and the name of span is of interest.

Due to the nature of how IdentityServer handles routing with IEndpoint; all metrics are missing the http.route attribute. The relevant code that populates the attribute can be found in the OpenTelemetry repository. The inherent problem is that it relies on RouteEndpoint, but IdentityServer relies on a custom IEndpoint with a router and additional middleware that does not play well with the rest of the ASP.NET ecosystem.

The reproduction example listen below was used to generate the following screenshots.

image

Note that the marked area in the above screenshot should really measure .well-known/openid-configuration.

For traces, the display name is only GET without the route. The EndpointRoute class is again used by OpenTelemetry to populate this.

The end result is that both traces and metrics end up becoming quite useless as they do not contain the route as seen here:

image

Steps to reproduce

Add the OpenTelemetry ASP.NET Core Instrumentation to a project that uses IdentityServer and observe the traces and metrics.

I created a minimal example that can be found here that demonstrates the issue. Just follow the instructions in the README.

Potential solutions

I think the best solution would be to simply adopt ASP.NET Endpoint Routing. Should dynamic routing be required, use the EndpointDataSource functionality.

I know endpoint routing was previously discussed but I can't find the issue right now. I think it would be a good idea to revisit this and see if you can make it work with IdentityServer. This would also open up opportunities for swagger integration.

Workarounds

For now we will likely have to re-implement IdentityServerMiddleware and populate the http.route attribute and the span name manually. This is not ideal as it will require maintenance and will not be as accurate as the built-in functionality.

app.Use(async (context, next) =>
{
    var router = context.RequestServices.GetRequiredService<IEndpointRouter>();
    var endpoint = router.Find(context);
    if (endpoint is not null)
    {
        var result = await endpoint.ProcessAsync(context);
        if (result is not null)
        {
            // Not an ideal way to get the path, but it works.
            // If the path has any route parameters, they will be included which is less than ideal.
            var path = context.Request.Path.Value?.Trim('/');
            var tagsFeature = context.Features.Get<IHttpMetricsTagsFeature>();
            if (tagsFeature is not null)
            {
                tagsFeature.Tags.Add(new KeyValuePair<string, object?>("http.route", path));
            }

            // Get the activity from the context and set the display name to the path.
            var activity = System.Diagnostics.Activity.Current;
            if (activity is not null)
            {
                activity.DisplayName += $" {path}";
            }

            await result.ExecuteAsync(context);
        }

        return;
    }

    await next();
});

While this works, we really shouldn't have to do this.

AndersAbel commented 3 months ago

The discussion is here: https://github.com/DuendeSoftware/IdentityServer/issues/32

If I got the history right, IdentityServer4 had the endpoint concept before Asp.Net Core had endpoints and routing. The discussion was then if it was worth the effort to change. The conclusion was that endpoints could not replace the custom middlewares so IdSrv might as well continue using the existing solution.

Now with Open Telemetry (and possibly other features?) working with endpoint routing but not with IdSrv endpoints it might be worth to reconsider.

qhris commented 3 months ago

That's the issue I was referring to yes. I couldn't determine the reasoning behind why standard middleware or endpoint filtering couldn't achieve what you have today. From what I can see IdentityServer uses standard ASP.NET middleware?

It would be appreciated if you could reconsider or take another look at endpoint routing.