[ASP.NET Core] Fix missing http.route for requests outside of MVC

[vishweshbankwar]

Fix #3461 #2986 #3771

Changes

The change updates the logic to get route information for a request using Endpoint details available during OnStopEvent. The current way of getting route information does not work for requests outside of MVC e.g. Minimal APIs where the Microsoft.AspNetCore.Mvc.BeforeAction event itself is not triggered. This results in missing http.route tag on exported activities with DisplayName set to request path (high cardinality). This fix is working for both MVC versus Non-MVC for apps targeting net6.0 and net7.0.

For applications targeting netstandard2.0 and netstandard2.1 applications we would continue to subscribe to Microsoft.AspNetCore.Mvc.BeforeAction for route information.

Follow up: #4104 (comment)

Before

Routing	DisplayName	http.route (Trace)	http.route(Metrics)
Conventional	/Home/Privacy	not present	{controller=Home}/{action=Index}/{id?}
Attribute	Home/IndexAttributeRouting	Home/IndexAttributeRouting	Home/IndexAttributeRouting
Minimal API	/api/values/2	not present	api/Values/{id}
Verb	Verb/{id}	Verb/{id}	Verb/{id}

After

Routing	DisplayName	http.route (Trace)	http.route(Metrics)
Conventional	{controller=Home}/{action=Index}/{id?}	{controller=Home}/{action=Index}/{id?}	{controller=Home}/{action=Index}/{id?}
Attribute	Home/IndexAttributeRouting	Home/IndexAttributeRouting	Home/IndexAttributeRouting
Minimal API	api/Values/{id}	api/Values/{id}	api/Values/{id}
Verb	Verb/{id}	Verb/{id}	Verb/{id}

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #4104 (2a5a6fe) into main (64df5e3) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 2a5a6fe differs from pull request most recent head ef166a9. Consider uploading reports for the commit ef166a9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4104      +/-   ##
==========================================
+ Coverage   85.14%   85.24%   +0.10%     
==========================================
  Files         318      315       -3     
  Lines       12620    12541      -79     
==========================================
- Hits        10745    10691      -54     
+ Misses       1875     1850      -25     

Impacted Files	Coverage Δ
...umentation.AspNetCore/AspNetCoreInstrumentation.cs	100.00% <100.00%> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs	89.47% <100.00%> (-0.72%)	⬇️

... and 20 files with indirect coverage changes

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[alanwest]

I've tried things out on an MVC app before and after this PR. I think we need to consider the test matrix a bit before going forward with this PR.

Below is abbreviated output from the console exporter highlighting the important differences. Three endpoints are exercised:

• http://localhost:5199/
• http://localhost:5199/Home/Index
• http://localhost:5199/Home/Privacy

Two things of note:

1. Note that the display name of the Activity is now the same for every endpoint.
2. Now that the http.route attribute is set, it too is the same for all of the endpoints - I'm not sure if this is a simply a limitation due to the type of routing I'm using, or if there's a way we can get the actual controller/action of the route in the http.route.

Before
Activity.DisplayName: /
Activity.Tags:
http.target: /
http.url: http://localhost:5199/

Activity.DisplayName: /Home/Index
Activity.Tags:
http.target: /Home/Index
http.url: http://localhost:5199/Home/Index

Activity.DisplayName: /Home/Privacy
Activity.Tags:
http.target: /Home/Privacy
http.url: http://localhost:5199/Home/Privacy

After
Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /
http.url: http://localhost:5199/
http.route: {controller=Home}/{action=Index}/{id?}

Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /Home/Index
http.url: http://localhost:5199/Home/Index
http.route: {controller=Home}/{action=Index}/{id?}

Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /Home/Privacy
http.url: http://localhost:5199/Home/Privacy
http.route: {controller=Home}/{action=Index}/{id?}

[alanwest]

Just a quick brainstorm of the various routing use cases we might need coverage for.

• Conventional routing
• Attribute-based routing
• Minimal API
• Verb templates
• There may be other things that may or may not be relevant to us. Like Areas

I'm not suggesting we need coverage for all these things in this PR, but given that routing is so complex, I think it'd be nice to be able to have a table to be able to point to that makes it clear what to expect when using different styles of routing. Something like:

	Activity.DisplayName	Span http.route	Metric http.route
Conventional routing	...	...	...
Attribute-based routing	...	...	...
...	...	...	...

[vishweshbankwar]

@alanwest - Have updated PR description. I think the case you had #4104 (comment) is conventional routing

We could have these added to test

[alanwest]

With this PR if /Home/Privacy maps to {controller=Home}/{action=Index}/{id?} with conventional routing, then I think we should resolve this before merging.

[JamesNK]

It looks like you have expected behavior.

I haven't run your tests, but I guess you're getting one route for http://localhost:5199/ and http://localhost:5199/Home/Index and http://localhost:5199/Home/Privacy because they all match to the same conventional route. That's how conventional routing works: an app centrally specifies just a few route patterns, then those patterns match to multiple controllers and actions.

The pattern {controller=Home}/{action=Index}/{id?} will take any URL with 0-3 segments, e.g. /, /Home/Index, /Home/Privacy, and match the controller/action to controllers and actions in your app.

You could add some customization like replacing controller, action and page parameters with values from RouteData. That might be more useful, but it wouldn't be the original template anymore.

[vishweshbankwar]

Thanks @JamesNK for reviewing this. We have a similar issue with API versioning #4525. Where the template has same value for different API versions. Would that one also require some customization?

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.