Log Response Details for Notifier errors for Webhooks, Pushover and VictorOps (#3103)

* Pass in response body to Retrier Check
* Custom error details for webhook notifier

Signed-off-by: Oktarian Tilney-Bassett <oktariantilneybassett@improbable.io>

notify/pushover/pushover.go

@@ -130,5 +130,5 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	}
 	defer notify.Drain(resp)
 
-	return n.retrier.Check(resp.StatusCode, nil)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
 }

notify/victorops/victorops.go

@@ -86,7 +86,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
 	}
 	defer notify.Drain(resp)
 
-	return n.retrier.Check(resp.StatusCode, nil)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
 }
 
 // Create the JSON payload to be sent to the VictorOps API.

notify/webhook/webhook.go

@@ -17,6 +17,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"net/http"
 
@@ -53,8 +54,8 @@ func New(conf *config.WebhookConfig, t *template.Template, l log.Logger, httpOpt
 		// Webhooks are assumed to respond with 2xx response codes on a successful
 		// request and 5xx response codes are assumed to be recoverable.
 		retrier: &notify.Retrier{
-			CustomDetailsFunc: func(int, io.Reader) string {
-				return conf.URL.String()
+			CustomDetailsFunc: func(_ int, body io.Reader) string {
+				return errDetails(body, conf.URL.String())
 			},
 		},
 	}, nil
@@ -104,7 +105,18 @@ func (n *Notifier) Notify(ctx context.Context, alerts ...*types.Alert) (bool, er
 	if err != nil {
 		return true, err
 	}
-	notify.Drain(resp)
+	defer notify.Drain(resp)
 
-	return n.retrier.Check(resp.StatusCode, nil)
+	return n.retrier.Check(resp.StatusCode, resp.Body)
+}
+
+func errDetails(body io.Reader, url string) string {
+	if body == nil {
+		return url
+	}
+	bs, err := io.ReadAll(body)
+	if err != nil {
+		return url
+	}
+	return fmt.Sprintf("%s: %s", url, string(bs))
 }

notify/webhook/webhook_test.go

@@ -14,7 +14,10 @@
 package webhook
 
 import (
+	"bytes"
 	"fmt"
+	"io"
+	"net/http"
 	"net/url"
 	"testing"
 
@@ -43,10 +46,41 @@ func TestWebhookRetry(t *testing.T) {
 	if err != nil {
 		require.NoError(t, err)
 	}
-	for statusCode, expected := range test.RetryTests(test.DefaultRetryCodes()) {
-		actual, _ := notifier.retrier.Check(statusCode, nil)
-		require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode))
-	}
+
+	t.Run("test retry status code", func(t *testing.T) {
+		for statusCode, expected := range test.RetryTests(test.DefaultRetryCodes()) {
+			actual, _ := notifier.retrier.Check(statusCode, nil)
+			require.Equal(t, expected, actual, fmt.Sprintf("error on status %d", statusCode))
+		}
+	})
+
+	t.Run("test retry error details", func(t *testing.T) {
+		for _, tc := range []struct {
+			status int
+			body   io.Reader
+
+			exp string
+		}{
+			{
+				status: http.StatusBadRequest,
+				body: bytes.NewBuffer([]byte(
+					`{"status":"invalid event"}`,
+				)),
+
+				exp: fmt.Sprintf(`unexpected status code %d: %s: {"status":"invalid event"}`, http.StatusBadRequest, u.String()),
+			},
+			{
+				status: http.StatusBadRequest,
+
+				exp: fmt.Sprintf(`unexpected status code %d: %s`, http.StatusBadRequest, u.String()),
+			},
+		} {
+			t.Run("", func(t *testing.T) {
+				_, err = notifier.retrier.Check(tc.status, tc.body)
+				require.Equal(t, tc.exp, err.Error())
+			})
+		}
+	})
 }
 
 func TestWebhookTruncateAlerts(t *testing.T) {