Replace static parameters in route pattern for telemetry - #64854
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances telemetry for ASP.NET Core routing by replacing static route parameters (like {controller}, {action}, {area}, and {page}) with their actual values in the http.route attribute for OpenTelemetry. This aligns with the OpenTelemetry specification requirement that static path segments with constrained values should use the actual value rather than the parameter placeholder. For example, a conventional route {controller}/{action}/{id?} for the HomeController/Index endpoint will now report as Home/Index/{id?} instead of the template pattern.
- Modified
RoutePattern.DebuggerToString()to replace parameters with required values when available - Enhanced
RoutePatternParameterPart.DebuggerToString()to better format constraints, especially RegexRouteConstraint - Added comprehensive test coverage for the new parameter replacement logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Http/Routing/src/Patterns/RoutePattern.cs | Implements the core logic to replace route parameters with their required values in the DebuggerToString method, adding new helper methods GetSegmentDebuggerToString and TryGetRequiredValue |
| src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs | Enhances constraint formatting in DebuggerToString to properly display RegexRouteConstraint patterns |
| src/Http/Routing/test/UnitTests/Patterns/RoutePatternTest.cs | Adds comprehensive test coverage with 18 test methods covering various scenarios including required values, catch-all parameters, complex segments, constraints, and edge cases |
| return string.Join(string.Empty, parts); | ||
| } | ||
|
|
||
| private bool TryGetRequiredValue(string parameterName, [NotNullWhen(true)]out string? value) |
There was a problem hiding this comment.
Missing space between the attribute and the parameter declaration. The code has [NotNullWhen(true)]out string? value but should be [NotNullWhen(true)] out string? value with a space between the closing bracket and out.
| private bool TryGetRequiredValue(string parameterName, [NotNullWhen(true)]out string? value) | |
| private bool TryGetRequiredValue(string parameterName, [NotNullWhen(true)] out string? value) |
javiercn
left a comment
There was a problem hiding this comment.
Looks good to me, but the tests have a TON of repetition. It might be worth to combine many cases into theories based on the expected output.
63f7916 to
89ee7c9
Compare
OTEL
http.routeattributes have a new rule: static route parameters should be replaced with the static value.In the context of ASP.NET Core that means conventional routes should have
{controller},{action},{area}and{page}parameters replaced when the route is added to telemetry.For example, with conventional route
{controller=Home}/{action=Index}/{id?}, theStoreController.Checkout()endpoint should have a route ofStore/Checkout/{id?}.Fixes #64852