Skip to content
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.

Sign up for GitHub

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

Allow root action rewrites #5402

Closed
wants to merge 2 commits into from
Closed

Allow root action rewrites #5402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16da893
Allow root action rewrites
PascalSenn Feb 29, 2024
c54e0dc
Merge branch 'main' into pse/allow-root-action-rewrites
PascalSenn Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,13 @@ public void OnStopActivity(Activity activity, object payload)
context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(routePattern))
{
activity.DisplayName = GetDisplayName(context.Request.Method, routePattern);
// only override the display name if it was not set by the user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PascalSenn - Could you add a unit test for this? These tests are more focused on http.route tag value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean instead of the one in RoutingTests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to add this in addition to unit test. @alanwest - Do you have any preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to test this new behavior separate from the tests for http.route.

var previousDisplayName = GetDisplayName(context.Request.Method);
if (activity.DisplayName == previousDisplayName)
{
activity.DisplayName = GetDisplayName(context.Request.Method, routePattern);
}

activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
}
#endif
Expand Down
34 changes: 34 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net6.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
| :green_heart: | MinimalApi | [Action without parameter](#minimalapi-action-without-parameter) |
| :green_heart: | MinimalApi | [Action with parameter](#minimalapi-action-with-parameter) |
| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) |
| :broken_heart: | ConventionalRouting | [Overridden display name](#conventionalrouting-overridden-display-name) |

## ConventionalRouting: Root path

Expand Down Expand Up @@ -610,3 +611,36 @@
}
}
```

## ConventionalRouting: Overridden display name

```json
{
"IdealHttpRoute": null,
"ActivityDisplayName": "Overwritten",
"ActivityHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"MetricHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"RouteInfo": {
"HttpMethod": "GET",
"Path": "/ConventionalRoute/OverwriteRootSpan?num=3",
"RoutePattern.RawText": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"IRouteDiagnosticsMetadata.Route": null,
"HttpContext.GetRouteData()": {
"controller": "ConventionalRoute",
"action": "OverwriteRootSpan"
},
"ActionDescriptor": {
"AttributeRouteInfo.Template": null,
"Parameters": [
"id",
"num"
],
"ControllerActionDescriptor": {
"ControllerName": "ConventionalRoute",
"ActionName": "OverwriteRootSpan"
},
"PageActionDescriptor": null
}
}
}
```
34 changes: 34 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net7.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
| :green_heart: | MinimalApi | [Action without parameter (MapGroup)](#minimalapi-action-without-parameter-mapgroup) |
| :green_heart: | MinimalApi | [Action with parameter (MapGroup)](#minimalapi-action-with-parameter-mapgroup) |
| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) |
| :broken_heart: | ConventionalRouting | [Overridden display name](#conventionalrouting-overridden-display-name) |

## ConventionalRouting: Root path

Expand Down Expand Up @@ -652,3 +653,36 @@
}
}
```

## ConventionalRouting: Overridden display name

```json
{
"IdealHttpRoute": null,
"ActivityDisplayName": "Overwritten",
"ActivityHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"MetricHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"RouteInfo": {
"HttpMethod": "GET",
"Path": "/ConventionalRoute/OverwriteRootSpan?num=3",
"RoutePattern.RawText": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"IRouteDiagnosticsMetadata.Route": null,
"HttpContext.GetRouteData()": {
"controller": "ConventionalRoute",
"action": "OverwriteRootSpan"
},
"ActionDescriptor": {
"AttributeRouteInfo.Template": null,
"Parameters": [
"id",
"num"
],
"ControllerActionDescriptor": {
"ControllerName": "ConventionalRoute",
"ActionName": "OverwriteRootSpan"
},
"PageActionDescriptor": null
}
}
}
```
34 changes: 34 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/README.net8.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
| :green_heart: | MinimalApi | [Action without parameter (MapGroup)](#minimalapi-action-without-parameter-mapgroup) |
| :green_heart: | MinimalApi | [Action with parameter (MapGroup)](#minimalapi-action-with-parameter-mapgroup) |
| :green_heart: | ExceptionMiddleware | [Exception Handled by Exception Handler Middleware](#exceptionmiddleware-exception-handled-by-exception-handler-middleware) |
| :broken_heart: | ConventionalRouting | [Overridden display name](#conventionalrouting-overridden-display-name) |

## ConventionalRouting: Root path

Expand Down Expand Up @@ -652,3 +653,36 @@
}
}
```

## ConventionalRouting: Overridden display name

```json
{
"IdealHttpRoute": null,
"ActivityDisplayName": "Overwritten",
"ActivityHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"MetricHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"RouteInfo": {
"HttpMethod": "GET",
"Path": "/ConventionalRoute/OverwriteRootSpan?num=3",
"RoutePattern.RawText": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"IRouteDiagnosticsMetadata.Route": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"HttpContext.GetRouteData()": {
"controller": "ConventionalRoute",
"action": "OverwriteRootSpan"
},
"ActionDescriptor": {
"AttributeRouteInfo.Template": null,
"Parameters": [
"id",
"num"
],
"ControllerActionDescriptor": {
"ControllerName": "ConventionalRoute",
"ActionName": "OverwriteRootSpan"
},
"PageActionDescriptor": null
}
}
}
```
2 changes: 2 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ public class TestCase

public string? ExpectedHttpRoute { get; set; }

public string? ExpectedDisplayName { get; set; }

public string? CurrentHttpRoute { get; set; }

public override string ToString()
Expand Down
10 changes: 10 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTestCases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,5 +207,15 @@
"expectedStatusCode": 500,
"currentHttpRoute": null,
"expectedHttpRoute": "/Exception"
},
{
"name": "Overridden display name",
"testApplicationScenario": "ConventionalRouting",
"httpMethod": "GET",
"path": "/ConventionalRoute/OverwriteRootSpan?num=3",
"expectedStatusCode": 200,
"currentHttpRoute": "{controller=ConventionalRoute}/{action=Default}/{id?}",
"expectedHttpRoute": null,
"expectedDisplayName": "Overwritten"
}
]
9 changes: 6 additions & 3 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/RouteTests/RoutingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,12 @@ public async Task TestHttpRoute(RoutingTestCases.TestCase testCase)

// Activity.DisplayName should be a combination of http.method + http.route attributes, see:
// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md#name
var expectedActivityDisplayName = string.IsNullOrEmpty(expectedHttpRoute)
? testCase.HttpMethod
: $"{testCase.HttpMethod} {expectedHttpRoute}";
// unless the user has overriden the route name
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace typo overriden with overridden to satisfy ms-misspell PR check.

var expectedActivityDisplayName = string.IsNullOrEmpty(testCase.ExpectedDisplayName)
? string.IsNullOrEmpty(expectedHttpRoute)
? testCase.HttpMethod
: $"{testCase.HttpMethod} {expectedHttpRoute}"
: testCase.ExpectedDisplayName;

Assert.Equal(expectedActivityDisplayName, activity.DisplayName);

Expand Down
17 changes: 17 additions & 0 deletions ...on.AspNetCore.Tests/RouteTests/TestApplication/Controllers/ConventionalRouteController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#nullable disable

using System.Diagnostics;
using Microsoft.AspNetCore.Mvc;

namespace RouteTests.Controllers;
Expand All @@ -14,4 +15,20 @@ public class ConventionalRouteController : Controller
public IActionResult ActionWithParameter(int id) => this.Ok();

public IActionResult ActionWithStringParameter(string id, int num) => this.Ok();

public IActionResult OverwriteRootSpan(string id, int num)
{
var currentActivity = Activity.Current;
while (currentActivity?.Parent != null)
{
currentActivity = currentActivity.Parent;
}

if (currentActivity != null)
{
currentActivity.DisplayName = "Overwritten";
}

return this.Ok();
}
}
Loading