-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add routing metrics #48637
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 routing metrics #48637
Conversation
|
||
public void MatchFailure() | ||
{ | ||
_matchFailureCounter.Add(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to be clear in the docs that there are lots of requests handled by middleware that will be reported as route match failures. E.g. static files, auth callbacks, etc. I wonder if that will make this counter more noise than value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tekian ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to be clear in the docs that there are lots of requests handled by middleware that will be reported as route match failures. E.g. static files, auth callbacks, etc. I wonder if that will make this counter more noise than value.
Can you show an example of this? I'm not sure how desirable this is. We need to verify this for all sorts of applications. (blazor to apis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this counter more, and I think we should keep it.
- It doesn't cost anything to have it. Implementation is simple, and the counter doesn't hurt perf if no one listens. If someone doesn't care about it, then they won't listen.
- If people see a counter that reports route matching success, they'll expect a counter that reports failure. Provides symmetry.
- Potential confusion about what the counter reports and what it means can be explained in the description. The description can say that unmatched requests may be handled later by other middleware, such as static files or authentication callbacks. The description could even recommend
http-server-unhandled-requests
if you want a counter for requests that go nowhere.
I'll make these changes and then merge this PR soon. If I don't hear feedback, I'll assume people are okay with this. It's not an end to discussion; I don't want to hold up metrics progress towards done.
cc @geeknoid |
1eadab1
to
f0ef1d7
Compare
API review: #48670 |
a099073
to
80cc820
Compare
@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
@JamesNK Marking this PR as blog-candidate, but we probably want to mention all the metrics work. |
Fixes #46404
Note that the route is still added as a tag to the hosting HTTP request counter. The intent of the new routing counters is to provide additional detail:
Ready for review but blocked on merging until API review of counter names/description is done.