Replace static parameters in route pattern for telemetry - #5
Open
Conversation
Code Review by Qodo (Alpha)
1. Regex compiled in debug
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+106
to
+111
| else if (constraint.ParameterPolicy is Constraints.RegexRouteConstraint regexConstraint) | ||
| { | ||
| builder.Append("regex("); | ||
| builder.Append(regexConstraint.Constraint.ToString()); | ||
| builder.Append(')'); | ||
| } |
There was a problem hiding this comment.
1. Regex compiled in debug 🐞 Bug
• RoutePatternParameterPart.DebuggerToString now formats RegexRouteConstraint by reading regexConstraint.Constraint, which forces the lazily-created Regex instance to be created/compiled. • DebuggerToString is executed during endpoint construction for both RouteEndpoint.DisplayName and default IRouteDiagnosticsMetadata, so this can regress startup performance for apps using regex-based constraints/policies.
Agent Prompt
### Issue description
`RoutePatternParameterPart.DebuggerToString()` accesses `RegexRouteConstraint.Constraint` to print the regex pattern. This forces lazy regex creation/compilation during endpoint build, regressing startup performance and undermining the explicit lazy-init design of `RegexRouteConstraint`.
### Issue Context
`DebuggerToString()` is used beyond the debugger: it feeds `RouteEndpoint.DisplayName` and default `IRouteDiagnosticsMetadata`.
### Fix Focus Areas
- src/Http/Routing/src/Patterns/RoutePatternParameterPart.cs[99-116]
- src/Http/Routing/src/Constraints/RegexRouteConstraint.cs[41-76]
- src/Http/Routing/src/Patterns/RoutePatternFactory.cs[946-963]
### Implementation notes
- If the policy came from text, prefer emitting that text (`constraint.Content`) without materializing the regex.
- For `RegexRouteConstraint` instances created from strings (e.g., `Constraint(object)` converting strings to `RegexRouteConstraint`), consider preserving the original pattern string in the policy reference (or enhancing `RegexRouteConstraint` to expose it) so debug formatting does not require compiling the regex.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+32
to
+36
| // Preserve leading slash from raw text | ||
| if (pattern.RawText is { Length: > 0 } rt && rt[0] == Separator) | ||
| { | ||
| result = Separator + result; | ||
| } |
There was a problem hiding this comment.
2. Drops tilde-slash prefix 🐞 Bug
• When RequiredValues match parameters, RoutePatternDebugStringFormatter rebuilds the string from segments and only re-adds a leading '/' (not "~/"). • For patterns whose RawText starts with "~/" (supported by RoutePatternParser), this means the returned debug string loses that prefix only in the rebuild/substitution path.
Agent Prompt
### Issue description
When required-value substitution triggers the rebuild path, `RoutePatternDebugStringFormatter.Format` only preserves a leading `/`. Templates starting with `~/` will lose that prefix in the returned debug string.
### Issue Context
`RoutePatternParser` allows `~/` and strips it for parsing, but `RoutePattern.RawText` retains the original input. The formatter should preserve the same prefix semantics as the original raw template, even when rebuilding.
### Fix Focus Areas
- src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs[13-45]
- src/Http/Routing/src/Patterns/RoutePatternParser.cs[457-472]
- src/Http/Routing/test/UnitTests/Patterns/RoutePatternDebugStringFormatterTest.cs[197-230]
### Implementation notes
- Add handling like:
- if `RawText` starts with `"~/"`, prefix the result with `"~/"`
- else if `RawText` starts with `/`, prefix with `/`
- Add unit test coverage for `template="~/{controller}/{action}"` with required values causing substitution.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+92
to
+98
| if (pattern.RequiredValues.TryGetValue(parameterName, out var requiredValue) && | ||
| !RoutePattern.IsRequiredValueAny(requiredValue) && | ||
| requiredValue?.ToString() is { Length: > 0 } v) | ||
| { | ||
| value = v; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
3. Required value culture drift 🐞 Bug
• Required value substitution uses requiredValue.ToString(), which is culture-dependent for non-string values and may produce different endpoint display names/route metadata across environments. • Routing already normalizes values with Convert.ToString(..., CultureInfo.InvariantCulture) elsewhere; using object.ToString() here is inconsistent and can also surface unexpected ToString() exceptions into endpoint construction.
Agent Prompt
### Issue description
`TryGetRequiredValue` uses `requiredValue?.ToString()` which is culture-dependent for non-strings and can make endpoint display names and route diagnostics metadata differ across environments.
### Issue Context
Routing already treats non-string values as route strings using `Convert.ToString(..., CultureInfo.InvariantCulture)` (see `RouteValueEqualityComparer`).
### Fix Focus Areas
- src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs[90-101]
- src/Http/Routing/src/RouteValueEqualityComparer.cs[12-35]
### Implementation notes
- Add `using System.Globalization;`
- Replace `requiredValue?.ToString()` with invariant conversion:
- `var v = requiredValue as string ?? Convert.ToString(requiredValue, CultureInfo.InvariantCulture);`
- Keep the existing `{ Length: > 0 }` check and RequiredValueAny filtering.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| { | ||
| return RawText ?? string.Join(SeparatorString, PathSegments.Select(s => s.DebuggerToString())); | ||
| } | ||
| internal string DebuggerToString() => RoutePatternDebugStringFormatter.Format(this); |
There was a problem hiding this comment.
4. Metrics tag cardinality 🐞 Bug
• The default IRouteDiagnosticsMetadata.Route is used as a metrics tag; substituting RequiredValues (controller/action names) changes the tag from a generic template to a per-endpoint value. • This increases metric label cardinality (bounded by number of endpoints, but can be large in big MVC apps) and may increase memory/CPU cost in metrics pipelines/backends.
Agent Prompt
### Issue description
`IRouteDiagnosticsMetadata.Route` is used as a metrics tag. With required-value substitution, the value becomes more endpoint-specific (e.g., controller/action names), increasing tag cardinality.
### Issue Context
This is a trade-off: better per-endpoint readability vs higher cardinality. Cardinality is bounded by endpoint count but can be large for big apps.
### Fix Focus Areas
- src/Http/Routing/src/Patterns/RoutePattern.cs[154-159]
- src/Http/Routing/src/RouteEndpointBuilder.cs[108-112]
- src/Http/Routing/src/EndpointRoutingMiddleware.cs[129-133]
- src/Http/Routing/src/Patterns/RoutePatternDebugStringFormatter.cs[13-45]
### Implementation notes
Options to consider:
- Keep the substituted formatter for `DisplayName`, but use `RoutePattern.RawText` (or a template-only formatter) for default `IRouteDiagnosticsMetadata.Route`.
- Alternatively add a second API (e.g., `GetDiagnosticsRouteString()` vs `GetDebuggerDisplayString()`) and use the lower-cardinality one for metrics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from qodo-benchmark#20