Repository: newrelic/newrelic-client-go
Pull request #637 (2021-01-22 17:32:11)

fix(http): display underlying errors on max retries


Patch:
diff --git a/internal/http/client.go b/internal/http/client.go
index 81e8d89..6529475 100644
--- a/internal/http/client.go
+++ b/internal/http/client.go
@@ -337,11 +337,15 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
 	for i := 0; ; i++ {
 		var shouldRetry bool
 		var err error
-
 		errorValue = req.errorValue.New()
 		resp, body, shouldRetry, err = c.innerDo(req, errorValue, i)
-		if err != nil {
-			return nil, err
+
+		if serr, ok := err.(*nrErrors.NotFound); ok {
+			return nil, serr
+		}
+
+		if serr, ok := err.(*nrErrors.MaxRetriesReached); ok {
+			return nil, serr
 		}
 
 		if !shouldRetry {
@@ -361,10 +365,6 @@ func (c *Client) Do(req *Request) (*http.Response, error) {
 		return nil, nrErrors.NewNotFound("resource not found")
 	}
 
-	if errorValue.IsRetryableError() {
-		return nil, nrErrors.NewTimeout("server timeout")
-	}
-
 	if errorValue.Error() != "" {
 		return nil, errors.New(errorValue.Error())
 	}
@@ -457,7 +457,7 @@ func (c *Client) innerDo(req *Request, errorValue ErrorResponse, i int) (*http.R
 	remain := c.client.RetryMax - i
 	if remain <= 0 {
 		c.logger.Debug(fmt.Sprintf("giving up after %d attempts", c.client.RetryMax), "method", req.method, "url", r.URL)
-		return resp, body, false, nil
+		return resp, body, false, nrErrors.NewMaxRetriesReached(errorValue.Error())
 	}
 
 	wait := c.client.Backoff(c.client.RetryWaitMin, c.client.RetryWaitMax, i, resp)
diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go
index 8b1a1de..a5063e4 100644
--- a/pkg/errors/errors.go
+++ b/pkg/errors/errors.go
@@ -34,34 +34,6 @@ func (e *NotFound) Error() string {
 	return e.err
 }
 
-// NewTimeout returns a new instance of Timeout with an optional custom error message.
-func NewTimeout(err string) *Timeout {
-	e := Timeout{
-		err: err,
-	}
-
-	return &e
-}
-
-// NewTimeoutf returns a new instance of Timeout
-// with an optional formatted custom error message.
-func NewTimeoutf(format string, args ...interface{}) *Timeout {
-	return NewTimeout(fmt.Sprintf(format, args...))
-}
-
-// Timeout is returned when the target resource cannot be located.
-type Timeout struct {
-	err string
-}
-
-func (e *Timeout) Error() string {
-	if e.err == "" {
-		return "server timeout"
-	}
-
-	return e.err
-}
-
 // NewUnexpectedStatusCode returns a new instance of UnexpectedStatusCode
 // with an optional custom message.
 func NewUnexpectedStatusCode(statusCode int, err string) *UnexpectedStatusCode {
@@ -119,3 +91,27 @@ func (e *UnauthorizedError) Error() string {
 
 	return msg
 }
+
+// NewMaxRetriesReached returns a new instance of MaxRetriesReached with an optional custom error message.
+func NewMaxRetriesReached(err string) *MaxRetriesReached {
+	e := MaxRetriesReached{
+		err: err,
+	}
+
+	return &e
+}
+
+// NewMaxRetriesReachedf returns a new instance of MaxRetriesReached
+// with an optional formatted custom error message.
+func NewMaxRetriesReachedf(format string, args ...interface{}) *MaxRetriesReached {
+	return NewMaxRetriesReached(fmt.Sprintf(format, args...))
+}
+
+// MaxRetriesReached is returned when the target resource cannot be located.
+type MaxRetriesReached struct {
+	err string
+}
+
+func (e *MaxRetriesReached) Error() string {
+	return fmt.Sprintf("maximum retries reached: %s", e.err)
+}


Test patch:
diff --git a/internal/http/client_test.go b/internal/http/client_test.go
index 242c6c2..8219fd0 100644
--- a/internal/http/client_test.go
+++ b/internal/http/client_test.go
@@ -94,6 +94,7 @@ func TestUnauthorizedErrorValue(t *testing.T) {
 
 	// Ensure our custom 401 unauthorized error message is returned
 	assert.Contains(t, err.Error(), "Invalid credentials provided")
+	assert.IsType(t, &errors.UnauthorizedError{}, err)
 }
 
 type CustomErrorResponse struct {
@@ -321,16 +322,41 @@ func TestRetryOnNerdGraphTimeout(t *testing.T) {
 	attempts := 0
 	c := NewTestAPIClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
-		_, _ = w.Write([]byte(`{"errors":[{"message": "error", "extensions":{"errorClass":"TIMEOUT"}}]}`))
+		_, _ = w.Write([]byte(`{"errors":[{"message": "some error", "extensions":{"errorClass":"TIMEOUT"}}]}`))
 		attempts++
 	}))
 
+	c.client.RetryWaitMax = 10 * time.Millisecond
+
+	c.errorValue = &GraphQLErrorResponse{}
+	_, err := c.Get(c.config.Region().NerdGraphURL("path"), nil, nil)
+
+	assert.Equal(t, 4, attempts)
+	assert.Error(t, err)
+	assert.IsType(t, &errors.MaxRetriesReached{}, err)
+	assert.Contains(t, err.Error(), "maximum retries reached")
+	assert.Contains(t, err.Error(), "some error")
+}
+
+func TestRetryOnNerdGraphInternalServerError(t *testing.T) {
+	t.Parallel()
+	attempts := 0
+	c := NewTestAPIClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"errors":[{"message": "some error", "extensions":{"errorClass":"INTERNAL_SERVER_ERROR"}}]}`))
+		attempts++
+	}))
+
+	c.client.RetryWaitMax = 10 * time.Millisecond
+
 	c.errorValue = &GraphQLErrorResponse{}
 	_, err := c.Get(c.config.Region().NerdGraphURL("path"), nil, nil)
 
 	assert.Equal(t, 4, attempts)
 	assert.Error(t, err)
-	assert.IsType(t, &errors.Timeout{}, err)
+	assert.IsType(t, &errors.MaxRetriesReached{}, err)
+	assert.Contains(t, err.Error(), "maximum retries reached")
+	assert.Contains(t, err.Error(), "some error")
 }
 
 func TestInternalServerError(t *testing.T) {