Skip to content

Allow root action rewrites #5402

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

Closed
PascalSenn wants to merge 2 commits into from
Closed

Allow root action rewrites #5402

Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Allow root action rewrites
  • Loading branch information
@PascalSenn
PascalSenn committed Feb 29, 2024
commit 16da893073b0fe58475b1b16e7cf46f27f464c92
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

@vishweshbankwar vishweshbankwar Mar 5, 2024

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

@PascalSenn PascalSenn Mar 5, 2024

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

@vishweshbankwar vishweshbankwar Mar 5, 2024

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

@alanwest alanwest Mar 6, 2024

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

@OptimusPi OptimusPi Mar 6, 2024

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();
}
}