Revert the retry handling logic

prometheus · May 24, 2023 · 13bda64 · 13bda64
1 parent 55cb0d7
commit 13bda64
Show file tree

Hide file tree

Showing 11 changed files with 22 additions and 56 deletions.
diff --git a/notify/msteams/msteams.go b/notify/msteams/msteams.go
@@ -17,7 +17,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"fmt"
 	"io"
 	"net/http"
 
@@ -129,10 +128,9 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	defer notify.Drain(resp)
 
 	// https://learn.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/connectors-using?tabs=cURL#rate-limiting-for-connectors
-	retry, err := n.retrier.Check(resp.StatusCode, resp.Body)
+	shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
-		reasonErr := notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, fmt.Sprintf("%v", err.Error())), err)
-		return retry, reasonErr
+		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
-	return false, nil
+	return shouldRetry, err
 }
diff --git a/notify/msteams/msteams_test.go b/notify/msteams/msteams_test.go
@@ -137,12 +137,6 @@ func TestNotifier_Notify_WithReason(t *testing.T) {
 		expectedReason  notify.Reason
 		noError         bool
 	}{
-		{
-			name:            "with a 2xx status code and response HTTP error 429",
-			statusCode:      http.StatusOK,
-			responseContent: "Webhook message delivery failed with error: Microsoft Teams endpoint returned HTTP error 429",
-			expectedReason:  notify.ClientErrorReason,
-		},
 		{
 			name:            "with a 2xx status code and response 1",
 			statusCode:      http.StatusOK,

diff --git a/notify/opsgenie/opsgenie.go b/notify/opsgenie/opsgenie.go
@@ -110,7 +110,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 		shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 		notify.Drain(resp)
 		if err != nil {
-			return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+			return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 		}
 	}
 	return true, nil

diff --git a/notify/pagerduty/pagerduty.go b/notify/pagerduty/pagerduty.go
@@ -298,7 +298,7 @@ func (n *Notifier) notifyV2(
 
 	retry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
-		return retry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+		return retry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
 	return retry, err
 }

diff --git a/notify/pushover/pushover.go b/notify/pushover/pushover.go
@@ -166,7 +166,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 
 	shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
-		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
 	return shouldRetry, err
 }
diff --git a/notify/slack/slack.go b/notify/slack/slack.go
@@ -218,7 +218,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	retry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
 		err = errors.Wrap(err, fmt.Sprintf("channel %q", req.Channel))
-		return retry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+		return retry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
 
 	return false, nil

diff --git a/notify/sns/sns.go b/notify/sns/sns.go
@@ -85,7 +85,7 @@ func (n *Notifier) Notify(ctx context.Context, alert ...*types.Alert) (bool, err
 		if e, ok := err.(awserr.RequestFailure); ok {
 			retryable, error := n.retrier.Check(e.StatusCode(), strings.NewReader(e.Message()))
 
-			reasonErr := notify.NewErrorWithReason(notify.GetFailureReason(e.StatusCode(), ""), error)
+			reasonErr := notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(e.StatusCode()), error)
 			return retryable, reasonErr
 		}
 		return true, err

diff --git a/notify/util.go b/notify/util.go
@@ -34,9 +34,6 @@ import (
 // truncationMarker is the character used to represent a truncation.
 const truncationMarker = "…"
 
-// Retry messages
-var RetryMsgs = []string{"Microsoft Teams endpoint returned HTTP error 429"}
-
 // UserAgentHeader is the default User-Agent for notification requests
 var UserAgentHeader = fmt.Sprintf("Alertmanager/%s", version.Version)
 
@@ -216,39 +213,17 @@ type Retrier struct {
 	RetryCodes []int
 }
 
-// Check whether a given string contains one item in pattern list.
-func isMatched(patterns []string, msg string) bool {
-	matched := false
-	for _, pattern := range patterns {
-		if strings.Contains(msg, pattern) {
-			matched = true
-			break
-		}
-	}
-	return matched
-}
-
 // Check returns a boolean indicating whether the request should be retried
 // and an optional error if the request has failed. If body is not nil, it will
 // be included in the error message.
 func (r *Retrier) Check(statusCode int, body io.Reader) (bool, error) {
-	var details string
-	if r.CustomDetailsFunc != nil {
-		details = r.CustomDetailsFunc(statusCode, body)
-	} else {
-		details = readAll(body)
-	}
-
-	retry := isMatched(RetryMsgs, details)
-
 	// 2xx responses are considered to be always successful.
-	// except that response content contains retry message.
-	if !retry && statusCode/100 == 2 {
+	if statusCode/100 == 2 {
 		return false, nil
 	}
 
 	// 5xx responses are considered to be always retried.
-	retry = statusCode/100 == 5
+	retry := statusCode/100 == 5
 	if !retry {
 		for _, code := range r.RetryCodes {
 			if code == statusCode {
@@ -259,12 +234,14 @@ func (r *Retrier) Check(statusCode int, body io.Reader) (bool, error) {
 	}
 
 	s := fmt.Sprintf("unexpected status code %v", statusCode)
+	var details string
+	if r.CustomDetailsFunc != nil {
+		details = r.CustomDetailsFunc(statusCode, body)
+	} else {
+		details = readAll(body)
+	}
 	if details != "" {
-		if statusCode/100 != 2 {
-			s = fmt.Sprintf("%s: %s", s, details)
-		} else {
-			s = details
-		}
+		s = fmt.Sprintf("%s: %s", s, details)
 	}
 	return retry, errors.New(s)
 }
@@ -311,11 +288,8 @@ func (s Reason) String() string {
 // possibleFailureReasonCategory is a list of possible failure reason.
 var possibleFailureReasonCategory = []string{DefaultReason.String(), ClientErrorReason.String(), ServerErrorReason.String()}
 
-// GetFailureReason returns the reason for the failure based on the status code and response content provided.
-func GetFailureReason(statusCode int, responseContent string) Reason {
-	if len(responseContent) > 0 && statusCode/100 == 2 && isMatched(RetryMsgs, responseContent) {
-		return ClientErrorReason
-	}
+// GetFailureReasonFromStatusCode returns the reason for the failure based on the status code provided.
+func GetFailureReasonFromStatusCode(statusCode int) Reason {
 	if statusCode/100 == 4 {
 		return ClientErrorReason
 	}

diff --git a/notify/victorops/victorops.go b/notify/victorops/victorops.go
@@ -106,7 +106,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 
 	shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
-		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
 	return shouldRetry, err
 }

diff --git a/notify/webhook/webhook.go b/notify/webhook/webhook.go
@@ -122,7 +122,7 @@ func (n *Notifier) Notify(ctx context.Context, alerts ...*types.Alert) (bool, er
 
 	shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body)
 	if err != nil {
-		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), err)
+		return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
 	}
 	return shouldRetry, err
 }

diff --git a/notify/wechat/wechat.go b/notify/wechat/wechat.go
@@ -168,7 +168,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	defer notify.Drain(resp)
 
 	if resp.StatusCode != 200 {
-		return true, notify.NewErrorWithReason(notify.GetFailureReason(resp.StatusCode, ""), fmt.Errorf("unexpected status code %v", resp.StatusCode))
+		return true, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), fmt.Errorf("unexpected status code %v", resp.StatusCode))
 	}
 
 	body, err := io.ReadAll(resp.Body)