Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reason code to slack notifier #3252

Merged
merged 3 commits into from
Apr 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions notify/slack/slack.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"os"
"strings"
Expand All @@ -43,6 +44,8 @@ type Notifier struct {
logger log.Logger
client *http.Client
retrier *notify.Retrier

postJSONFunc func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error)
}

// New returns a new Slack notification handler.
Expand All @@ -53,11 +56,12 @@ func New(c *config.SlackConfig, t *template.Template, l log.Logger, httpOpts ...
}

return &Notifier{
conf: c,
tmpl: t,
logger: l,
client: client,
retrier: &notify.Retrier{},
conf: c,
tmpl: t,
logger: l,
client: client,
retrier: &notify.Retrier{},
postJSONFunc: notify.PostJSON,
}, nil
}

Expand Down Expand Up @@ -202,7 +206,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
u = strings.TrimSpace(string(content))
}

resp, err := notify.PostJSON(ctx, n.client, u, &buf)
resp, err := n.postJSONFunc(ctx, n.client, u, &buf)
if err != nil {
return true, notify.RedactURL(err)
}
Expand All @@ -213,5 +217,7 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error)
// https://api.slack.com/changelog/2016-05-17-changes-to-errors-for-incoming-webhooks
retry, err := n.retrier.Check(resp.StatusCode, resp.Body)
err = errors.Wrap(err, fmt.Sprintf("channel %q", req.Channel))
return retry, err
reasonErr := notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err)
Copy link
Contributor

@knyar knyar Apr 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not this make Notify return a non-nil error even when err is nil here?

UPD: fixing as part of #3121


return retry, reasonErr
}
67 changes: 67 additions & 0 deletions notify/slack/slack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,25 @@
package slack

import (
"context"
"fmt"
"io"
"net/http"
"net/http/httptest"
"net/url"
"os"
"testing"
"time"

"github.com/go-kit/log"
commoncfg "github.com/prometheus/common/config"
"github.com/prometheus/common/model"
"github.com/stretchr/testify/require"

"github.com/prometheus/alertmanager/config"
"github.com/prometheus/alertmanager/notify"
"github.com/prometheus/alertmanager/notify/test"
"github.com/prometheus/alertmanager/types"
)

func TestSlackRetry(t *testing.T) {
Expand Down Expand Up @@ -102,3 +111,61 @@ func TestTrimmingSlackURLFromFile(t *testing.T) {

test.AssertNotifyLeaksNoSecret(ctx, t, notifier, u.String())
}

func TestNotifier_Notify_WithReason(t *testing.T) {
tests := []struct {
name string
statusCode int
expectedReason notify.Reason
}{
{
name: "with a 4xx status code",
statusCode: http.StatusUnauthorized,
expectedReason: notify.ClientErrorReason,
},
{
name: "with a 5xx status code",
statusCode: http.StatusInternalServerError,
expectedReason: notify.ServerErrorReason,
},
{
name: "with any other status code",
statusCode: http.StatusTemporaryRedirect,
expectedReason: notify.DefaultReason,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
apiurl, _ := url.Parse("https://slack.com/post.Message")
notifier, err := New(
&config.SlackConfig{
NotifierConfig: config.NotifierConfig{},
HTTPConfig: &commoncfg.HTTPClientConfig{},
APIURL: &config.SecretURL{URL: apiurl},
},
test.CreateTmpl(t),
log.NewNopLogger(),
)
require.NoError(t, err)

notifier.postJSONFunc = func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error) {
resp := httptest.NewRecorder()
resp.WriteHeader(tt.statusCode)
return resp.Result(), nil
}
ctx := context.Background()
ctx = notify.WithGroupKey(ctx, "1")

alert1 := &types.Alert{
Alert: model.Alert{
StartsAt: time.Now(),
EndsAt: time.Now().Add(time.Hour),
},
}
_, err = notifier.Notify(ctx, alert1)
reasonError, ok := err.(*notify.ErrorWithReason)
require.True(t, ok)
require.Equal(t, tt.expectedReason, reasonError.Reason)
})
}
}