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

Add Counters & Logs for 404s and MapFallback #46404

Closed
1 task done
Tratcher opened this issue Feb 1, 2023 · 7 comments · Fixed by #48637 or #48669
Closed
1 task done

Add Counters & Logs for 404s and MapFallback #46404

Tratcher opened this issue Feb 1, 2023 · 7 comments · Fixed by #48637 or #48669
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@Tratcher
Copy link
Member

Tratcher commented Feb 1, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

If a request reaches the end of the pipeline (e.g. isn't handled anywhere else) there is a default middleware that sends an empty 404 response.

if (!context.Response.HasStarted)
{
context.Response.StatusCode = StatusCodes.Status404NotFound;
}

Similarly MapFallback catches any request that didn't map to another route.

public static RouteHandlerBuilder MapFallback(this IEndpointRouteBuilder endpoints, Delegate handler)
{
return endpoints.MapFallback("{*path:nonfile}", handler);
}

Problem: There's no visibility in the system for tracking how many requests are missing routes and why. This information would be needed to identify bad links, misconfigured routes, and monitor the customer experience.

Describe the solution you'd like

  • Add a meter/counter that tracks how many requests reach the 404 middleware.
  • Add a meter/counter that tracks how many requests reach the fallback route. This may already be covered by Add Meter/Counters to track hits for each route/endpoint #46361
  • Add an Info or Debug level log in each of these places that includes the following:
    • Scheme, host, port, path base, path
    • Method
    • Protocol
  • Use structured logging to allow low level filtering and avoid unnecessary allocations
  • These should be under granular log prefixes for targeted enablement.
    • Microsoft.AspNetCore.Routing.EndOfPipeline.UnexpectedRequest(scheme...
    • Microsoft.AspNetCore.Routing.Fallback.HandledRequest(scheme...

Additional context

The counters would be used to monitor the volume of unexpected requests, and then the logs could be enabled to determine the specific request types.

Having sampling support in the loggers would also help deal with high volume output.

@Tratcher Tratcher self-assigned this Feb 1, 2023
@davidfowl
Copy link
Member

Use structured logging to allow low level filtering and avoid unnecessary allocations

What is this? How are these logs going to be consumed?

@BrennanConroy BrennanConroy added this to the .NET 8 Planning milestone Feb 6, 2023
@ghost
Copy link

ghost commented Feb 6, 2023

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@Tratcher
Copy link
Member Author

Tratcher commented Feb 7, 2023

Triage feedback (@halter73): The counter can go directly into hosting, not the app builder, so we know what status code was actually sent to the client. Similarly, we can already use hosting's ResponseFinished log for this.

info: Microsoft.AspNetCore.Hosting.Diagnostics[2]
      Request finished HTTP/1.1 GET http://localhost:5000/favicon.ico - 404 0 - 1.7527ms

Hmm, using ResponseFinished doesn't account for the requirement about not logging paths if they're considered sensitive.

Since it's hosting, would you do a counter for all responses with a status code dimension? There's only a few common ones (200, 301, 400, 401, 404, 500, etc.).

Having sampling support in the loggers would also help deal with high volume output.

Submit this to the runtime/logging owners.

@Tratcher
Copy link
Member Author

I think we'd still want an info level log in the places indicated above, that covers our pattern of logging when we've taken decisive action on a request (e.g. produced a response, refused the request, etc.). The hosting level logs have a few drawbacks:

  • They cover every request, not just these unmatched ones. That's a lot more noise to sort through.
  • They don't address the question of avoiding logging paths with sensitive data. If we're only logging unmatched requests they're less likely to contain sensitive data.

@JamesNK JamesNK self-assigned this Mar 20, 2023
@Tratcher Tratcher closed this as completed May 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
@Tratcher
Copy link
Member Author

Tratcher commented Jun 5, 2023

The customer reviewed this and said that the host metric is not sufficient for fallback routes. They also requested tweaks for missed routes.

  • Fallback routes are not called out as fallbacks, you have to know which route pattern is which. These should still have their own metric for easy discovery and tracking.
  • Do not add another dimension identifying these as fallback or missed, too many dimensions.
  • Do not add a data within the dimension value ('fallback: {rout pattern}'), the tools don't want to parse the field to understand if it's a fallback route.
  • Missed routes should set the route description metadata to something like '-missed-' to be easily identified.

Note, since the logs are debug and info respectively, and in specific categories, dotnet/runtime#82465 is not needed at this time.

@Tratcher Tratcher reopened this Jun 5, 2023
@Tratcher Tratcher removed this from the 8.0-preview5 milestone Jun 5, 2023
@JamesNK
Copy link
Member

JamesNK commented Jun 6, 2023

What does a missed route mean? Is it a route that failed to match? That's easily identifiable because the "route" tag value will be null.

@Tratcher
Copy link
Member Author

Tratcher commented Jun 6, 2023

Missed in the sense that it reached the end of the pipeline without anything handling it. Other components won't have routes either like static files and auth callbacks.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
6 participants