Skip to content
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

Use path with query string on WebSockets logs #1385

Merged
merged 2 commits into from Sep 11, 2022

Conversation

levrik
Copy link
Contributor

@levrik levrik commented Feb 22, 2022

I've started to use get_path_with_query_string for websockets logging as well to have a single place responsible for creating the path for logging.
I don't know why this method wasn't used before so in case I'm missing something here, please let me know.

Fixes #1384

@levrik levrik changed the title Fix access logs not containing root path Fix access logs for mounted apps not containing root path Feb 22, 2022
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good,
could you please @aminalaee take a look at it since you fixed #1290 with #1294
I'm wondering here if #1290 was legit in fact, but not sure

@levrik
Copy link
Contributor Author

levrik commented Feb 22, 2022

I was looking into the linked issue encode/starlette#1336 and since FastAPI uses Starlette under the hood, this is probably where the issue originates from. From my point of view the fix done in #1290 is legit but causes some issues due to encode/starlette#1336.
The change to using raw_path might not be correct as well as path should be completed by spec but is broken in combination with Starlette. Maybe a revert to combining root_path and path should be done until Starlette is fixed? I'm feeling unsure about the change to raw_path.

@aminalaee
Copy link
Member

aminalaee commented Feb 22, 2022

@levrik Yes I think you're right and also changing back to root_path + path would make more sense as according the ASGI the raw_path can be optional.

The change to use get_path_with_query_string for websocket looks reasonable though.

Kludex
Kludex approved these changes Sep 11, 2022
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @levrik :)

@Kludex Kludex changed the title Fix access logs for mounted apps not containing root path Use path with query string on WebSockets logs Sep 11, 2022
@Kludex Kludex merged commit e2164fb into encode:master Sep 11, 2022
12 checks passed
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex added a commit that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access logs for mounted apps doesnt not contain the root path
4 participants