-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add more information to HTTP errors to help with debugging. #11878
Conversation
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
pub const DEBUG_HEADERS: &[&str] = &[ | ||
"x-amz-cf-id", | ||
"x-amz-cf-pop", | ||
"x-amz-id-2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a little nervous about special casing aws in perpetuity for all registries. We need these headers because at the moment aws is used for serving crates.io. But we may switch providers for crates.io and other registries already use their own stack. I wish there was a more flexible/future proof solution, but only if it doesn't add too much complexity. And I don't know what that looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda view this as a "best effort" approach where it provides diagnostics for the things we know could be useful now. I don't know what other headers would be useful, and I'm reluctant to show all of them because most headers just add a bunch of noise.
If someone wants to debug a reproducible issue, then they can use HTTP debugging to view all of the headers.
I realize this doesn't cover all cases. My balance here tips towards keeping the error messages a little cleaner as opposed to being exhaustive.
That being said, I'd be happy to go with the opposite approach of removing certain headers and showing the rest. That will be a bit more verbose than this approach, but still not as bad as showing all of them.
BTW, this also includes some fastly headers as well as aws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to add this, we may want to also include via
(for more proxies) and www-authenticate
(for authentication issues). Note that sparse registries currently do read www-authenticate
, but it's not displayed in errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of this information is also in the request body? The issue mentioned in Zulip appeared to have a good amount.
<?xml version="1.0" encoding="UTF-8"?><Error><Code>SlowDown</Code><Message>Please reduce your request rate.</Message><RequestId>HJT1QW870MWXTRJQ</RequestId><HostId>Fa1X04OlNaOF9iIoX5FB3NRlJhqQKjq2AbfZIDjGUw51zbRndr6f8mAMGtkExpsLbJZy2aazpDs=</HostId></Error>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request ID is in the body of that particular message. However, that's the first time I've ever seen an error message with it, so I'm not sure in which circumstances it actually shows up.
One issue is that we don't have direct access to the logs (which are currently not enabled), so having just a request ID would be a slow process of working with infra and AWS.
Knowing the POP and IP can allow us to try to diagnose and debug an issue. For example, in #8647, I was able to update my HOSTS file to the Mumbai pop and reproduce the issue.
Knowing other information, like cache hit status, can also help identify patterns of issues to further diagnose potential problems.
Also, I'm not sure what information other services like fastly will display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weihanglo Did you have any opinion on this or any other feedback on this PR? Although it is not critical, I think it might be nice to have something like this in 1.70.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document why some headers are included here? It is not a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I also added a header I missed.
And FWIW, I think the idea of logging all headers (minus the uninteresting ones) is also a good suggestion. I'm just concerned the error message could get quite large. I'm happy to go that route if others feel like it would be preferable.
☔ The latest upstream changes (presumably #11881) made this pull request unmergeable. Please resolve the merge conflicts. |
ed5b1c2
to
d74d0bd
Compare
I pushed a change so that the headers are not shown in the spurious warning message. I was concerned about the amount of output that might result from this (potentially many thousands of lines). It should now only show the headers in the final error message. |
☔ The latest upstream changes (presumably #11936) made this pull request unmergeable. Please resolve the merge conflicts. |
d74d0bd
to
118d0ad
Compare
☔ The latest upstream changes (presumably #11949) made this pull request unmergeable. Please resolve the merge conflicts. |
The headers can significantly contribute to noise in the output, drowning out the rest of the output. Most investigation will likely be focused on the case where cargo completely fails to download, so this only shows the full detail in the final error message.
118d0ad
to
c00a633
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Only one nit
pub const DEBUG_HEADERS: &[&str] = &[ | ||
"x-amz-cf-id", | ||
"x-amz-cf-pop", | ||
"x-amz-id-2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document why some headers are included here? It is not a blocker though.
Move on and merge this. If you have any objection please shout out. @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 4 commits in 7bf43f028ba5eb1f4d70d271c2546c38512c9875..84b7041fd2745ee6b3b4a150314f81aabb78e6b2 2023-04-10 16:01:41 +0000 to 2023-04-13 20:08:40 +0000 - Stabilize `cargo logout` (rust-lang/cargo#11950) - Add more information to HTTP errors to help with debugging. (rust-lang/cargo#11878) - Use registry.default for login/logout (rust-lang/cargo#11949) - Change -C to be unstable (rust-lang/cargo#11960)
…ehuss Make cargo a workspace 8 commits in 7bf43f028ba5eb1f4d70d271c2546c38512c9875..39116ccc9b420a883a98a960f0597f9cf87414b8 2023-04-10 16:01:41 +0000 to 2023-04-15 20:24:15 +0000 - Make cargo a workspace (rust-lang/cargo#11851) - Fix flaky not_found_permutations test. (rust-lang/cargo#11976) - Use restricted Damerau-Levenshtein algorithm (rust-lang/cargo#11963) - Correct the bug report for `cargo clippy --fix` (rust-lang/cargo#11882) - Stabilize `cargo logout` (rust-lang/cargo#11950) - Add more information to HTTP errors to help with debugging. (rust-lang/cargo#11878) - Use registry.default for login/logout (rust-lang/cargo#11949) - Change -C to be unstable (rust-lang/cargo#11960) --- ### What does this PR try to resolve? Making cargo a workspace. Why doing this? * `rustc-workspace-hack` is primarily for sharing dependencies between rls and cargo, as rls previously depends on cargo. After rls retired, it is no longer the case sharing dependencies. * It's q bit painful that cargo needs to deal with some dependency and licensing complexities. For example, rust-lang#108665 failed because of the interaction bewteen `windows-sys` and `raw-dylib`. It currenctly blocks cargo's feature `-Zgitxodie` from moving forward. * See rust-lang/cargo#11851 ### Benchmark result I've done a simple benchmark on both keeping or removing entire `rustc-workspace-hack`. It had no significant difference. Both took ~2m30s to finish `./x.py build -j8 src/tools/cargo src/tools/rls src/tools/clippy src/tools/miri src/tools/rustfmt`. Environment info: ``` host: aarch64-apple-darwin os: Mac OS 13.2.1 [64-bit] ``` A sophisticated benchmark may be needed. ### Additional information This depends on prior works from `@Muscraft` and `@ehuss.` Credits to them!
This adds some extra information to the HTTP error message about some headers and the remote IP address that could potentially be useful in diagnosing issues.
Closes #8691