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

RassK · 2025-10-23T10:46:19Z

Follow up to #3160

Changes

Normalizes route template according to spec.

Replaces controller, action, area tokens.
Removes constraints.
Removes unused optional parameters

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes

github-actions · 2025-10-31T03:31:28Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

codecov · 2025-11-04T12:47:21Z

Codecov Report

❌ Patch coverage is 78.04878% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.77%. Comparing base (a5f3fc0) to head (5bfe512).
⚠️ Report is 72 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
....AspNetCore/Implementation/RouteAttributeHelper.cs	84.84%	5 Missing ⚠️
...AspNetCore/Implementation/HttpInMetricsListener.cs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3338      +/-   ##
==========================================
+ Coverage   68.61%   68.77%   +0.16%     
==========================================
  Files         431      422       -9     
  Lines       16429    16410      -19     
==========================================
+ Hits        11272    11286      +14     
+ Misses       5157     5124      -33

Flag	Coverage Δ
unittests-Instrumentation.AspNet	`77.92% <ø> (-0.23%)`	⬇️
unittests-Instrumentation.AspNetCore	`72.82% <78.04%> (+1.13%)`	⬆️
unittests-Instrumentation.Cassandra	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs	`73.28% <100.00%> (-0.19%)`	⬇️
...AspNetCore/Implementation/HttpInMetricsListener.cs	`0.00% <0.00%> (ø)`
....AspNetCore/Implementation/RouteAttributeHelper.cs	`84.84% <84.84%> (ø)`

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestResult.cs

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

RassK · 2025-11-04T16:40:17Z

I'll recheck http route tests readme and update the changelog. So after that, this change should be ready to go.

# Conflicts: # src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

Kielek · 2025-11-05T16:59:25Z

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net10.0.md

 | :broken_heart: | ConventionalRouting | [Non-default action with query string](#metrics__conventionalrouting-non-default-action-with-query-string) |
 | :green_heart: | ConventionalRouting | [Not Found (404)](#metrics__conventionalrouting-not-found-404) |
-| :green_heart: | ConventionalRouting | [Route template with parameter constraint](#metrics__conventionalrouting-route-template-with-parameter-constraint) |
+| :broken_heart: | ConventionalRouting | [Route template with parameter constraint](#metrics__conventionalrouting-route-template-with-parameter-constraint) |


There is some green_heart to broken_heart changes across all net targets. Is it intentional?

yes, you can recheck some ideal route changes. It's caused because of the different ideal route (cleaned constraints, excess symbols like '/', removing optionals if not present, etc)

We shouldn't introduce more broken_hearts. If you're making a behavior change that you think is correct then the test harness should be adjusted to produce a green_heart. The main thing these markdown files were meant to highlight is remaining gaps we should aim to get fixed.

For code that we do not control, my hope was that we could push on the ASP.NET team to consider making changes.

Also behavior must remain identical (even if the behavior is not ideal) between the attributes added to metrics and those added to spans. Is this still true with this PR?

There is no way that hearts will remain the same, when the expectation was also wrong.
New expectations are in line with AspNet package, spec and consistent output.

There is also no way that spans and metrics can have identical behaviour. Since one part is what we control and another is uncontrollable. I don't know what is more ideal, either ASP.NET Core has to adapt faster or SDK apis are extended for packages around API to be able to override the src behaviour.

Kielek · 2025-11-05T17:51:21Z

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

 * Add support for .NET 10.0.
  ([#2822](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2822))

+* Fixes an issue where `http.route` attribute reported only a plain route


http.route for spans? I think you are not touching metrics for most cases.

true, needs some rewording, so that only spans were affected.

github-actions · 2025-11-14T03:32:16Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Kielek · 2025-11-14T07:47:58Z

I have some conversation with @alanwest last week. Some notes:

He would like to avoid differences in the http.route related parameters between metrics and the traces. The main reason behind this, is the fact that some backends are using it to correlate these signals based on this attribute.
@JamesNK, is there any chance that you can advice how to fix http.route spans for metrics? Do you see any options to modify these attributes on OTel instrumentation side? If not, can you apply changes directly in the AspNetCore?
Not directly related, but AspNetCore is creating activities, but without needed attributes, so we here recreating such activities in this package. Again, @JamesNK, do you see any option to fully manage these activities+attributes by the AspNetCore directly?

JamesNK · 2025-11-14T23:12:31Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

+        }
+    }
+
+    private static string GetRoutePattern(RoutePattern routePattern, RouteValueDictionary routeValues)


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

JamesNK · 2025-11-14T23:13:13Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

+                            if (routePattern.RequiredValues.TryGetValue(parameterPart.Name, out var parameterValue) && parameterValue != null)
+                            {
+                                sb.Append(parameterValue);
+                                sb.Append('/');


Why not look for separator part for adding separator?

JamesNK · 2025-11-14T23:13:42Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

+                                (parameterPart.IsOptional && routeValues.ContainsKey(parameterPart.Name)))
+                            {
+                                sb.Append('{');
+                                sb.Append(parameterPart.Name);


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.

JamesNK · 2025-11-14T23:15:30Z

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json

-    "expectedHttpRoute": "{area:exists}/ControllerForMyArea/NonDefault/{id?}"
+    "currentHttpRoute": null,
+    "currentMetricRoute": "{area:exists}/{controller=ControllerForMyArea}/{action=Default}/{id?}",
+    "expectedHttpRoute": "MyArea/ControllerForMyArea/NonDefault"


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.

JamesNK · 2025-11-14T23:20:56Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

+        }
+    }
+
+    private static string GetRoutePattern(RoutePattern routePattern, RouteValueDictionary routeValues)


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.

JamesNK · 2025-11-14T23:23:13Z

Metric tags are accessible via https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.features.ihttpmetricstagsfeature?view=aspnetcore-9.0 on HttpContext. You could get and modify http.route there.

We'll do work to HTTP semantic convention activity attributes in .NET 11.

If what you're doing here is the correct way to represent http.route then we can copy this into ASP.NET Core.

JamesNK · 2025-11-14T23:32:26Z

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json

    "currentHttpRoute": null,
-    "expectedHttpRoute": "/MinimalApiUsingMapGroup/"
+    "currentMetricRoute": "/MinimalApiUsingMapGroup/",
+    "expectedHttpRoute": "MinimalApiUsingMapGroup"


Why remove preceeding /?

JamesNK · 2025-11-14T23:35:09Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs

+        }
+    }
+
+    private static string GetRoutePattern(RoutePattern routePattern, RouteValueDictionary routeValues)


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.

JamesNK · 2025-11-14T23:35:36Z

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json

    "currentHttpRoute": null,
-    "expectedHttpRoute": "SomePath/{id}/{num:int}"
+    "currentMetricRoute": "SomePath/{id}/{num:int}",
+    "expectedHttpRoute": "SomePath/{id}/{num}"


The route constraint is being lost

JamesNK · 2025-11-14T23:38:34Z

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json

    "currentHttpRoute": null,
-    "expectedHttpRoute": "/MinimalApi/{id}"
+    "currentMetricRoute": "/MinimalApi/{id}",
+    "expectedHttpRoute": "MinimalApi/{id}"


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.

github-actions bot added the comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore label Oct 23, 2025

RassK mentioned this pull request Oct 23, 2025

[Instrumentation.AspNetCore] Tests are preventing attribute changes #3339

Open

martincostello mentioned this pull request Oct 24, 2025

[Instrumentation.AspNet] Stable release #2909

Open

github-actions bot added Stale and removed Stale labels Oct 31, 2025

RassK added 2 commits November 4, 2025 12:42

fix route attribute logic

fc78c0e

update

d3ab87a

RassK force-pushed the aspnetcore-route-fix branch from 54c0044 to d3ab87a Compare November 4, 2025 12:45

RassK added 2 commits November 4, 2025 13:29

fix traces and update routes

103f520

revert routingtests and update cases

cee12f9

martincostello reviewed Nov 4, 2025

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs Outdated Show resolved Hide resolved

RassK added 2 commits November 4, 2025 16:07

update metric tests

edc8a72

simplify routepattern for page

b46f36e

martincostello reviewed Nov 4, 2025

View reviewed changes

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestResult.cs Outdated Show resolved Hide resolved

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/RouteAttributeHelper.cs Outdated Show resolved Hide resolved

RassK added 2 commits November 4, 2025 16:16

typo fix

ed8680a

fix if statement

5361b9f

martincostello mentioned this pull request Nov 4, 2025

Support .NET 10 #2822

Merged

4 tasks

RassK added 6 commits November 5, 2025 12:31

update docs

92d1c9c

Merge branch 'main' into aspnetcore-route-fix

512256b

# Conflicts: # src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

fix line length

c75d643

fix md012

bb380b9

remove space

833fc39

fix .net10

d57c244

RassK marked this pull request as ready for review November 5, 2025 16:37

RassK requested a review from a team as a code owner November 5, 2025 16:37

martincostello approved these changes Nov 5, 2025

View reviewed changes

Kielek reviewed Nov 5, 2025

View reviewed changes

RassK changed the title ~~[Instrumentation.AspNetCore] Normalize route template~~ [Instrumentation.AspNetCore] Fix http.route for HTTP server spans Nov 6, 2025

RassK added 2 commits November 6, 2025 10:11

update changelog

49b93a1

fix line length

5bfe512

github-actions bot added the Stale label Nov 14, 2025

JamesNK reviewed Nov 14, 2025

View reviewed changes

github-actions bot removed the Stale label Nov 15, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

RassK commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

codecov bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RassK commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RassK Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

Kielek commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RassK commented Oct 23, 2025 •

edited

Loading

codecov bot commented Nov 4, 2025 •

edited

Loading

RassK Nov 5, 2025 •

edited

Loading

JamesNK Nov 14, 2025 •

edited

Loading

JamesNK Nov 14, 2025 •

edited

Loading