notify: refactor code to retry requests (#1974)

* notify: refactor code to retry requests Signed-off-by: Simon Pasquier <spasquie@redhat.com> * s/Process/Check/ Signed-off-by: Simon Pasquier <spasquie@redhat.com>
prometheus · Aug 2, 2019 · 655947d · 655947d
1 parent f45f870
commit 655947d
Show file tree

Hide file tree

Showing 16 changed files with 334 additions and 265 deletions.
diff --git a/notify/hipchat/hipchat.go b/notify/hipchat/hipchat.go
@@ -31,10 +31,11 @@ import (
 
 // Notifier implements a Notifier for Hipchat notifications.
 type Notifier struct {
-	conf   *config.HipchatConfig
-	tmpl   *template.Template
-	logger log.Logger
-	client *http.Client
+	conf    *config.HipchatConfig
+	tmpl    *template.Template
+	logger  log.Logger
+	client  *http.Client
+	retrier *notify.Retrier
 }
 
 // New returns a new Hipchat notification handler.
@@ -48,6 +49,10 @@ func New(c *config.HipchatConfig, t *template.Template, l log.Logger) (*Notifier
 		tmpl:   t,
 		logger: l,
 		client: client,
+		// Response codes 429 (rate limiting) and 5xx can potentially recover.
+		// 2xx response codes indicate successful requests.
+		// https://developer.atlassian.com/hipchat/guide/hipchat-rest-api/api-response-codes
+		retrier: &notify.Retrier{RetryCodes: []int{http.StatusTooManyRequests}},
 	}, nil
 }
 
@@ -103,16 +108,5 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	}
 	defer notify.Drain(resp)
 
-	return n.retry(resp.StatusCode)
-}
-
-func (n *Notifier) retry(statusCode int) (bool, error) {
-	// Response codes 429 (rate limiting) and 5xx can potentially recover.
-	// 2xx response codes indicate successful requests.
-	// https://developer.atlassian.com/hipchat/guide/hipchat-rest-api/api-response-codes
-	if statusCode/100 != 2 {
-		return (statusCode == 429 || statusCode/100 == 5), fmt.Errorf("unexpected status code %v", statusCode)
-	}
-
-	return false, nil
+	return n.retrier.Check(resp.StatusCode, nil)
 }
diff --git a/notify/hipchat/hipchat_test.go b/notify/hipchat/hipchat_test.go
@@ -27,10 +27,17 @@ import (
 )
 
 func TestHipchatRetry(t *testing.T) {
-	notifier := new(Notifier)
+	notifier, err := New(
+		&config.HipchatConfig{
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
 	retryCodes := append(test.DefaultRetryCodes(), http.StatusTooManyRequests)
 	for statusCode, expected := range test.RetryTests(retryCodes) {
-		actual, _ := notifier.retry(statusCode)
+		actual, _ := notifier.retrier.Check(statusCode, nil)
 		require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode))
 	}
 }

diff --git a/notify/opsgenie/opsgenie.go b/notify/opsgenie/opsgenie.go
@@ -18,8 +18,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io"
-	"io/ioutil"
 	"net/http"
 	"strings"
 
@@ -37,10 +35,11 @@ import (
 
 // Notifier implements a Notifier for OpsGenie notifications.
 type Notifier struct {
-	conf   *config.OpsGenieConfig
-	tmpl   *template.Template
-	logger log.Logger
-	client *http.Client
+	conf    *config.OpsGenieConfig
+	tmpl    *template.Template
+	logger  log.Logger
+	client  *http.Client
+	retrier *notify.Retrier
 }
 
 // New returns a new OpsGenie notifier.
@@ -49,7 +48,13 @@ func New(c *config.OpsGenieConfig, t *template.Template, l log.Logger) (*Notifie
 	if err != nil {
 		return nil, err
 	}
-	return &Notifier{conf: c, tmpl: t, logger: l, client: client}, nil
+	return &Notifier{
+		conf:    c,
+		tmpl:    t,
+		logger:  l,
+		client:  client,
+		retrier: &notify.Retrier{RetryCodes: []int{http.StatusTooManyRequests}},
+	}, nil
 }
 
 type opsGenieCreateMessage struct {
@@ -88,7 +93,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	}
 	defer notify.Drain(resp)
 
-	return n.retry(resp.StatusCode, resp.Body)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
 }
 
 // Like Split but filter out empty strings.
@@ -191,20 +196,3 @@ func (n *Notifier) createRequest(ctx context.Context, as ...*types.Alert) (*http
 	req.Header.Set("Authorization", fmt.Sprintf("GenieKey %s", apiKey))
 	return req.WithContext(ctx), true, nil
 }
-
-func (n *Notifier) retry(statusCode int, body io.Reader) (bool, error) {
-	if statusCode/100 == 2 {
-		return false, nil
-	}
-
-	err := errors.Errorf("unexpected status code %v", statusCode)
-	if body != nil {
-		if bs, errRead := ioutil.ReadAll(body); errRead == nil {
-			err = errors.Errorf("%s: %s", err, string(bs))
-		}
-	}
-
-	// https://docs.opsgenie.com/docs/response#section-response-codes
-	// Response codes 429 (rate limiting) and 5xx are potentially recoverable
-	return statusCode/100 == 5 || statusCode == 429, err
-}
diff --git a/notify/opsgenie/opsgenie_test.go b/notify/opsgenie/opsgenie_test.go
@@ -14,10 +14,8 @@
 package opsgenie
 
 import (
-	"bytes"
 	"context"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"net/http"
 	"net/url"
@@ -36,45 +34,22 @@ import (
 )
 
 func TestOpsGenieRetry(t *testing.T) {
-	notifier := new(Notifier)
+	notifier, err := New(
+		&config.OpsGenieConfig{
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
 
 	retryCodes := append(test.DefaultRetryCodes(), http.StatusTooManyRequests)
 	for statusCode, expected := range test.RetryTests(retryCodes) {
-		actual, _ := notifier.retry(statusCode, nil)
+		actual, _ := notifier.retrier.Check(statusCode, nil)
 		require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode))
 	}
 }
 
-func TestOpsGenieErr(t *testing.T) {
-	notifier := new(Notifier)
-	for _, tc := range []struct {
-		status   int
-		body     io.Reader
-		expected string
-	}{
-		{
-			status:   http.StatusUnprocessableEntity,
-			body:     nil,
-			expected: "unexpected status code 422",
-		},
-		{
-			status:   http.StatusUnprocessableEntity,
-			body:     bytes.NewBuffer([]byte(`{"message":"Request body is not processable. Please check the errors.","errors":{"priority":"should be one of [ P1, P2, P3, P4, P5 ]"},"took":0.002,"requestId":"865a4f83-99d9-48c8-9550-42a375a3a387"}`)),
-			expected: `unexpected status code 422: {"message":"Request body is not processable. Please check the errors.","errors":{"priority":"should be one of [ P1, P2, P3, P4, P5 ]"},"took":0.002,"requestId":"865a4f83-99d9-48c8-9550-42a375a3a387"}`,
-		},
-		{
-			status:   http.StatusInternalServerError,
-			body:     bytes.NewBuffer([]byte("internal error")),
-			expected: "unexpected status code 500: internal error",
-		},
-	} {
-		t.Run("", func(t *testing.T) {
-			_, err := notifier.retry(tc.status, tc.body)
-			require.Equal(t, err.Error(), tc.expected)
-		})
-	}
-}
-
 func TestOpsGenieRedactedURL(t *testing.T) {
 	ctx, u, fn := test.GetContextWithCancelingURL()
 	defer fn()

diff --git a/notify/pagerduty/pagerduty.go b/notify/pagerduty/pagerduty.go
@@ -35,11 +35,12 @@ import (
 
 // Notifier implements a Notifier for PagerDuty notifications.
 type Notifier struct {
-	conf   *config.PagerdutyConfig
-	tmpl   *template.Template
-	logger log.Logger
-	apiV1  string // for tests.
-	client *http.Client
+	conf    *config.PagerdutyConfig
+	tmpl    *template.Template
+	logger  log.Logger
+	apiV1   string // for tests.
+	client  *http.Client
+	retrier *notify.Retrier
 }
 
 // New returns a new PagerDuty notifier.
@@ -51,6 +52,13 @@ func New(c *config.PagerdutyConfig, t *template.Template, l log.Logger) (*Notifi
 	n := &Notifier{conf: c, tmpl: t, logger: l, client: client}
 	if c.ServiceKey != "" {
 		n.apiV1 = "https://events.pagerduty.com/generic/2010-04-15/create_event.json"
+		// Retrying can solve the issue on 403 (rate limiting) and 5xx response codes.
+		// https://v2.developer.pagerduty.com/docs/trigger-events
+		n.retrier = &notify.Retrier{RetryCodes: []int{http.StatusForbidden}, CustomDetailsFunc: errDetails}
+	} else {
+		// Retrying can solve the issue on 429 (rate limiting) and 5xx response codes.
+		// https://v2.developer.pagerduty.com/docs/events-api-v2#api-response-codes--retry-logic
+		n.retrier = &notify.Retrier{RetryCodes: []int{http.StatusTooManyRequests}, CustomDetailsFunc: errDetails}
 	}
 	return n, nil
 }
@@ -142,7 +150,7 @@ func (n *Notifier) notifyV1(
 	}
 	defer notify.Drain(resp)
 
-	return n.retryV1(resp)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
 }
 
 func (n *Notifier) notifyV2(
@@ -210,7 +218,7 @@ func (n *Notifier) notifyV2(
 	}
 	defer notify.Drain(resp)
 
-	return n.retryV2(resp)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
 }
 
 // Notify implements the Notifier interface.
@@ -248,44 +256,19 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	return n.notifyV2(ctx, eventType, key, data, details, as...)
 }
 
-func pagerDutyErr(status int, body io.Reader) error {
+func errDetails(status int, body io.Reader) string {
 	// See https://v2.developer.pagerduty.com/docs/trigger-events for the v1 events API.
 	// See https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2 for the v2 events API.
-	type pagerDutyResponse struct {
+	if status != http.StatusBadRequest || body == nil {
+		return ""
+	}
+	var pgr struct {
 		Status  string   `json:"status"`
 		Message string   `json:"message"`
 		Errors  []string `json:"errors"`
 	}
-	if status == http.StatusBadRequest && body != nil {
-		var r pagerDutyResponse
-		if err := json.NewDecoder(body).Decode(&r); err == nil {
-			return fmt.Errorf("%s: %s", r.Message, strings.Join(r.Errors, ","))
-		}
-	}
-	return fmt.Errorf("unexpected status code: %v", status)
-}
-
-func (n *Notifier) retryV1(resp *http.Response) (bool, error) {
-	// Retrying can solve the issue on 403 (rate limiting) and 5xx response codes.
-	// 2xx response codes indicate a successful request.
-	// https://v2.developer.pagerduty.com/docs/trigger-events
-	statusCode := resp.StatusCode
-
-	if statusCode/100 != 2 {
-		return (statusCode == http.StatusForbidden || statusCode/100 == 5), pagerDutyErr(statusCode, resp.Body)
+	if err := json.NewDecoder(body).Decode(&pgr); err != nil {
+		return ""
 	}
-	return false, nil
-}
-
-func (n *Notifier) retryV2(resp *http.Response) (bool, error) {
-	// Retrying can solve the issue on 429 (rate limiting) and 5xx response codes.
-	// 2xx response codes indicate a successful request.
-	// https://v2.developer.pagerduty.com/docs/events-api-v2#api-response-codes--retry-logic
-	statusCode := resp.StatusCode
-
-	if statusCode/100 != 2 {
-		return (statusCode == http.StatusTooManyRequests || statusCode/100 == 5), pagerDutyErr(statusCode, resp.Body)
-	}
-
-	return false, nil
+	return fmt.Sprintf("%s: %s", pgr.Message, strings.Join(pgr.Errors, ","))
 }
diff --git a/notify/pagerduty/pagerduty_test.go b/notify/pagerduty/pagerduty_test.go
@@ -29,27 +29,37 @@ import (
 )
 
 func TestPagerDutyRetryV1(t *testing.T) {
-	notifier := new(Notifier)
+	notifier, err := New(
+		&config.PagerdutyConfig{
+			ServiceKey: config.Secret("01234567890123456789012345678901"),
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
 
 	retryCodes := append(test.DefaultRetryCodes(), http.StatusForbidden)
 	for statusCode, expected := range test.RetryTests(retryCodes) {
-		resp := &http.Response{
-			StatusCode: statusCode,
-		}
-		actual, _ := notifier.retryV1(resp)
+		actual, _ := notifier.retrier.Check(statusCode, nil)
 		require.Equal(t, expected, actual, fmt.Sprintf("retryv1 - error on status %d", statusCode))
 	}
 }
 
 func TestPagerDutyRetryV2(t *testing.T) {
-	notifier := new(Notifier)
+	notifier, err := New(
+		&config.PagerdutyConfig{
+			RoutingKey: config.Secret("01234567890123456789012345678901"),
+			HTTPConfig: &commoncfg.HTTPClientConfig{},
+		},
+		test.CreateTmpl(t),
+		log.NewNopLogger(),
+	)
+	require.NoError(t, err)
 
 	retryCodes := append(test.DefaultRetryCodes(), http.StatusTooManyRequests)
 	for statusCode, expected := range test.RetryTests(retryCodes) {
-		resp := &http.Response{
-			StatusCode: statusCode,
-		}
-		actual, _ := notifier.retryV2(resp)
+		actual, _ := notifier.retrier.Check(statusCode, nil)
 		require.Equal(t, expected, actual, fmt.Sprintf("retryv2 - error on status %d", statusCode))
 	}
 }
@@ -92,7 +102,7 @@ func TestPagerDutyRedactedURLV2(t *testing.T) {
 	test.AssertNotifyLeaksNoSecret(t, ctx, notifier, key)
 }
 
-func TestPagerDutyErr(t *testing.T) {
+func TestErrDetails(t *testing.T) {
 	for _, tc := range []struct {
 		status int
 		body   io.Reader
@@ -111,25 +121,23 @@ func TestPagerDutyErr(t *testing.T) {
 			status: http.StatusBadRequest,
 			body:   bytes.NewBuffer([]byte(`{"status"}`)),
 
-			exp: "unexpected status code: 400",
+			exp: "",
 		},
 		{
 			status: http.StatusBadRequest,
-			body:   nil,
 
-			exp: "unexpected status code: 400",
+			exp: "",
 		},
 		{
 			status: http.StatusTooManyRequests,
-			body:   bytes.NewBuffer([]byte("")),
 
-			exp: "unexpected status code: 429",
+			exp: "",
 		},
 	} {
 		tc := tc
 		t.Run("", func(t *testing.T) {
-			err := pagerDutyErr(tc.status, tc.body)
-			require.Contains(t, err.Error(), tc.exp)
+			err := errDetails(tc.status, tc.body)
+			require.Contains(t, err, tc.exp)
 		})
 	}
 }