fix(logging): record the complete request path for variant endpoints - #862
Open
davereinhart wants to merge 1 commit into
Open
fix(logging): record the complete request path for variant endpoints#862davereinhart wants to merge 1 commit into
davereinhart wants to merge 1 commit into
Conversation
PopulatedRawContextMiddleware took the request path from request.url.path. Starlette builds that URL
by reassembling the scope into a string and re-parsing it, so urlsplit reads everything after the
first '#' as a fragment. A variant URN is {score_set_urn}#{n}, which means the path in the canonical
log line stopped at the score set: the variant number and the sub-resource were both dropped, and so
was the query string, which sits after the '#' in the reassembled string. /variants/{urn},
/variants/{urn}/csv and /variants/{urn}/csv-namespaces logged one indistinguishable path, and no log
line recorded which variant had been asked for.
Read the path from the ASGI scope, which arrives percent-decoded and whole. ASGI guarantees 'path' on
both http and websocket scopes, so this needs none of the defensiveness the method read below it
carries, where only an http scope has the key.
Nothing reads this context key. log_request serializes the context into the "Request completed." line
and no behaviour depends on the value, so the change is confined to what the logs say.
lib/slack.py interpolates request.url whole, and URL.__str__ returns the unparsed string, so the
fragment survives there and it was never affected.
Coverage Report for CI Build 33686096846Warning No base build found for commit Coverage: 88.841%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request updates how the request path is logged in the context for incoming requests. Instead of using
request.url.path, which could incorrectly truncate paths at the#character, it now uses the ASGI scope'spathdirectly to ensure the full path is captured. This change addresses an issue where different/variantsroutes were being logged as the same path.Logging improvements:
set_contextmethod incontext.pyto userequest.scope["path"]instead ofrequest.url.pathfor more accurate logging of request paths, especially for variant URNs containing#characters.