Added route value to scope #804
Conversation
|
Hi @tomchristie, would you mind helping me here, if possible? |
|
Needs some thinking about: Do we need to track any Mount that might have been matched too? Probably, otherwise eg. you can't actually build up the matched path. Perhaps we need eg. |
|
Hmm, and inside each of the matching routes can have a filed saying if it was a full match or partial match? |
|
@tomchristie I added the routes to the scope as a draft to what you purposed. If you like this solution, I would gladly add tests. I maintained the |
|
@tomchristie what are your thoughts here? Can I proceed writing tests with this solution? Or should I change something? |
|
@tomchristie any updates here? |
|
I'd rather we don't include The |
|
@tomchristie updated to what I understood about you comment! |
|
Hi @tomchristie and @florimondmanca! I see this pr was mentioned in many places! Can we get this moving? I will update the PR when I wake up (it's pretty late here) but would like to know if you guys are ok with this approach! As I see, with this list of routes, the user would need to iterate over the list and join the path values for the Mounts and finally the Route, is that right? Also, if that's the case, should we save the final full route value too? |
|
PR updated @tomchristie @florimondmanca ! |
| @@ -557,17 +560,20 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |||
|
|
|||
| partial = None | |||
|
|
|||
| scope["routes"] = [] | |||
There was a problem hiding this comment.
Hmm, perhaps I'm misunderstanding the aim of this — assuming we want routes to contain all BaseRoute items (mounts, routes) that have matched while going through the routing system, it seems that currently it will only ever contain a single item, i.e. the first one that matched? In case of a route under a Mount-ed app, it will be the Mount (even if the mounted app is actually a Starlette app perhaps with its own router), and in case of a regular route then it will be that Route. Correct?
There was a problem hiding this comment.
Basing my judgement off #804 (comment) here - Not entirely sure that's something that's actually possible at all, given the ASGI-interoperable design aspects of Starlette.
For example, if you have…
async def other_non_starlette_app(scope, receive, send):
...
app = Starlette(routes=[Mount("/subapp", other_non_starlette_app)])Then in this example context using scope["routes"][-1] as a way to get the "route that actually responds to the request" might be a bit confusing — it's only guaranteed to be the last route within this app that handles the request at least partly.
Don't know if we want to play on words here, or if for the use case you're describing (manipulating route information for telemetry purposes) having partial route info in some cases and marking it as a limitation would be enough?
There was a problem hiding this comment.
it seems that currently it will only ever contain a single item, i.e. the first one that matched?
Yes, you are right! I didn't proceed with the PR because wanted to discuss the best strategy with the maintainers before and wasn't sure this was it.
There was a problem hiding this comment.
Not entirely sure that's something that's actually possible at all, given the ASGI-interoperable design aspects of Starlette.
An example would be subapps on a FastAPI app? If so, I posted some code on a comment here that works by iterating the routes and searching for a match, but I don't think the problem you described would happen with FastAPI :/
|
I think I understand the need to get the route used for a request, but I'm a bit unclear on exactly how the routes would be used, especially when there are multiple routes. Maybe some test cases would make that clearer? |
What do you mean by multiple routes? Are you referring to having multiple Mounts and then finally a Route? I didn't write test cases because I wanted to discuss the best strategy to implement this before doing so. In my mind the ideal is getting a single value from scope that represents the full route. This means of the user have sub apps, sub routers and etc, the value would be there with the full route. But at the same type, there is a problem here. As routes are only matched after middlewares,and I think this would be the most common place to use the route value, thinking of a way to add middlewares after routes were matched, would be a nice addition. |
|
Just and example of what I do in a middleware today to get this value (it could be improved): route = None
for r in request.app.router.routes:
if r.matches(request.scope)[0] == Match.FULL:
route = r
break
complete_route = ""
while isinstance(route, Mount):
complete_route += route.path
scope = dict(request.scope)
scope["path"] = scope.get("path").replace(route.path, "")
matched = False
for r in route.routes:
if r.matches(scope)[0] == Match.FULL:
route = r
matched = True
break
if matched is False:
route = None
complete_route += (
route.path
if route is not None
else request.url.path
) |
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
* 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
|
Hi. Currently running into this issue. Any progress on this? Something that needs help? |
* 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
|
Hi @victoraugustolls 👋 Can you share the details behind closing this PR? Could be useful in open-telemetry/opentelemetry-python-contrib#1759. Currently OpenTelemetry handle this manually. |
The purpose of this pr is to add the RAW value of a path (route) to the scope.
Recently I tried to get this value and it didn't seem possible. Don't know if I did it the best way possible, but it seems so.
The main reason (at least for me) for having this value available is to aggregate values based on each endpoint in metric systems (Microsoft Application Insights, New Relic, Datadog...).
Please let me know if I need to change something or if this is not desirable!
Thanks!