-
Notifications
You must be signed in to change notification settings - Fork 43
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
Replace notify.Factory with a build function that accepts typed configuration #63
Changes from 9 commits
2a18a67
c9f0b98
d5c03e8
8144826
cd4a081
809ab30
94e6d30
0c24bd1
dbee25f
9cc9790
4db293d
ddaf024
f6f6b7a
ff741c1
f6d5712
8ea2fed
7657484
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,10 +1,14 @@ | ||||||
package notify | ||||||
|
||||||
import ( | ||||||
"strings" | ||||||
"fmt" | ||||||
|
||||||
"github.com/prometheus/alertmanager/notify" | ||||||
"github.com/prometheus/alertmanager/template" | ||||||
"github.com/prometheus/alertmanager/types" | ||||||
|
||||||
"github.com/grafana/alerting/images" | ||||||
"github.com/grafana/alerting/logging" | ||||||
"github.com/grafana/alerting/receivers" | ||||||
"github.com/grafana/alerting/receivers/alertmanager" | ||||||
"github.com/grafana/alerting/receivers/dinding" | ||||||
|
@@ -27,57 +31,146 @@ import ( | |||||
"github.com/grafana/alerting/receivers/wecom" | ||||||
) | ||||||
|
||||||
var receiverFactories = map[string]func(receivers.FactoryConfig) (NotificationChannel, error){ | ||||||
"prometheus-alertmanager": wrap(alertmanager.New), | ||||||
"dingding": wrap(dinding.New), | ||||||
"discord": wrap(discord.New), | ||||||
"email": wrap(email.New), | ||||||
"googlechat": wrap(googlechat.New), | ||||||
"kafka": wrap(kafka.New), | ||||||
"line": wrap(line.New), | ||||||
"opsgenie": wrap(opsgenie.New), | ||||||
"pagerduty": wrap(pagerduty.New), | ||||||
"pushover": wrap(pushover.New), | ||||||
"sensugo": wrap(sensugo.New), | ||||||
"slack": wrap(slack.New), | ||||||
"teams": wrap(teams.New), | ||||||
"telegram": wrap(telegram.New), | ||||||
"threema": wrap(threema.New), | ||||||
"victorops": wrap(victorops.New), | ||||||
"webhook": wrap(webhook.New), | ||||||
"wecom": wrap(wecom.New), | ||||||
"webex": wrap(webex.New), | ||||||
} | ||||||
|
||||||
type NotificationChannel interface { | ||||||
notify.Notifier | ||||||
notify.ResolvedSender | ||||||
} | ||||||
// BuildReceiverIntegrations builds notifiers of the receiver and wraps each of them in Integration. | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
func BuildReceiverIntegrations( | ||||||
receiver GrafanaReceiverConfig, | ||||||
tmpl *template.Template, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
you need the local types otherwise you'll import the alertmanage directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will address it in a follow-up PR as it touches many lines. |
||||||
img images.ImageStore, // Used by some receivers to include as part of the source | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
newLogger logging.LoggerFactory, | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
newWebhookSender func(n receivers.Metadata) (receivers.WebhookSender, error), | ||||||
newEmailSender func(n receivers.Metadata) (receivers.EmailSender, error), | ||||||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these functions should use primitives, such as: func(ctx context.Context, client *http.Client, url string, body io.Reader) (*http.Response, error) Instead of whatever interface was defined by Grafana with the |
||||||
orgID int64, | ||||||
version string, | ||||||
) ([]*Integration, error) { | ||||||
|
||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
func Factory(receiverType string) (func(receivers.FactoryConfig) (NotificationChannel, error), bool) { | ||||||
receiverType = strings.ToLower(receiverType) | ||||||
factory, exists := receiverFactories[receiverType] | ||||||
return factory, exists | ||||||
} | ||||||
type notificationChannel interface { | ||||||
notify.Notifier | ||||||
notify.ResolvedSender | ||||||
} | ||||||
|
||||||
// wrap wraps the notifier's factory errors with receivers.ReceiverValidationError | ||||||
func wrap[T NotificationChannel](f func(fc receivers.FactoryConfig) (T, error)) func(receivers.FactoryConfig) (NotificationChannel, error) { | ||||||
return func(fc receivers.FactoryConfig) (NotificationChannel, error) { | ||||||
ch, err := f(fc) | ||||||
if err != nil { | ||||||
return nil, ReceiverValidationError{ | ||||||
Err: err, | ||||||
// TODO it will be removed in the next PR | ||||||
Cfg: &GrafanaReceiver{ | ||||||
UID: fc.Config.UID, | ||||||
Name: fc.Config.Name, | ||||||
Type: fc.Config.Type, | ||||||
DisableResolveMessage: fc.Config.DisableResolveMessage, | ||||||
Settings: fc.Config.Settings, | ||||||
SecureSettings: nil, | ||||||
}, | ||||||
var ( | ||||||
integrations []*Integration | ||||||
errors types.MultiError | ||||||
createIntegration = func(idx int, cfg receivers.Metadata, f func(logger logging.Logger) notificationChannel) { | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
logger := newLogger("ngalert.notifier."+cfg.Type, "notifierUID", cfg.UID) | ||||||
n := f(logger) | ||||||
i := NewIntegration(n, n, cfg.Type, idx) | ||||||
integrations = append(integrations, i) | ||||||
} | ||||||
createIntegrationWithWebhook = func(idx int, cfg receivers.Metadata, f func(logger logging.Logger, w receivers.WebhookSender) notificationChannel) { | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
w, e := newWebhookSender(cfg) | ||||||
if e != nil { | ||||||
errors.Add(fmt.Errorf("unable to build webhook client for %s notifier %s (UID: %s): %w ", cfg.Type, cfg.Name, cfg.UID, e)) | ||||||
return | ||||||
} | ||||||
createIntegration(idx, cfg, func(logger logging.Logger) notificationChannel { | ||||||
return f(logger, w) | ||||||
}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these to anonymous methods help reduce duplicated code below. |
||||||
} | ||||||
) | ||||||
|
||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
for i, cfg := range receiver.AlertmanagerConfigs { | ||||||
createIntegration(i, cfg.Metadata, func(l logging.Logger) notificationChannel { | ||||||
return alertmanager.New(cfg.Settings, cfg.Metadata, img, l) | ||||||
}) | ||||||
yuri-tceretian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
for i, cfg := range receiver.DingdingConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return dinding.New(cfg.Settings, cfg.Metadata, tmpl, w, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.DiscordConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return discord.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.EmailConfigs { | ||||||
mailCli, e := newEmailSender(cfg.Metadata) | ||||||
if e != nil { | ||||||
errors.Add(fmt.Errorf("unable to build email client for %s notifier %s (UID: %s): %w ", cfg.Type, cfg.Name, cfg.UID, e)) | ||||||
} | ||||||
return ch, nil | ||||||
createIntegration(i, cfg.Metadata, func(l logging.Logger) notificationChannel { | ||||||
return email.New(cfg.Settings, cfg.Metadata, tmpl, mailCli, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.GooglechatConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return googlechat.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.KafkaConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return kafka.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.LineConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return line.New(cfg.Settings, cfg.Metadata, tmpl, w, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.OpsgenieConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return opsgenie.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.PagerdutyConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return pagerduty.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.PushoverConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return pushover.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.SensugoConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return sensugo.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.SlackConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return slack.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.TeamsConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return teams.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.TelegramConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return telegram.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.ThreemaConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return threema.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.VictoropsConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return victorops.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, version) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.WebhookConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return webhook.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, orgID) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.WecomConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return wecom.New(cfg.Settings, cfg.Metadata, tmpl, w, l) | ||||||
}) | ||||||
} | ||||||
for i, cfg := range receiver.WebexConfigs { | ||||||
createIntegrationWithWebhook(i, cfg.Metadata, func(l logging.Logger, w receivers.WebhookSender) notificationChannel { | ||||||
return webex.New(cfg.Settings, cfg.Metadata, tmpl, w, img, l, orgID) | ||||||
}) | ||||||
} | ||||||
|
||||||
if errors.Len() > 0 { | ||||||
return nil, &errors | ||||||
} | ||||||
return integrations, nil | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
package notify | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"math/rand" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/grafana/alerting/images" | ||
"github.com/grafana/alerting/logging" | ||
"github.com/grafana/alerting/receivers" | ||
"github.com/grafana/alerting/templates" | ||
) | ||
|
||
func TestBuildReceiverIntegrations(t *testing.T) { | ||
var orgID = rand.Int63() | ||
var version = fmt.Sprintf("Grafana v%d", rand.Uint32()) | ||
imageStore := &images.FakeImageStore{} | ||
tmpl := templates.ForTests(t) | ||
|
||
webhookFactory := func(n receivers.Metadata) (receivers.WebhookSender, error) { | ||
return receivers.MockNotificationService(), nil | ||
} | ||
emailFactory := func(n receivers.Metadata) (receivers.EmailSender, error) { | ||
return receivers.MockNotificationService(), nil | ||
} | ||
loggerFactory := func(_ string, _ ...interface{}) logging.Logger { | ||
return &logging.FakeLogger{} | ||
} | ||
|
||
getFullConfig := func(t *testing.T) (GrafanaReceiverConfig, int) { | ||
recCfg := &APIReceiver{ConfigReceiver: ConfigReceiver{Name: "test-receiver"}} | ||
for notifierType, cfg := range allKnownConfigs { | ||
recCfg.Receivers = append(recCfg.Receivers, cfg.getRawNotifierConfig(notifierType)) | ||
} | ||
parsed, err := BuildReceiverConfiguration(context.Background(), recCfg, GetDecryptedValueFnForTesting) | ||
require.NoError(t, err) | ||
return parsed, len(recCfg.Receivers) | ||
} | ||
|
||
t.Run("should build all supported notifiers", func(t *testing.T) { | ||
fullCfg, qty := getFullConfig(t) | ||
|
||
loggerNames := make(map[string]struct{}, qty) | ||
logger := func(name string, _ ...interface{}) logging.Logger { | ||
loggerNames[name] = struct{}{} | ||
return &logging.FakeLogger{} | ||
} | ||
|
||
webhooks := make(map[receivers.Metadata]struct{}, qty) | ||
wh := func(n receivers.Metadata) (receivers.WebhookSender, error) { | ||
webhooks[n] = struct{}{} | ||
return webhookFactory(n) | ||
} | ||
|
||
emails := make(map[receivers.Metadata]struct{}, qty) | ||
em := func(n receivers.Metadata) (receivers.EmailSender, error) { | ||
emails[n] = struct{}{} | ||
return emailFactory(n) | ||
} | ||
|
||
integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, logger, wh, em, orgID, version) | ||
|
||
require.NoError(t, err) | ||
require.Len(t, integrations, qty) | ||
|
||
t.Run("should call logger factory for each config", func(t *testing.T) { | ||
require.Len(t, loggerNames, qty) | ||
}) | ||
t.Run("should call webhook factory for each config that needs it", func(t *testing.T) { | ||
require.Len(t, webhooks, 17) // we have 17 notifiers that support webhook | ||
}) | ||
t.Run("should call email factory for each config that needs it", func(t *testing.T) { | ||
require.Len(t, emails, 1) // we have only email notifier that needs sender | ||
}) | ||
}) | ||
t.Run("should return errors if webhook factory fails", func(t *testing.T) { | ||
fullCfg, _ := getFullConfig(t) | ||
calls := 0 | ||
failingFactory := func(n receivers.Metadata) (receivers.WebhookSender, error) { | ||
calls++ | ||
return nil, errors.New("bad-test") | ||
} | ||
|
||
integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, loggerFactory, failingFactory, emailFactory, orgID, version) | ||
|
||
require.Empty(t, integrations) | ||
require.NotNil(t, err) | ||
require.ErrorContains(t, err, "bad-test") | ||
require.Greater(t, calls, 0) | ||
}) | ||
t.Run("should return errors if email factory fails", func(t *testing.T) { | ||
fullCfg, _ := getFullConfig(t) | ||
calls := 0 | ||
failingFactory := func(n receivers.Metadata) (receivers.EmailSender, error) { | ||
calls++ | ||
return nil, errors.New("bad-test") | ||
} | ||
|
||
integrations, err := BuildReceiverIntegrations(fullCfg, tmpl, imageStore, loggerFactory, webhookFactory, failingFactory, orgID, version) | ||
|
||
require.Empty(t, integrations) | ||
require.NotNil(t, err) | ||
require.ErrorContains(t, err, "bad-test") | ||
require.Greater(t, calls, 0) | ||
}) | ||
t.Run("should not produce any integration if config is empty", func(t *testing.T) { | ||
cfg := GrafanaReceiverConfig{Name: "test"} | ||
|
||
integrations, err := BuildReceiverIntegrations(cfg, tmpl, imageStore, loggerFactory, webhookFactory, emailFactory, orgID, version) | ||
|
||
require.NoError(t, err) | ||
require.Empty(t, integrations) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -347,8 +347,12 @@ type NotifierConfig[T interface{}] struct { | |
Settings T | ||
} | ||
|
||
// GetDecryptedValueFn is a function that returns the decrypted value of | ||
// the given key. If the key is not present, then it returns the fallback value. | ||
type GetDecryptedValueFn func(ctx context.Context, sjd map[string][]byte, key string, fallback string) string | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is moved from the receivers package because it is not used there anymore. |
||
// BuildReceiverConfiguration parses, decrypts and validates the APIReceiver. | ||
func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt receivers.GetDecryptedValueFn) (GrafanaReceiverConfig, error) { | ||
func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt GetDecryptedValueFn) (GrafanaReceiverConfig, error) { | ||
result := GrafanaReceiverConfig{ | ||
Name: api.Name, | ||
} | ||
|
@@ -365,7 +369,7 @@ func BuildReceiverConfiguration(ctx context.Context, api *APIReceiver, decrypt r | |
} | ||
|
||
// parseNotifier parses receivers and populates the corresponding field in GrafanaReceiverConfig. Returns an error if the configuration cannot be parsed. | ||
func parseNotifier(ctx context.Context, result *GrafanaReceiverConfig, receiver *GrafanaReceiver, decrypt receivers.GetDecryptedValueFn) error { | ||
func parseNotifier(ctx context.Context, result *GrafanaReceiverConfig, receiver *GrafanaReceiver, decrypt GetDecryptedValueFn) error { | ||
secureSettings, err := decodeSecretsFromBase64(receiver.SecureSettings) | ||
if err != nil { | ||
return err | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes it clear that first element is always logger name whereas the remaining go to context (that's how it is implemented in Grafana)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change, but wouldn't it mean that we'll be using a different interface than the one defined in Grafana's
infra
package? In order to use the logger, we would have to add this string as the first item in thectx
slice.I don't think it's a big deal, just want to make sure we're aware of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regularly, in the logging context is a slice of tuples. go-kit adds {Missing} if slice size is odd. However, In Grafana logger assumes that the first element is not a tuple's label but a logger name and automatically adds the
logger
label. However, this package can (and will) use a different logger implementation but usages of this interface still assume that the first element of the context is a logger. This change makes this interface more explicit.There is no interface for
New
method in infra. What I do here is basically define it.https://github.com/grafana/grafana/blob/862a6a2fa625100b690c5a48a9b12e7a7dd42041/pkg/infra/log/log.go#L231-L238