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

feat: improve httputil error messages #624

Merged

Conversation

jjmaestro
Copy link
Contributor

  • add attempt count to the timeout error message so it's clear the timeout is due to the total time accumulated during retries.

  • add lastFailure to the timeout error message so we can get a clear reason for the retries (or, at the very least, the last retry). Otherwise, the error message obscures the real reason(s) for the retries and potentially misleads users into thinking that the errors could be e.g. network related (more typical of a timeout) rather than other types of errors (e.g. "tls: failed to verify certificate", etc.)

@jjmaestro
Copy link
Contributor Author

The motivation behind this PR was my own debugging creating a Docker image where I was getting constant timeouts and I eventually figured it was due to not having ca-certificates installed in my Debian-based image. I debugged it by patching the error messages with this PR.

I hope it's also useful for others!

@jjmaestro jjmaestro changed the title fix: improve httputil error messages feat: improve httputil error messages Oct 23, 2024
* add attempt count to the timeout error message so it's clear the
  timeout is due to the total time accumulated during retries.

* add lastFailure to the timeout error message so we can get a clear
  reason for the retries (or, at the very least, the last retry).
  Otherwise, the error message obscures the real reason(s) for the
  retries and potentially misleads users into thinking that the errors
  could be e.g. network related (more typical of a timeout) rather than
  other types of errors (e.g. "tls: failed to verify certificate", etc.)
@jjmaestro jjmaestro force-pushed the fix-improve-timeout-error-messages branch from 091c746 to 0430338 Compare October 23, 2024 15:01
@meteorcloudy meteorcloudy requested a review from fweikert November 5, 2024 16:20
@fweikert fweikert merged commit 22d3367 into bazelbuild:master Nov 5, 2024
2 checks passed
@jjmaestro jjmaestro deleted the fix-improve-timeout-error-messages branch November 5, 2024 18:31
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.

2 participants