Consider using route information instead of URL for Starlette transactions #833
Comments
HenrikOssipoff
changed the title
|
The Starlette support is new enough I'd be willing to entertain a change like this. I would much rather have route information than the full path, but like you mentioned, it's unavailable without manually calculating the route. My only concern with that solution is that it adds some processing overhead. But I do think it would give us more useful transaction names, and hopefully they'll add an official way to get that information in middleware. @HenrikOssipoff is this a change for which you'd be willing to open a PR? |
|
@basepi Definitely, I'll give it a go in the near future. Would enable us to use Starlette much more than we currently do :) |
|
@HenrikOssipoff Is this a change you're still interested in contributing? |
@basepi There is a solution mentioned here fastapi/fastapi#486 (comment), but it needs to iterate through all the routes for each request. path = [route for route in request.scope['router'].routes if route.endpoint == request.scope['endpoint']][0].path |
|
Thanks. I think that's probably worthwhile overhead, the current implementation isn't super great. |
Can't wait for the new version. Maybe we can make it an optional choice via an env variable. |
|
I'm honestly still leaning towards just making the change without backwards compatibility considerations. The starlette support is very new and the current behavior could probably be considered a bug. Thoughts? |
In my use case, I am happy to fix this one a bug. I don't need old behavior. |
Sorry very late to the game - I can see @beniwohli has assigned himself, which is more than fine by me. Work's not leaving me much free time unfortunately. Looking forward to the "fix" though - sorry for being slow here. Personally I would say you shouldn't think about backwards compatibility for this one, given most likely very few people even use this particular integration, but that's my opinion alone. |
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
|
@HenrikOssipoff I opened #957 with (hopefully) a fix for this. If you have time, would you mind giving that a quick spin and see if it works? I agree that backwards compatibility is not a major concern. I would consider the current behavior as buggy compared to our other integrations. |
|
@beniwohli Sweet I'll take it for a spin later tonight to test things out :) |
* 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 #833 * detect trailing slash redirects from Starlette and give them a dedicated transaction name * adapt original route for slash redirects
* 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
We're slowly started using Starlette for some smaller applications due to its simplicity and async capabilities. We also happen to use Elastic APM for our monitoring, but we've found that the agent works quite different from other agents, such as Django or Flask.
The default for Django, is "HTTP Method + path.to.view". This can be changed for Django 2.2+ using the
django-transaction-name-from-routesetting, to instead use the route that was matched against. For exampleGET /users/<int:user_id>/.For Flask, the default seems to be the that of method + route, e.g.:
GET /users/<int:user_id/.For Starlette however, information about the matched route is not actually available on the request, and for that reason (I assume), the full URL is instead used as the transaction name.
In the example above, it would mean that every time a user with a different URL was hit, it would generate two separate transaction entries. For some of our views, that would mean 1+ milllion transaction names for the same view.
However, it is somewhat possible to extract the route by mimicking what Starlette itself does. On the request, the Application itself is available, and from that route router + routes. This means it's possible to loop over them, and match their regular expression against the current request.
What would your thought be on such a change?
For us at our workplace, we've actually have one major of our main applications ported to Starlette, but once we saw what was happening to the transaction names, we had to revert the deployment. We'd love to have it running, so we'd probably be interested in providing a patch for it as well.
EDIT
Just wanted to add this here, since it's a discussion of making this type of information available in the middleware system: encode/starlette#685
It also links another repo for timing of ASGI that basically does what I describe here, by looping over all routes and comparing them to the current URL.
The text was updated successfully, but these errors were encountered: