Repository navigation
Log the egress evidence an unexplained 429 needs - #171
Merged
Merged
Conversation
Two red tests, no production change: - handleChatEgress: a 4xx/5xx from GitHub on a credentialed request is logged once with method, host, path, the credential actually sent, the status, GitHub's request id and the retry-after / x-ratelimit-* headers, correlated by session and turn, and never the token. A 200 is not logged. - makeEgressContext: a workspace control request that fails is logged with the query op and the reason, instead of silently forwarding the request with no credential.
- handleChatEgress logs one warning when GitHub or Multica answers 401, 403, 429 or 5xx, with the method, host, path (no query string), the credential actually sent (read, write, multica or none), the status, and GitHub's request id, retry-after and x-ratelimit-* headers when present. The token, the body and the request headers never go into the log. - makeEgressContext logs a warning, with the query op and the reason, when a failed tokens or multica control request means the request goes out with no credential. Request behaviour doesn't change, and nothing is retried.
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
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.
Janitor's sandbox pushes hit two HTTP 429s that we can't explain after the fact. The egress proxy keeps no record of what upstream answered, and it doesn't record when it forwarded a request without a credential because the workspace couldn't be reached. This PR adds the two log lines that would tell those cases apart next time. Request behaviour doesn't change, and nothing is retried.
handleChatEgresslogs one warning when a GitHub or Multica request comes back 401, 403, 429 or 5xx. The warning carries the method, host, path (no query string), the credential actually sent (read,write,multicaornone) and the status. When present, it also carriesx-github-request-id,retry-after,x-ratelimit-remaining,x-ratelimit-resetandx-ratelimit-resource. Session and turn ids come from the existing log annotations. The token, the body and the request headers are never logged.makeEgressContextlogs a warning with the query op and the reason when atokensormulticacontrol request fails. Before, that failure was swallowed and the request went out anonymously.Tests are from the investigation commit
0fb33e4and are unchanged.Closes EFF-3200