Skip to content

Add a diagnostic source event that fires when a route is matched #11685

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

Merged
merged 3 commits into from
Jul 1, 2019

Conversation

davidfowl
Copy link
Member

  • Usually more information becomes available about a request once route is matched. This event should allow diagnostic systems to enlighten the typical "begin request" metadata to include more information about the matched route and more importantly the selected endpoint and associated metadata.

cc @lmolkova @SergeyKanzhelev

I'd expect open telemetry to use this to determine what type of request is being routed instead of the specific MVC event:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/3538e05f0eed9df008d30e82a70bef11bd579bb5/src/OpenTelemetry.Collector.AspNetCore/Implementation/HttpInListener.cs#L108-L137

You could also imagine crawling through the endpoint metadata to light up more information about the request (like the gRPC method or controller method that got selected).

- Usually more information becomes available about a request once route is matched. This event shoud allow diagnositc systems to enlighten the typical "begin request" metadata to include more information about the matched route and more importantly the selected endpoint and associated metadata.
@analogrelay analogrelay added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 28, 2019
@rynowak
Copy link
Member

rynowak commented Jun 28, 2019

once route is matched

What's a route :trollface:

@davidfowl
Copy link
Member Author

Man this broke a bunch of tests 😂

@JamesNK
Copy link
Member

JamesNK commented Jun 28, 2019

@davidfowl Is this PR what you refer to here: grpc/grpc-dotnet#341 (comment)

So would the idea be that a diagnostic framework could listen for this event, get the endpoint, then inspect the endpoint metadata to figure out that it is a gRPC/MVC/SignalR request?

if (_diagnosticListener.IsEnabled() && _diagnosticListener.IsEnabled(DiagnosticsRouteMatchedKey))
{
// We're just going to send the HttpContext since it has all of the relevant information
_diagnosticListener.Write(DiagnosticsRouteMatchedKey, httpContext);
Copy link

@lmolkova lmolkova Jun 28, 2019

Choose a reason for hiding this comment

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

  1. Is it correct that this event will fire for both: WebApi and MVC?
  2. How do we get route from HttpContext? httpContext.GetEndpoint()?
  3. AppInsights uses https://github.com/aspnet/Mvc/blob/master/src/Microsoft.AspNetCore.Mvc.Core/MvcCoreDiagnosticSourceExtensions.cs events to know about routing today. Should we stop listening to those?

Copy link
Member

@JamesNK JamesNK Jun 28, 2019

Choose a reason for hiding this comment

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

  1. It will fire for any framework that matches using endpoint routing. Today that means MVC, razor pages, gRPC, SignalR. Likely community frameworks will use it as well. On the endpoint there will be metadata that will let you figure out what the route is to. MVC for example will put the ActionDescriptor in the endpoint metadata. gRPC adds its own metadata, etc.
  2. Yes
  3. It should still be used for legacy apps. You should be able to listen for both side-by-side. I can't imagine why an ASP.NET Core app would use both old and new routing.

@davidfowl
Copy link
Member Author

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2705

@davidfowl davidfowl added this to the 3.0.0-preview8 milestone Jun 30, 2019
@davidfowl
Copy link
Member Author

Hmm preview7 or 8...

@davidfowl davidfowl removed this from the 3.0.0-preview8 milestone Jul 1, 2019
@davidfowl davidfowl added this to the 3.0.0-preview7 milestone Jul 1, 2019
@davidfowl davidfowl merged commit d3640d5 into master Jul 1, 2019
@davidfowl
Copy link
Member Author

7

@ghost ghost deleted the davidfowl/diagnostic-events branch July 1, 2019 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants