diff --git a/apierr/errors.go b/apierr/errors.go index 550a2b15b..38925a314 100644 --- a/apierr/errors.go +++ b/apierr/errors.go @@ -223,8 +223,12 @@ func parseUnknownError(resp *http.Response, requestBody, responseBody []byte, er } func MakeUnexpectedError(resp *http.Response, err error, requestBody, responseBody []byte) error { + var req *http.Request + if resp != nil { + req = resp.Request + } rts := httplog.RoundTripStringer{ - Request: resp.Request, + Request: req, Response: resp, Err: err, RequestBody: requestBody, diff --git a/httpclient/api_client.go b/httpclient/api_client.go index 5296bb7c4..ff3163fca 100644 --- a/httpclient/api_client.go +++ b/httpclient/api_client.go @@ -194,7 +194,7 @@ func (c *ApiClient) isRetriable(ctx context.Context, err error) bool { // If it is certain that an error should not be retried, use failRequest() instead. func (c *ApiClient) handleError(ctx context.Context, err error, body common.RequestBody) (*common.ResponseWrapper, *retries.Err) { if !c.isRetriable(ctx, err) { - return c.failRequest(ctx, "non-retriable error", err) + return nil, retries.Halt(err) } if resetErr := body.Reset(); resetErr != nil { return nil, retries.Halt(resetErr) @@ -203,8 +203,8 @@ func (c *ApiClient) handleError(ctx context.Context, err error, body common.Requ } // Fails the request with a retries.Err to halt future retries. -func (c *ApiClient) failRequest(ctx context.Context, msg string, err error) (*common.ResponseWrapper, *retries.Err) { - logger.Debugf(ctx, "%s: %s", msg, err) +func (c *ApiClient) failRequest(msg string, err error) (*common.ResponseWrapper, *retries.Err) { + err = fmt.Errorf("%s: %w", msg, err) return nil, retries.Halt(err) } @@ -218,7 +218,7 @@ func (c *ApiClient) attempt( return func() (*common.ResponseWrapper, *retries.Err) { err := c.rateLimiter.Wait(ctx) if err != nil { - return c.failRequest(ctx, "failed in rate limiter", err) + return c.failRequest("failed in rate limiter", err) } pctx := ctx @@ -229,12 +229,12 @@ func (c *ApiClient) attempt( ctx, ticker := newTimeoutContext(pctx, c.config.HTTPTimeout) request, err := http.NewRequestWithContext(ctx, method, requestURL, requestBody.Reader) if err != nil { - return c.failRequest(ctx, "failed creating new request", err) + return c.failRequest("failed creating new request", err) } for _, requestVisitor := range visitors { err = requestVisitor(request) if err != nil { - return c.failRequest(ctx, "failed during request visitor", err) + return c.failRequest("failed during request visitor", err) } } // Set traceparent for distributed tracing. @@ -284,7 +284,7 @@ func (c *ApiClient) attempt( err = c.config.ErrorMapper(ctx, responseWrapper) } - defer c.recordRequestLog(ctx, request, response, err, requestBody.DebugBytes, responseWrapper.DebugBytes) + c.recordRequestLog(ctx, request, response, err, requestBody.DebugBytes, responseWrapper.DebugBytes) if err == nil { return &responseWrapper, nil diff --git a/httpclient/api_client_test.go b/httpclient/api_client_test.go index 6e3191197..2c25ba556 100644 --- a/httpclient/api_client_test.go +++ b/httpclient/api_client_test.go @@ -188,7 +188,7 @@ func TestHaltAttemptForLimit(t *testing.T) { _, rerr := c.attempt(ctx, "GET", "foo", req)() require.NotNil(t, rerr) require.Equal(t, true, rerr.Halt) - require.EqualError(t, rerr.Err, "rate: Wait(n=1) exceeds limiter's burst 0") + require.EqualError(t, rerr.Err, "failed in rate limiter: rate: Wait(n=1) exceeds limiter's burst 0") } func TestHaltAttemptForNewRequest(t *testing.T) { @@ -199,7 +199,7 @@ func TestHaltAttemptForNewRequest(t *testing.T) { _, rerr := c.attempt(ctx, "🥱", "/", req)() require.NotNil(t, rerr) require.Equal(t, true, rerr.Halt) - require.EqualError(t, rerr.Err, `net/http: invalid method "🥱"`) + require.EqualError(t, rerr.Err, `failed creating new request: net/http: invalid method "🥱"`) } func TestHaltAttemptForVisitor(t *testing.T) { @@ -213,7 +213,7 @@ func TestHaltAttemptForVisitor(t *testing.T) { })() require.NotNil(t, rerr) require.Equal(t, true, rerr.Halt) - require.EqualError(t, rerr.Err, "🥱") + require.EqualError(t, rerr.Err, "failed during request visitor: 🥱") } func TestFailPerformChannel(t *testing.T) { @@ -353,18 +353,18 @@ func (l *BufferLogger) Errorf(_ context.Context, format string, v ...interface{} l.WriteString(fmt.Sprintf("[ERROR] "+format+"\n", v...)) } -func configureBufferedLogger() (*BufferLogger, func()) { +func configureBufferedLogger(t *testing.T) *BufferLogger { prevLogger := logger.DefaultLogger bufLogger := &BufferLogger{} logger.DefaultLogger = bufLogger - return bufLogger, func() { + t.Cleanup(func() { logger.DefaultLogger = prevLogger - } + }) + return bufLogger } func TestSimpleResponseRedaction(t *testing.T) { - bufLogger, resetLogger := configureBufferedLogger() - defer resetLogger() + bufLogger := configureBufferedLogger(t) c := NewApiClient(ClientConfig{ DebugTruncateBytes: 16, @@ -407,8 +407,7 @@ func TestSimpleResponseRedaction(t *testing.T) { } func TestInlineArrayDebugging(t *testing.T) { - bufLogger, resetLogger := configureBufferedLogger() - defer resetLogger() + bufLogger := configureBufferedLogger(t) c := NewApiClient(ClientConfig{ DebugTruncateBytes: 2048, @@ -443,8 +442,7 @@ func TestInlineArrayDebugging(t *testing.T) { } func TestInlineArrayDebugging_StreamResponse(t *testing.T) { - bufLogger, resetLogger := configureBufferedLogger() - defer resetLogger() + bufLogger := configureBufferedLogger(t) c := NewApiClient(ClientConfig{ DebugTruncateBytes: 2048, @@ -473,8 +471,7 @@ func TestInlineArrayDebugging_StreamResponse(t *testing.T) { } func TestLogQueryParametersWithPercent(t *testing.T) { - bufLogger, resetLogger := configureBufferedLogger() - defer resetLogger() + bufLogger := configureBufferedLogger(t) c := NewApiClient(ClientConfig{ DebugTruncateBytes: 2048, @@ -502,8 +499,7 @@ func TestLogQueryParametersWithPercent(t *testing.T) { } func TestLogCancelledRequest(t *testing.T) { - bufLogger, resetLogger := configureBufferedLogger() - defer resetLogger() + bufLogger := configureBufferedLogger(t) ctx, cancel := context.WithCancel(context.Background()) c := NewApiClient(ClientConfig{ @@ -515,9 +511,9 @@ func TestLogCancelledRequest(t *testing.T) { }) err := c.Do(context.Background(), "GET", "/a") assert.Error(t, err) - assert.Equal(t, `[DEBUG] non-retriable error: Get "/a": request timed out after 30s of inactivity -[DEBUG] GET /a + assert.Equal(t, `[DEBUG] GET /a < Error: Get "/a": request timed out after 30s of inactivity +[DEBUG] non-retriable error: Get "/a": request timed out after 30s of inactivity `, bufLogger.String()) } diff --git a/retries/retries.go b/retries/retries.go index 47572f388..8517e0e07 100644 --- a/retries/retries.go +++ b/retries/retries.go @@ -210,6 +210,7 @@ func (r Retrier[T]) Run(ctx context.Context, fn func(context.Context) (*T, error return entity, nil } if !r.config.ShouldRetry(err) { + logger.Debugf(ctx, "non-retriable error: %s", err) return nil, err } lastErr = err