starlette_exporter
starlette_exporter copied to clipboard
stephenhillier
fix: return the actual path instead of None on partial matches
If the match is partial we want the path of the route and not None, which would result in a dropped metric if filter_unhandled_paths is set to True and a request is made to an actually handled endpoint but with the wrong http method.
Thanks for opening a PR. Do you have a test case or an example of a route that would end up with a partial match (to help me test)?
Edit: ok, I reread your comment and I think I understand better. Wouldn't that just result in a 405: Method Not Allowed error though? Or are you saying you want to include 405 errors in metrics even though 404 errors are dropped?
Exactly. I want to know if existing routes are used wrong — but I do not want to have all the routes in my metrics that a malicious crawler might try.
@janLo I accidently pushed a commit to your fork branch (I didn't actually realize I could do that..). I added some test cases and meant to add them to my own branch. Feel free to revert it if you want.
One of the new tests fails which is for a partial match on a mounted route (a second router instance mounted to another one). I think changing the line at https://github.com/stephenhillier/starlette_exporter/pull/29/files#diff-20b93ddcf1235118346c28f13a10a6057c7772939f4a72cfbafc62d998d5aa3bL25 to if Match.FULL or Match.PARTIAL: will make that test pass. I just need a bit more time to test for any other edge cases.
FYI: There is an option to allow maintainers to push commits on the source-branch of the pull request ;). It's all file.
I originally thought it was just a bug because I just changed the no-op assignment that was there before to a return. I thought, returning the path for partial matches was the intended behavior. If it was not, I'm completely fine with making this behavior configurable.
The main blocker is that it doesn't work well with mounted routes (see the failing unit test for mounted routes in 9f99da4). If you have any ideas for mounted routes feel free to add them in, otherwise I can try to see what I can come up with (can't promise a timeline right now though).
Hi @janLo, not sure if you are still interested but I'm trying to revive this PR. I made some small changes.
The issue I'm hitting right now is that (as far as I know), a client can submit any arbitrary text as the HTTP method, not just GET, POST etc. That means the method label could become unconstrained if somebody purposefully makes bad requests.
As soon as I figure out a way to keep the method label constrained, I'll merge this in.
Hi @stephenhillier, thank ,ou for reaching out. I am still interested and this pr is still on my to-do list. I am also running the code from the original PR now for quite a while and can at least confirm, that it works.
If you want to limit the methods I would recommend to make the table from section 4.1 of https://www.rfc-editor.org/rfc/rfc7231#section-4 the default and maybe let the user add custom methods at configuration time.