address comments

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,

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

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())
 }
 

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