fix(logging): resolve caller identity before early-return rejections - #133
Merged
Merged
Conversation
`origin`, `client`, `session_id` and `request_id` were derived only after routing succeeded, but three rejections log a response row before that point — unroutable model, missing provider key, and the new streaming refusal. Every one of those rows carried "origin": null. Caught by grepping the live pod straight after deploying the streaming rejection: the row was there, but anonymous. That defeats the reason it is logged — "who is asking for streaming?" needs the calling app, not just a model id and a timestamp. Resolve identity immediately after auth, ahead of all three rejections. This also fixes the pre-existing gap on the `unrouted` path, so a model-id miss is attributable to the app that sent it. Both rejection tests now assert origin and request_id are captured; reverting the hoist fails them.
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
1 participant
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.
Follow-up to #131, caught by verifying the deploy rather than trusting it.
Right after the rollout I checked that the new streaming rejection actually logged, and it did — but anonymously:
The request was sent with
Origin: https://deploy-verify.local.Cause
origin,client,session_idandrequest_idare derived after routing succeeds, but three rejections log a response row before reaching that point:unrouted) — pre-existingstream: truewith a 400 instead of ignoring it (#129) #131So all three logged
"origin": null. For the streaming row that defeats the stated reason it's logged at all: "who is asking for streaming?" needs the calling app, not just a model id and a timestamp. Same forunrouted— a model-id miss you can't attribute to an app is much less actionable.Fix
Resolve caller identity immediately after auth, ahead of all three rejections, and pass it to each. No behavior change beyond what lands in the log rows.
Tests
Both rejection tests now assert
originandrequest_idare captured. Mutation-checked: reverting the hoist fails both (2 failed, 1 passed), so they're load-bearing.pytest test_logging.py— 59 passed.