Skip to content

[Instrumentation.AspNetCore] Fix http.route for HTTP server spans #3338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RassK wants to merge 16 commits into
base: main
Choose a base branch
from
Open

[Instrumentation.AspNetCore] Fix http.route for HTTP server spans #3338

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fc78c0e
fix route attribute logic
RassK Oct 21, 2025
d3ab87a
update
RassK Oct 23, 2025
103f520
fix traces and update routes
RassK Nov 4, 2025
cee12f9
revert routingtests and update cases
RassK Nov 4, 2025
edc8a72
update metric tests
RassK Nov 4, 2025
b46f36e
simplify routepattern for page
RassK Nov 4, 2025
ed8680a
typo fix
RassK Nov 4, 2025
5361b9f
fix if statement
RassK Nov 4, 2025
92d1c9c
update docs
RassK Nov 5, 2025
512256b
Merge branch 'main' into aspnetcore-route-fix
RassK Nov 5, 2025
c75d643
fix line length
RassK Nov 5, 2025
bb380b9
fix md012
RassK Nov 5, 2025
833fc39
remove space
RassK Nov 5, 2025
d57c244
fix .net10
RassK Nov 5, 2025
49b93a1
update changelog
RassK Nov 6, 2025
5bfe512
fix line length
RassK Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,12 @@ public void OnStopActivity(Activity activity, object? payload)
var response = context.Response;

#if !NETSTANDARD
var routePattern = (context.Features.Get<IExceptionHandlerPathFeature>()?.Endpoint as RouteEndpoint ??
context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(routePattern))
var endpoint = context.Features.Get<IExceptionHandlerPathFeature>()?.Endpoint as RouteEndpoint
?? context.GetEndpoint() as RouteEndpoint;

if (endpoint != null)
{
TelemetryHelper.RequestDataHelper.SetActivityDisplayName(activity, context.Request.Method, routePattern);
activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
activity.SetRouteAttributeTag(endpoint, context.Request);
}
#endif

Expand Down
9 changes: 5 additions & 4 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,12 @@ public static void OnStopEventWritten(string name, object? payload)

#if NET
// Check the exception handler feature first in case the endpoint was overwritten
var route = (context.Features.Get<IExceptionHandlerPathFeature>()?.Endpoint as RouteEndpoint ??
context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(route))
var endpoint = context.Features.Get<IExceptionHandlerPathFeature>()?.Endpoint as RouteEndpoint
?? context.GetEndpoint() as RouteEndpoint;

if (endpoint != null)
{
tags.Add(new KeyValuePair<string, object?>(SemanticConventions.AttributeHttpRoute, route));
tags.AddRouteAttribute(endpoint, context.Request);
}
#endif
if (context.Items.TryGetValue(ErrorTypeHttpContextItemsKey, out var errorType))
Expand Down
100 changes: 100 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#if !NETSTANDARD

using System.Diagnostics;
using System.Text;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation;

internal static class RouteAttributeHelper
{
public static void AddRouteAttribute(this TagList tags, RouteEndpoint endpoint, HttpRequest request)
{
var routePattern = GetRoutePattern(endpoint.RoutePattern, request.RouteValues);

if (!string.IsNullOrEmpty(routePattern))
{
tags.Add(new KeyValuePair<string, object?>(SemanticConventions.AttributeHttpRoute, routePattern));
}
}

public static void SetRouteAttributeTag(this Activity activity, RouteEndpoint endpoint, HttpRequest request)
{
var routePattern = GetRoutePattern(endpoint.RoutePattern, request.RouteValues);

if (!string.IsNullOrEmpty(routePattern))
{
TelemetryHelper.RequestDataHelper.SetActivityDisplayName(activity, request.Method, routePattern);
activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
}
}

private static string GetRoutePattern(RoutePattern routePattern, RouteValueDictionary routeValues)
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a very large suite of unit tests to ensure every possible route is correctly converted into a string.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that there are probably loads of assumptions (if a general case is working).

Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but I think an empty string route should resolve to /. An empty string by itself is confusing. We're already doing that with http.route tag in metrics based on customer feedback.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it should be removed completely if it's empty (empty means no route found), since:

[5] http.route: MUST NOT be populated when this is not supported by the HTTP server framework as the route attribute should have low-cardinality and the URI path can NOT substitute it. SHOULD include the application root if there is one.

src: https://opentelemetry.io/docs/specs/semconv/registry/attributes/http/

Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always runs, right? Should it only run if the route needs to change? i.e. there are area/controller/action/page attributes and a value is present in the requests route values collection.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. It would be a nice optimization. Also connected to #3338 (comment) (so it runs only if the framework provides the values).

{
if (routePattern.PathSegments.Count == 0)
{
// RazorPage default route
if (routePattern.Defaults.ContainsKey("page"))
{
return routePattern.Defaults["page"]?.ToString()?.Trim('/')
?? string.Empty;
}

return string.Empty;
}

var sb = new StringBuilder();

foreach (var segment in routePattern.PathSegments)
{
foreach (var part in segment.Parts)
{
if (part is RoutePatternLiteralPart literalPart)
{
sb.Append(literalPart.Content);
sb.Append('/');
}
else if (part is RoutePatternParameterPart parameterPart)
{
switch (parameterPart.Name)
{
case "area":
case "controller":
case "action":
routePattern.RequiredValues.TryGetValue(parameterPart.Name, out var parameterValue);
if (parameterValue != null)
{
sb.Append(parameterValue);
sb.Append('/');
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not look for separator part for adding separator?

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesNK there are no separator parts although routePattern is "{controller=ConventionalRoute}/{action=Default}/{id?}". Only 3 times RoutePatternParameterPart.

According to the doc RoutePatternSeparatorPart is for something else.

break;
}

goto default;
default:
if (!parameterPart.IsOptional ||
(parameterPart.IsOptional && routeValues.ContainsKey(parameterPart.Name)))
{
sb.Append('{');
sb.Append(parameterPart.Name);
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there are default values? Or it is catch all? Or has constraints? Or has optional path extension.

There are also complex segments. Are those being handled correctly? See https://learn.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-9.0#complex-segments

Maybe these scenarios are already handled correctly, but unit tests to verify are important.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the test infra is doing a loads of assumptions. In that case it needs to be rewritten so HttpContext is correctly populated by the framework and not by the assumptions (doing manual construction).

sb.Append('}');
sb.Append('/');
}

break;
}
}
}
}

// Remove the trailing '/'
return sb.ToString(0, sb.Length - 1);
}
}

#endif
46 changes: 23 additions & 23 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,26 @@
"httpMethod": "GET",
"path": "/",
"expectedStatusCode": 200,
"currentHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"expectedHttpRoute": "ConventionalRoute/Default/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "ConventionalRoute/Default"
},
{
"name": "Non-default action with route parameter and query string",
"testApplicationScenario": "ConventionalRouting",
"httpMethod": "GET",
"path": "/ConventionalRoute/ActionWithStringParameter/2?num=3",
"expectedStatusCode": 200,
"currentHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"expectedHttpRoute": "ConventionalRoute/ActionWithStringParameter/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "ConventionalRoute/ActionWithStringParameter/{id}"
},
{
"name": "Non-default action with query string",
"testApplicationScenario": "ConventionalRouting",
"httpMethod": "GET",
"path": "/ConventionalRoute/ActionWithStringParameter?num=3",
"expectedStatusCode": 200,
"currentHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"expectedHttpRoute": "ConventionalRoute/ActionWithStringParameter/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "ConventionalRoute/ActionWithStringParameter"
},
{
"name": "Not Found (404)",
Expand All @@ -42,7 +42,7 @@
"path": "/SomePath/SomeString/2",
"expectedStatusCode": 200,
"currentHttpRoute": null,
"expectedHttpRoute": "SomePath/{id}/{num:int}"
"expectedHttpRoute": "SomePath/{id}/{num}"
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The route constraint is being lost

Copy link
Contributor Author

@RassK RassK Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional change:

pros:

  • produces similar routes as asp.net mvc5
  • reduces cardinality
  • simplifies the template

negs:

  • no visibility into same routes but different constraints (possibly very rare case)

},
{
"name": "Path that does not match parameter constraint",
Expand All @@ -59,26 +59,26 @@
"httpMethod": "GET",
"path": "/MyArea",
"expectedStatusCode": 200,
"currentHttpRoute": "{area:exists}/{controller=ControllerForMyArea}/{action=Default}/{id?}",
"expectedHttpRoute": "{area:exists}/ControllerForMyArea/Default/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "MyArea/ControllerForMyArea/Default"
},
{
"name": "Area using `area:exists`, non-default action",
"testApplicationScenario": "ConventionalRouting",
"httpMethod": "GET",
"path": "/MyArea/ControllerForMyArea/NonDefault",
"expectedStatusCode": 200,
"currentHttpRoute": "{area:exists}/{controller=ControllerForMyArea}/{action=Default}/{id?}",
"expectedHttpRoute": "{area:exists}/ControllerForMyArea/NonDefault/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "MyArea/ControllerForMyArea/NonDefault"
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you just modifying area, controller and page? Isn't the expected route "MyArea/ControllerForMyArea/NonDefault/{id?}". Or is it removed because it's optional?

I don't think removing it has any value. It's still a dynamic placeholder in the route and should be present according to the spec.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? is a constraint, so connected to #3338 (comment)

for ? it produces 2 perfectly valid routes if the route is using optional path arg and supports both cases (with and without arg).

},
{
"name": "Area w/o `area:exists`, default controller/action",
"testApplicationScenario": "ConventionalRouting",
"httpMethod": "GET",
"path": "/SomePrefix",
"expectedStatusCode": 200,
"currentHttpRoute": "SomePrefix/{controller=AnotherArea}/{action=Index}/{id?}",
"expectedHttpRoute": "SomePrefix/AnotherArea/Index/{id?}"
"currentHttpRoute": null,
"expectedHttpRoute": "SomePrefix/AnotherArea/Index"
},
{
"name": "Default action",
Expand Down Expand Up @@ -132,17 +132,17 @@
"httpMethod": "GET",
"path": "/",
"expectedStatusCode": 200,
"currentHttpRoute": "",
"expectedHttpRoute": "/Index"
"currentHttpRoute": null,
"expectedHttpRoute": "Index"
},
{
"name": "Index page",
"testApplicationScenario": "RazorPages",
"httpMethod": "GET",
"path": "/Index",
"expectedStatusCode": 200,
"currentHttpRoute": "Index",
"expectedHttpRoute": "/Index"
"currentHttpRoute": null,
"expectedHttpRoute": "Index"
},
{
"name": "Throws exception",
Expand All @@ -151,7 +151,7 @@
"path": "/PageThatThrowsException",
"expectedStatusCode": 500,
"currentHttpRoute": "PageThatThrowsException",
"expectedHttpRoute": "/PageThatThrowsException"
"expectedHttpRoute": "PageThatThrowsException"
},
{
"name": "Static content",
Expand All @@ -169,7 +169,7 @@
"path": "/MinimalApi",
"expectedStatusCode": 200,
"currentHttpRoute": null,
"expectedHttpRoute": "/MinimalApi"
"expectedHttpRoute": "MinimalApi"
},
{
"name": "Action with parameter",
Expand All @@ -178,7 +178,7 @@
"path": "/MinimalApi/123",
"expectedStatusCode": 200,
"currentHttpRoute": null,
"expectedHttpRoute": "/MinimalApi/{id}"
"expectedHttpRoute": "MinimalApi/{id}"
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a minimal api route has a parameter called action? Will it be rewritten? There is nothing stopping a minimal API having a parameter with that name and it isn't a static part of the route.

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently yes, unless there is a framework marker so we can check if it was included by the framework.

},
{
"name": "Action without parameter (MapGroup)",
Expand All @@ -188,7 +188,7 @@
"path": "/MinimalApiUsingMapGroup",
"expectedStatusCode": 200,
"currentHttpRoute": null,
"expectedHttpRoute": "/MinimalApiUsingMapGroup/"
"expectedHttpRoute": "MinimalApiUsingMapGroup"
Copy link
Contributor

@JamesNK JamesNK Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove preceeding /?

Copy link
Contributor Author

@RassK RassK Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 ways, either to include / always or not to include. The same route must not produce 2 different versions.
So if there is no extra meaning (and a need), then there is no point to include it. Same in ASP.NET MVC5.

},
{
"name": "Action with parameter (MapGroup)",
Expand All @@ -198,7 +198,7 @@
"path": "/MinimalApiUsingMapGroup/123",
"expectedStatusCode": 200,
"currentHttpRoute": null,
"expectedHttpRoute": "/MinimalApiUsingMapGroup/{id}"
"expectedHttpRoute": "MinimalApiUsingMapGroup/{id}"
},
{
"name": "Exception Handled by Exception Handler Middleware",
Expand All @@ -207,6 +207,6 @@
"path": "/Exception",
"expectedStatusCode": 500,
"currentHttpRoute": null,
"expectedHttpRoute": "/Exception"
"expectedHttpRoute": "Exception"
}
]
Loading