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.

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

[ASP.NET Core] Fix missing http.route for requests outside of MVC #4104

Closed
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
821b1b0
Fix missing http.route for non-mvc requests
vishweshbankwar Jan 26, 2023
0e12ed7
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar Feb 17, 2023
bd7f6e0
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
cijothomas Mar 5, 2023
517e4c1
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 1, 2023
af0f9df
add back !net6
vishweshbankwar May 1, 2023
ad11484
Merge branch 'vibankwa/fix-route-template-aspnetcore' of https://gith…
vishweshbankwar May 1, 2023
7c31a1b
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 1, 2023
f99f33a
fix test
vishweshbankwar May 1, 2023
37607e9
Merge branch 'vibankwa/fix-route-template-aspnetcore' of https://gith…
vishweshbankwar May 1, 2023
7361079
fix assert
vishweshbankwar May 1, 2023
c33d201
chagelog
vishweshbankwar May 5, 2023
fa133f2
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 5, 2023
8527767
fix md error
vishweshbankwar May 5, 2023
dfd7774
revert
vishweshbankwar May 5, 2023
a966577
remove todo
vishweshbankwar May 5, 2023
15cde25
Merge branch 'vibankwa/fix-route-template-aspnetcore' of https://gith…
vishweshbankwar May 5, 2023
f9f7607
revert
vishweshbankwar May 5, 2023
32f71a6
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 5, 2023
99f9396
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 12, 2023
2a5a6fe
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
vishweshbankwar May 15, 2023
ef166a9
Merge branch 'main' into vibankwa/fix-route-template-aspnetcore
cijothomas May 31, 2023
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
Original file line number Diff line number Diff line change
@@ -27,7 +27,6 @@ internal sealed class AspNetCoreInstrumentation : IDisposable
"Microsoft.AspNetCore.Hosting.HttpRequestIn",
"Microsoft.AspNetCore.Hosting.HttpRequestIn.Start",
"Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop",
"Microsoft.AspNetCore.Mvc.BeforeAction",
"Microsoft.AspNetCore.Diagnostics.UnhandledException",
"Microsoft.AspNetCore.Hosting.UnhandledException",
};
Original file line number Diff line number Diff line change
@@ -21,7 +21,7 @@
#endif
using Microsoft.AspNetCore.Http;
#if NET6_0_OR_GREATER
using Microsoft.AspNetCore.Mvc.Diagnostics;
using Microsoft.AspNetCore.Routing;
#endif
using OpenTelemetry.Context.Propagation;
#if !NETSTANDARD2_0
@@ -86,12 +86,6 @@ public override void OnEventWritten(string name, object payload)
this.OnStopActivity(Activity.Current, payload);
}

break;
case OnMvcBeforeActionEvent:
{
this.OnMvcBeforeAction(Activity.Current, payload);
}

break;
case OnUnhandledHostingExceptionEvent:
case OnUnHandledDiagnosticsExceptionEvent:
@@ -242,6 +236,15 @@ public void OnStopActivity(Activity activity, object payload)

activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);

#if NET6_0_OR_GREATER
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(route))
{
activity.DisplayName = route;
Copy link
Member

Choose a reason for hiding this comment

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

I've tried things out on an MVC app before and after this PR. I think we need to consider the test matrix a bit before going forward with this PR.

Below is abbreviated output from the console exporter highlighting the important differences. Three endpoints are exercised:

Two things of note:

  1. Note that the display name of the Activity is now the same for every endpoint.
  2. Now that the http.route attribute is set, it too is the same for all of the endpoints - I'm not sure if this is a simply a limitation due to the type of routing I'm using, or if there's a way we can get the actual controller/action of the route in the http.route.

Before
Activity.DisplayName: /
Activity.Tags:
http.target: /
http.url: http://localhost:5199/

Activity.DisplayName: /Home/Index
Activity.Tags:
http.target: /Home/Index
http.url: http://localhost:5199/Home/Index

Activity.DisplayName: /Home/Privacy
Activity.Tags:
http.target: /Home/Privacy
http.url: http://localhost:5199/Home/Privacy

After
Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /
http.url: http://localhost:5199/
http.route: {controller=Home}/{action=Index}/{id?}

Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /Home/Index
http.url: http://localhost:5199/Home/Index
http.route: {controller=Home}/{action=Index}/{id?}

Activity.DisplayName: {controller=Home}/{action=Index}/{id?}
Activity.Tags:
http.target: /Home/Privacy
http.url: http://localhost:5199/Home/Privacy
http.route: {controller=Home}/{action=Index}/{id?}

Copy link
Member

Choose a reason for hiding this comment

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

Just a quick brainstorm of the various routing use cases we might need coverage for.

I'm not suggesting we need coverage for all these things in this PR, but given that routing is so complex, I think it'd be nice to be able to have a table to be able to point to that makes it clear what to expect when using different styles of routing. Something like:

Activity.DisplayName Span http.route Metric http.route
Conventional routing ... ... ...
Attribute-based routing ... ... ...
... ... ... ...

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest - Have updated PR description. I think the case you had #4104 (comment) is conventional routing

We could have these added to test

Copy link
Member

Choose a reason for hiding this comment

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

With this PR if /Home/Privacy maps to {controller=Home}/{action=Index}/{id?} with conventional routing, then I think we should resolve this before merging.

Copy link
Contributor

@JamesNK JamesNK Jun 4, 2023

Choose a reason for hiding this comment

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

It looks like you have expected behavior.

I haven't run your tests, but I guess you're getting one route for http://localhost:5199/ and http://localhost:5199/Home/Index and http://localhost:5199/Home/Privacy because they all match to the same conventional route. That's how conventional routing works: an app centrally specifies just a few route patterns, then those patterns match to multiple controllers and actions.

The pattern {controller=Home}/{action=Index}/{id?} will take any URL with 0-3 segments, e.g. /, /Home/Index, /Home/Privacy, and match the controller/action to controllers and actions in your app.

You could add some customization like replacing controller, action and page parameters with values from RouteData. That might be more useful, but it wouldn't be the original template anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @JamesNK for reviewing this. We have a similar issue with API versioning #4525. Where the template has same value for different API versions. Would that one also require some customization?

activity.SetTag(SemanticConventions.AttributeHttpRoute, route);
}
#endif

#if !NETSTANDARD2_0
if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod))
{
@@ -294,57 +297,6 @@ public void OnStopActivity(Activity activity, object payload)
}
}

public void OnMvcBeforeAction(Activity activity, object payload)
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
{
// We cannot rely on Activity.Current here
// There could be activities started by middleware
// after activity started by framework resulting in different Activity.Current.
// so, we need to first find the activity started by Asp.Net Core.
// For .net6.0 onwards we could use IHttpActivityFeature to get the activity created by framework
// var httpActivityFeature = context.Features.Get<IHttpActivityFeature>();
// activity = httpActivityFeature.Activity;
// However, this will not work as in case of custom propagator
// we start a new activity during onStart event which is a sibling to the activity created by framework
// So, in that case we need to get the activity created by us here.
// we can do so only by looping through activity.Parent chain.
while (activity != null)
{
if (string.Equals(activity.OperationName, ActivityOperationName, StringComparison.Ordinal))
{
break;
}

activity = activity.Parent;
}

if (activity == null)
{
return;
}

if (activity.IsAllDataRequested)
{
#if !NET6_0_OR_GREATER
_ = this.beforeActionActionDescriptorFetcher.TryFetch(payload, out var actionDescriptor);
_ = this.beforeActionAttributeRouteInfoFetcher.TryFetch(actionDescriptor, out var attributeRouteInfo);
_ = this.beforeActionTemplateFetcher.TryFetch(attributeRouteInfo, out var template);
#else
var beforeActionEventData = payload as BeforeActionEventData;
var template = beforeActionEventData.ActionDescriptor?.AttributeRouteInfo?.Template;
#endif
if (!string.IsNullOrEmpty(template))
{
// override the span name that was previously set to the path part of URL.
activity.DisplayName = template;
activity.SetTag(SemanticConventions.AttributeHttpRoute, template);
}

// TODO: Should we get values from RouteData?
// private readonly PropertyFetcher beforeActionRouteDataFetcher = new PropertyFetcher("routeData");
// var routeData = this.beforeActionRouteDataFetcher.Fetch(payload) as RouteData;
}
}

public void OnException(Activity activity, object payload)
{
if (activity.IsAllDataRequested)
Original file line number Diff line number Diff line change
@@ -676,10 +676,10 @@ public async Task ActivitiesStartedInMiddlewareBySettingHostActivityToNullShould
Assert.Equal(activityName, middlewareActivity.OperationName);
Assert.Equal(activityName, middlewareActivity.DisplayName);

// tag http.route should not be added on activity started by asp.net core as it will not be found during OnEventWritten event
Assert.DoesNotContain(aspnetcoreframeworkactivity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute);
// tag http.route should be added on activity started by asp.net core as it is set during onStop event
Assert.Contains(aspnetcoreframeworkactivity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute);
Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName);
Assert.Equal("/api/values/2", aspnetcoreframeworkactivity.DisplayName);
Assert.Equal("api/Values/{id}", aspnetcoreframeworkactivity.DisplayName);
}

#if NET7_0_OR_GREATER
@@ -779,12 +779,6 @@ void ConfigureTestServices(IServiceCollection services)
numberofSubscribedEvents++;
}

break;
case HttpInListener.OnMvcBeforeActionEvent:
{
numberofSubscribedEvents++;
}

break;
default:
{
@@ -814,7 +808,7 @@ void ConfigureTestServices(IServiceCollection services)
}

Assert.Equal(0, numberOfUnSubscribedEvents);
Assert.Equal(3, numberofSubscribedEvents);
Assert.Equal(2, numberofSubscribedEvents);
}

[Fact]
@@ -844,12 +838,6 @@ void ConfigureTestServices(IServiceCollection services)
numberofSubscribedEvents++;
}

break;
case HttpInListener.OnMvcBeforeActionEvent:
{
numberofSubscribedEvents++;
}

break;

// TODO: Add test case for validating name for both the types
@@ -898,7 +886,7 @@ void ConfigureTestServices(IServiceCollection services)

Assert.Equal(1, numberOfExceptionCallbacks);
Assert.Equal(0, numberOfUnSubscribedEvents);
Assert.Equal(4, numberofSubscribedEvents);
Assert.Equal(3, numberofSubscribedEvents);
}

[Fact]