Starlette: use route name as transaction name if available #957
Conversation
Unfortunately, we need to do some matching here that Starlette does too, but currently isn't exposed. If/when encode/starlette#804 is merged, we can re-use the route from there. closes elastic#833
🐛 Flaky test report❕ There are test failures but not known flaky tests. Expand to view the summary
Test stats 🧪
Genuine test errors
💔 There are test failures but not known flaky tests, most likely a genuine test failure.
|
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
|
@beniwohli As promised I've taken this for a spin! It works nicely, and is miles better than the previous implementation! Thank you :) There's a small thing that I guess is likely working as intended, but from a user perspective isn't the best experience. I made these two simple routes as a test: app = Starlette(routes=[
Route('/users/', list_users),
Route('/users/{pk:int}/', get_user),
])That is, I forced trailing slashes to my URLs. Attempting to make a request to the URL without the slash causes a redirect in Starlette, but the new code to get the route name couldn't match the route (obviously), causing this mismatch (the ID is listed instead of
This means if you had a big site, your transaction list would still be really cluttered with redirects containing the actual value for the URL path param. Not sure how easily that's fixable, but it would greatly improve the user experience :) |
There was a problem hiding this comment.
This looks like a huge improvement as is, one potential solution to the problems that @HenrikOssipoff is seeing below.
| def get_route_name(self, request: Request) -> str: | ||
| path = None | ||
| routes = request.scope["app"].routes | ||
| for route in routes: | ||
| match, _ = route.matches(request.scope) | ||
| if match == Match.FULL: | ||
| path = route.path | ||
| break | ||
| elif match == Match.PARTIAL and path is None: | ||
| path = route.path | ||
| return path |
There was a problem hiding this comment.
Based on @HenrikOssipoff's comment, I wonder if path is None when we go to return, should we try appending a "/" to path to attempt to get a match that way? That should solve the matching issues he's seeing, right? Slightly more overhead, but I still think it's worth it to avoid all of those redirect one-off transaction names.
|
@HenrikOssipoff @basepi I've implemented a fix for the issue with trailing slashes. In the end I decided to create a separate "bucket" for these transactions and not simply try to find a match with/without trailing slash. The latter would mix "real" requests with redirect requests, and that would give a false impression on the real requests' performance (as redirects will be extremely fast). WDYT? |
| for route in routes: | ||
| match, _ = route.matches(redirect_scope) | ||
| if match != Match.NONE: | ||
| route_name = "redirect trailing slashes" |
There was a problem hiding this comment.
| route_name = "redirect trailing slashes" | |
| route_name = "<internal redirect>" |
What do you think about this? I'm not sure the "trailing slashes" will mean anything to the average user.
I do like collecting all of these transactions in a separate bucket so that they don't affect the stats for the routes to which they redirect, I'm just not sure of the name.
There was a problem hiding this comment.
I struggled with naming, and you're probably right that my choice isn't super understandable by users not familiar with the internals of Starlette routing (especially because the Router.redirect_slashes flag seems to be undocumented and not configurable when instantiating a Starlette application). <internal redirect> seems to be very broad though, as we won't put any other kinds of redirects into that name bucket...
There was a problem hiding this comment.
Eh, I don't feel strongly. We can keep it as-is.
There was a problem hiding this comment.
Love the changes! Can't wait to see this in master.
As a user, I would probably expect the following behaviour:
| route_name = "redirect trailing slashes" | |
| route_name = route.path[:-1] |
I think that would be somewhat in line with what we see with other frameworks, that expose the route information on the request objects (I could be wrong though). This way, the routes are still collected in a separate bucket, but each "redirect route" has its own.
There was a problem hiding this comment.
@HenrikOssipoff I implemented your suggestion now. I'm a bit concerned that it could spam the transaction list with lots of these transaction names that are only redirects to real transactions, but I guess that's only a problem if the wrong URLs are used consistently.
# Conflicts: # elasticapm/contrib/starlette/__init__.py # tests/contrib/asyncio/starlette_tests.py
* Starlette: use route name as transaction name if available Unfortunately, we need to do some matching here that Starlette does too, but currently isn't exposed. If/when encode/starlette#804 is merged, we can re-use the route from there. closes elastic#833 * detect trailing slash redirects from Starlette and give them a dedicated transaction name * adapt original route for slash redirects
Unfortunately, we need to do some matching here that Starlette does
too, but currently isn't exposed. If/when encode/starlette#804
is merged, we can re-use the route from there.
closes #833