Skip to content

Commit

Permalink
feat: improve httputil error messages (#624)
Browse files Browse the repository at this point in the history
* 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.)
  • Loading branch information
jjmaestro authored Nov 5, 2024
1 parent d20eff9 commit 22d3367
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 4 deletions.
2 changes: 1 addition & 1 deletion httputil/httputil.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func get(url, auth string) (*http.Response, error) {

nextTryAt := RetryClock.Now().Add(waitFor)
if nextTryAt.After(deadline) {
return nil, fmt.Errorf("unable to complete request to %s within %v", url, MaxRequestDuration)
return nil, fmt.Errorf("unable to complete %d requests to %s within %v. Most recent failure: %s", attempt+1, url, MaxRequestDuration, lastFailure)
}
if attempt < MaxRetries {
RetryClock.Sleep(waitFor)
Expand Down
10 changes: 7 additions & 3 deletions httputil/httputil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"net/http"
"strconv"
"strings"
"testing"
"time"
)
Expand Down Expand Up @@ -219,10 +220,13 @@ func TestDeadlineExceeded(t *testing.T) {
t.Fatal("Expected request to fail with code 500")
}

wanted := "could not fetch http://bar: unable to complete request to http://bar within 8s"
wantedPrefix := "could not fetch http://bar: unable to complete "
wantedSuffix := " requests to http://bar within 8s. Most recent failure: HTTP 500"

got := err.Error()
if wanted != got {
t.Fatalf("Expected error %q, but got %q", wanted, got)
sameError := strings.HasPrefix(got, wantedPrefix) && strings.HasSuffix(got, wantedSuffix)
if !sameError {
t.Fatalf("Expected error %q, but got %q", wantedPrefix+"???"+wantedSuffix, got)
}
}

Expand Down

0 comments on commit 22d3367

Please sign in to comment.