-
Notifications
You must be signed in to change notification settings - Fork 357
[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.
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
fc78c0e
d3ab87a
103f520
cee12f9
edc8a72
b46f36e
ed8680a
5361b9f
92d1c9c
512256b
c75d643
bb380b9
833fc39
d57c244
49b93a1
5bfe512
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated, but I think an empty string route should resolve to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| { | ||
| if (routePattern.PathSegments.Count == 0) | ||
| { | ||
| // RazorPage default route | ||
| if (routePattern.Defaults.TryGetValue("page", out var pageValue)) | ||
| { | ||
| return pageValue?.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) | ||
martincostello marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| sb.Append(parameterValue); | ||
| sb.Append('/'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not look for separator part for adding separator? |
||
| break; | ||
| } | ||
|
|
||
| goto default; | ||
| default: | ||
| if (!parameterPart.IsOptional || | ||
| (parameterPart.IsOptional && routeValues.ContainsKey(parameterPart.Name))) | ||
| { | ||
| sb.Append('{'); | ||
| sb.Append(parameterPart.Name); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| sb.Append('}'); | ||
| sb.Append('/'); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Remove the trailing '/' | ||
| return sb.ToString(0, sb.Length - 1); | ||
| } | ||
| } | ||
|
|
||
| #endif | ||
There was a problem hiding this comment.
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.