From 964adae477eb633c7bb4df92f36382cbb56a0539 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Fri, 1 Jul 2022 09:18:21 +0100 Subject: [PATCH] Alerting: Remove withStoredImage and change forEachFunc (#51384) (#51641) (cherry picked from commit 7fea330dc1ae3cfdbc67c5e7611b42fdcf6d5c7f) Co-authored-by: George Robinson --- .../ngalert/notifier/channels/alertmanager.go | 4 +- .../ngalert/notifier/channels/discord.go | 6 +- .../ngalert/notifier/channels/email.go | 22 +++-- .../ngalert/notifier/channels/googlechat.go | 4 +- .../ngalert/notifier/channels/kafka.go | 4 +- .../ngalert/notifier/channels/opsgenie.go | 4 +- .../ngalert/notifier/channels/pagerduty.go | 4 +- .../ngalert/notifier/channels/pushover.go | 61 +++++++------- .../ngalert/notifier/channels/sensugo.go | 10 +-- .../ngalert/notifier/channels/slack.go | 13 +-- .../ngalert/notifier/channels/teams.go | 4 +- .../ngalert/notifier/channels/telegram.go | 46 +++++------ .../ngalert/notifier/channels/threema.go | 4 +- .../ngalert/notifier/channels/util.go | 82 +++++++++---------- .../ngalert/notifier/channels/util_test.go | 4 +- .../ngalert/notifier/channels/victorops.go | 4 +- .../ngalert/notifier/channels/webhook.go | 4 +- 17 files changed, 127 insertions(+), 153 deletions(-) diff --git a/pkg/services/ngalert/notifier/channels/alertmanager.go b/pkg/services/ngalert/notifier/channels/alertmanager.go index b7405dafb5d..9357b14a578 100644 --- a/pkg/services/ngalert/notifier/channels/alertmanager.go +++ b/pkg/services/ngalert/notifier/channels/alertmanager.go @@ -102,10 +102,10 @@ func (n *AlertmanagerNotifier) Notify(ctx context.Context, as ...*types.Alert) ( } _ = withStoredImages(ctx, n.logger, n.images, - func(index int, image *ngmodels.Image) error { + func(index int, image ngmodels.Image) error { // If there is an image for this alert and the image has been uploaded // to a public URL then include it as an annotation - if image != nil && image.URL != "" { + if image.URL != "" { as[index].Annotations["image"] = model.LabelValue(image.URL) } return nil diff --git a/pkg/services/ngalert/notifier/channels/discord.go b/pkg/services/ngalert/notifier/channels/discord.go index 48fac488c71..a6296b5a96f 100644 --- a/pkg/services/ngalert/notifier/channels/discord.go +++ b/pkg/services/ngalert/notifier/channels/discord.go @@ -199,15 +199,11 @@ func (d DiscordNotifier) constructAttachments(ctx context.Context, as []*types.A attachments := make([]discordAttachment, 0) _ = withStoredImages(ctx, d.log, d.images, - func(index int, image *ngmodels.Image) error { + func(index int, image ngmodels.Image) error { if embedQuota < 1 { return ErrImagesDone } - if image == nil { - return nil - } - if len(image.URL) > 0 { attachments = append(attachments, discordAttachment{ url: image.URL, diff --git a/pkg/services/ngalert/notifier/channels/email.go b/pkg/services/ngalert/notifier/channels/email.go index ebf7dac61b8..b33ab8dff6b 100644 --- a/pkg/services/ngalert/notifier/channels/email.go +++ b/pkg/services/ngalert/notifier/channels/email.go @@ -110,18 +110,16 @@ func (en *EmailNotifier) Notify(ctx context.Context, alerts ...*types.Alert) (bo // Extend alerts data with images, if available. var embeddedFiles []string _ = withStoredImages(ctx, en.log, en.images, - func(index int, image *ngmodels.Image) error { - if image != nil { - if len(image.URL) != 0 { - data.Alerts[index].ImageURL = image.URL - } else if len(image.Path) != 0 { - _, err := os.Stat(image.Path) - if err == nil { - data.Alerts[index].EmbeddedImage = path.Base(image.Path) - embeddedFiles = append(embeddedFiles, image.Path) - } else { - en.log.Warn("failed to get image file for email attachment", "file", image.Path, "err", err) - } + func(index int, image ngmodels.Image) error { + if len(image.URL) != 0 { + data.Alerts[index].ImageURL = image.URL + } else if len(image.Path) != 0 { + _, err := os.Stat(image.Path) + if err == nil { + data.Alerts[index].EmbeddedImage = path.Base(image.Path) + embeddedFiles = append(embeddedFiles, image.Path) + } else { + en.log.Warn("failed to get image file for email attachment", "file", image.Path, "err", err) } } return nil diff --git a/pkg/services/ngalert/notifier/channels/googlechat.go b/pkg/services/ngalert/notifier/channels/googlechat.go index 2e53e51a7fe..cd9dc30c359 100644 --- a/pkg/services/ngalert/notifier/channels/googlechat.go +++ b/pkg/services/ngalert/notifier/channels/googlechat.go @@ -191,8 +191,8 @@ func (gcn *GoogleChatNotifier) buildScreenshotCard(ctx context.Context, alerts [ } _ = withStoredImages(ctx, gcn.log, gcn.images, - func(index int, image *ngmodels.Image) error { - if image == nil || len(image.URL) == 0 { + func(index int, image ngmodels.Image) error { + if len(image.URL) == 0 { return nil } diff --git a/pkg/services/ngalert/notifier/channels/kafka.go b/pkg/services/ngalert/notifier/channels/kafka.go index b812fe2cc49..e97c0173662 100644 --- a/pkg/services/ngalert/notifier/channels/kafka.go +++ b/pkg/services/ngalert/notifier/channels/kafka.go @@ -107,8 +107,8 @@ func (kn *KafkaNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool, var contexts []interface{} _ = withStoredImages(ctx, kn.log, kn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && image.URL != "" { + func(_ int, image ngmodels.Image) error { + if image.URL != "" { imageJSON := simplejson.New() imageJSON.Set("type", "image") imageJSON.Set("src", image.URL) diff --git a/pkg/services/ngalert/notifier/channels/opsgenie.go b/pkg/services/ngalert/notifier/channels/opsgenie.go index b0737161b14..1e55b66b158 100644 --- a/pkg/services/ngalert/notifier/channels/opsgenie.go +++ b/pkg/services/ngalert/notifier/channels/opsgenie.go @@ -242,8 +242,8 @@ func (on *OpsgenieNotifier) buildOpsgenieMessage(ctx context.Context, alerts mod images := []string{} _ = withStoredImages(ctx, on.log, on.images, - func(index int, image *ngmodels.Image) error { - if image == nil || len(image.URL) == 0 { + func(_ int, image ngmodels.Image) error { + if len(image.URL) == 0 { return nil } images = append(images, image.URL) diff --git a/pkg/services/ngalert/notifier/channels/pagerduty.go b/pkg/services/ngalert/notifier/channels/pagerduty.go index ec50c3f37ff..5e9491409e5 100644 --- a/pkg/services/ngalert/notifier/channels/pagerduty.go +++ b/pkg/services/ngalert/notifier/channels/pagerduty.go @@ -189,8 +189,8 @@ func (pn *PagerdutyNotifier) buildPagerdutyMessage(ctx context.Context, alerts m } _ = withStoredImages(ctx, pn.log, pn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && len(image.URL) != 0 { + func(_ int, image ngmodels.Image) error { + if len(image.URL) != 0 { msg.Images = append(msg.Images, pagerDutyImage{Src: image.URL}) } diff --git a/pkg/services/ngalert/notifier/channels/pushover.go b/pkg/services/ngalert/notifier/channels/pushover.go index 0f6fb170ec4..43c107e8216 100644 --- a/pkg/services/ngalert/notifier/channels/pushover.go +++ b/pkg/services/ngalert/notifier/channels/pushover.go @@ -233,39 +233,36 @@ func (pn *PushoverNotifier) genPushoverBody(ctx context.Context, as ...*types.Al // Pushover supports at most one image attachment with a maximum size of pushoverMaxFileSize. // If the image is larger than pushoverMaxFileSize then return an error. - _ = withStoredImages(ctx, pn.log, pn.images, func(index int, image *ngmodels.Image) error { - if image != nil { - f, err := os.Open(image.Path) - if err != nil { - return fmt.Errorf("failed to open the image: %w", err) - } - defer func() { - if err := f.Close(); err != nil { - pn.log.Error("failed to close the image", "file", image.Path) - } - }() - - fileInfo, err := f.Stat() - if err != nil { - return fmt.Errorf("failed to stat the image: %w", err) - } - - if fileInfo.Size() > pushoverMaxFileSize { - return fmt.Errorf("image would exceeded maximum file size: %d", fileInfo.Size()) - } - - fw, err := w.CreateFormFile("attachment", image.Path) - if err != nil { - return fmt.Errorf("failed to create form file for the image: %w", err) - } - - if _, err = io.Copy(fw, f); err != nil { - return fmt.Errorf("failed to copy the image to the form file: %w", err) - } - - return ErrImagesDone + _ = withStoredImages(ctx, pn.log, pn.images, func(index int, image ngmodels.Image) error { + f, err := os.Open(image.Path) + if err != nil { + return fmt.Errorf("failed to open the image: %w", err) } - return nil + defer func() { + if err := f.Close(); err != nil { + pn.log.Error("failed to close the image", "file", image.Path) + } + }() + + fileInfo, err := f.Stat() + if err != nil { + return fmt.Errorf("failed to stat the image: %w", err) + } + + if fileInfo.Size() > pushoverMaxFileSize { + return fmt.Errorf("image would exceeded maximum file size: %d", fileInfo.Size()) + } + + fw, err := w.CreateFormFile("attachment", image.Path) + if err != nil { + return fmt.Errorf("failed to create form file for the image: %w", err) + } + + if _, err = io.Copy(fw, f); err != nil { + return fmt.Errorf("failed to copy the image to the form file: %w", err) + } + + return ErrImagesDone }, as...) var sound string diff --git a/pkg/services/ngalert/notifier/channels/sensugo.go b/pkg/services/ngalert/notifier/channels/sensugo.go index 35726d3ab8c..2407e340c3e 100644 --- a/pkg/services/ngalert/notifier/channels/sensugo.go +++ b/pkg/services/ngalert/notifier/channels/sensugo.go @@ -139,21 +139,17 @@ func (sn *SensuGoNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool labels := make(map[string]string) - var imageURL string _ = withStoredImages(ctx, sn.log, sn.images, - func(index int, image *ngmodels.Image) error { + func(_ int, image ngmodels.Image) error { // If there is an image for this alert and the image has been uploaded // to a public URL then add it to the request. We cannot add more than // one image per request. - if image != nil && image.URL != "" && imageURL == "" { - imageURL = image.URL + if image.URL != "" { + labels["imageURL"] = image.URL return ErrImagesDone } return nil }, as...) - if imageURL != "" { - labels["imageURL"] = imageURL - } ruleURL := joinUrlPath(sn.tmpl.ExternalURL.String(), "/alerting/list", sn.log) labels["ruleURL"] = ruleURL diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index 282e4f73857..741fde200cc 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -319,15 +319,10 @@ func (sn *SlackNotifier) buildSlackMessage(ctx context.Context, alrts []*types.A }, } - _ = withStoredImage(ctx, sn.log, sn.images, - func(index int, image *ngmodels.Image) error { - if image != nil { - req.Attachments[0].ImageURL = image.URL - return ErrImagesDone - } - return nil - }, - 0, alrts...) + _ = withStoredImages(ctx, sn.log, sn.images, func(index int, image ngmodels.Image) error { + req.Attachments[0].ImageURL = image.URL + return ErrImagesDone + }, alrts...) if tmplErr != nil { sn.log.Warn("failed to template Slack message", "err", tmplErr.Error()) diff --git a/pkg/services/ngalert/notifier/channels/teams.go b/pkg/services/ngalert/notifier/channels/teams.go index f08f68c15b4..fd45764a5c5 100644 --- a/pkg/services/ngalert/notifier/channels/teams.go +++ b/pkg/services/ngalert/notifier/channels/teams.go @@ -95,8 +95,8 @@ func (tn *TeamsNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool, images := []teamsImage{} _ = withStoredImages(ctx, tn.log, tn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && len(image.URL) != 0 { + func(_ int, image ngmodels.Image) error { + if len(image.URL) != 0 { images = append(images, teamsImage{Image: image.URL}) } return nil diff --git a/pkg/services/ngalert/notifier/channels/telegram.go b/pkg/services/ngalert/notifier/channels/telegram.go index 030b7b301d2..650a390c0da 100644 --- a/pkg/services/ngalert/notifier/channels/telegram.go +++ b/pkg/services/ngalert/notifier/channels/telegram.go @@ -117,33 +117,31 @@ func (tn *TelegramNotifier) Notify(ctx context.Context, as ...*types.Alert) (boo } // Create the cmd to upload each image - _ = withStoredImages(ctx, tn.log, tn.images, func(index int, image *ngmodels.Image) error { - if image != nil { - cmd, err = tn.newWebhookSyncCmd("sendPhoto", func(w *multipart.Writer) error { - f, err := os.Open(image.Path) - if err != nil { - return fmt.Errorf("failed to open image: %w", err) - } - defer func() { - if err := f.Close(); err != nil { - tn.log.Warn("failed to close image", "err", err) - } - }() - fw, err := w.CreateFormFile("photo", image.Path) - if err != nil { - return fmt.Errorf("failed to create form file: %w", err) - } - if _, err := io.Copy(fw, f); err != nil { - return fmt.Errorf("failed to write to form file: %w", err) - } - return nil - }) + _ = withStoredImages(ctx, tn.log, tn.images, func(index int, image ngmodels.Image) error { + cmd, err = tn.newWebhookSyncCmd("sendPhoto", func(w *multipart.Writer) error { + f, err := os.Open(image.Path) if err != nil { - return fmt.Errorf("failed to create image: %w", err) + return fmt.Errorf("failed to open image: %w", err) } - if err := tn.ns.SendWebhookSync(ctx, cmd); err != nil { - return fmt.Errorf("failed to upload image to telegram: %w", err) + defer func() { + if err := f.Close(); err != nil { + tn.log.Warn("failed to close image", "err", err) + } + }() + fw, err := w.CreateFormFile("photo", image.Path) + if err != nil { + return fmt.Errorf("failed to create form file: %w", err) } + if _, err := io.Copy(fw, f); err != nil { + return fmt.Errorf("failed to write to form file: %w", err) + } + return nil + }) + if err != nil { + return fmt.Errorf("failed to create image: %w", err) + } + if err := tn.ns.SendWebhookSync(ctx, cmd); err != nil { + return fmt.Errorf("failed to upload image to telegram: %w", err) } return nil }, as...) diff --git a/pkg/services/ngalert/notifier/channels/threema.go b/pkg/services/ngalert/notifier/channels/threema.go index c75f5e99193..81f53290bb5 100644 --- a/pkg/services/ngalert/notifier/channels/threema.go +++ b/pkg/services/ngalert/notifier/channels/threema.go @@ -132,8 +132,8 @@ func (tn *ThreemaNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool ) _ = withStoredImages(ctx, tn.log, tn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && image.URL != "" { + func(_ int, image ngmodels.Image) error { + if image.URL != "" { message += fmt.Sprintf("*Image:* %s\n", image.URL) } return nil diff --git a/pkg/services/ngalert/notifier/channels/util.go b/pkg/services/ngalert/notifier/channels/util.go index ab9ef83bc18..130d27826cf 100644 --- a/pkg/services/ngalert/notifier/channels/util.go +++ b/pkg/services/ngalert/notifier/channels/util.go @@ -46,57 +46,51 @@ var ( ErrImagesUnavailable = errors.New("alert screenshots are unavailable") ) -// For each alert, attempts to load the models.Image for an image token -// associated with the alert, then calls forEachFunc with the index of the -// alert and the retrieved image struct. If there is no image token, or the -// image does not exist, forEachFunc will be called with a nil value for the -// image. If forEachFunc returns an error, withStoredImages will return -// immediately. If there is a runtime error retrieving images from the image -// store, withStoredImages will attempt to continue executing, after logging -// a warning. -func withStoredImages(ctx context.Context, l log.Logger, imageStore ImageStore, forEachFunc func(index int, image *models.Image) error, alerts ...*types.Alert) error { - for i := range alerts { - err := withStoredImage(ctx, l, imageStore, forEachFunc, i, alerts...) - if err != nil { - // Stop iteration as forEachFunc has found the intended image or - // iterated the maximum number of images - if errors.Is(err, ErrImagesDone) { - return nil - } - return err - } +type forEachImageFunc func(index int, image models.Image) error + +// getImage returns the image for the alert or an error. It returns a nil +// image if the alert does not have an image token or the image does not exist. +func getImage(ctx context.Context, l log.Logger, imageStore ImageStore, alert types.Alert) (*models.Image, error) { + token := getTokenFromAnnotations(alert.Annotations) + if token == "" { + return nil, nil + } + + ctx, cancelFunc := context.WithTimeout(ctx, ImageStoreTimeout) + defer cancelFunc() + + img, err := imageStore.GetImage(ctx, token) + if errors.Is(err, models.ErrImageNotFound) || errors.Is(err, ErrImagesUnavailable) { + return nil, nil + } else if err != nil { + l.Warn("failed to get image with token", "token", token, "err", err) + return nil, err + } else { + return img, nil } - return nil } -func withStoredImage(ctx context.Context, l log.Logger, imageStore ImageStore, imageFunc func(index int, image *models.Image) error, index int, alerts ...*types.Alert) error { - imgToken := getTokenFromAnnotations(alerts[index].Annotations) - if len(imgToken) == 0 { - err := imageFunc(index, nil) +// withStoredImages retrieves the image for each alert and then calls forEachFunc +// with the index of the alert and the retrieved image struct. If the alert does +// not have an image token, or the image does not exist then forEachFunc will not be +// called for that alert. If forEachFunc returns an error, withStoredImages will return +// the error and not iterate the remaining alerts. A forEachFunc can return ErrImagesDone +// to stop the iteration of remaining alerts if the intended image or maximum number of +// images have been found. +func withStoredImages(ctx context.Context, l log.Logger, imageStore ImageStore, forEachFunc forEachImageFunc, alerts ...*types.Alert) error { + for index, alert := range alerts { + img, err := getImage(ctx, l, imageStore, *alert) if err != nil { return err + } else if img != nil { + if err := forEachFunc(index, *img); err != nil { + if errors.Is(err, ErrImagesDone) { + return nil + } + return err + } } } - - timeoutCtx, cancel := context.WithTimeout(ctx, ImageStoreTimeout) - img, err := imageStore.GetImage(timeoutCtx, imgToken) - cancel() - - if errors.Is(err, models.ErrImageNotFound) || errors.Is(err, ErrImagesUnavailable) { - err := imageFunc(index, nil) - if err != nil { - return err - } - } else if err != nil { - // Ignore errors. Don't log "ImageUnavailable", which means the storage doesn't exist. - l.Warn("failed to retrieve image url from store", "err", err) - } - - err = imageFunc(index, img) - if err != nil { - return err - } - return nil } diff --git a/pkg/services/ngalert/notifier/channels/util_test.go b/pkg/services/ngalert/notifier/channels/util_test.go index beec5dc848f..36049000a59 100644 --- a/pkg/services/ngalert/notifier/channels/util_test.go +++ b/pkg/services/ngalert/notifier/channels/util_test.go @@ -45,7 +45,7 @@ func TestWithStoredImages(t *testing.T) { ) // should iterate all images - err = withStoredImages(ctx, log.New(ctx), imageStore, func(index int, image *models.Image) error { + err = withStoredImages(ctx, log.New(ctx), imageStore, func(index int, image models.Image) error { i += 1 return nil }, alerts...) @@ -54,7 +54,7 @@ func TestWithStoredImages(t *testing.T) { // should iterate just the first image i = 0 - err = withStoredImages(ctx, log.New(ctx), imageStore, func(index int, image *models.Image) error { + err = withStoredImages(ctx, log.New(ctx), imageStore, func(index int, image models.Image) error { i += 1 return ErrImagesDone }, alerts...) diff --git a/pkg/services/ngalert/notifier/channels/victorops.go b/pkg/services/ngalert/notifier/channels/victorops.go index 072ce92d5e3..cfd66db7b97 100644 --- a/pkg/services/ngalert/notifier/channels/victorops.go +++ b/pkg/services/ngalert/notifier/channels/victorops.go @@ -119,8 +119,8 @@ func (vn *VictoropsNotifier) Notify(ctx context.Context, as ...*types.Alert) (bo bodyJSON.Set("monitoring_tool", "Grafana v"+setting.BuildVersion) _ = withStoredImages(ctx, vn.log, vn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && image.URL != "" { + func(index int, image ngmodels.Image) error { + if image.URL != "" { bodyJSON.Set("image_url", image.URL) return ErrImagesDone } diff --git a/pkg/services/ngalert/notifier/channels/webhook.go b/pkg/services/ngalert/notifier/channels/webhook.go index b88723277dd..4e92444cfed 100644 --- a/pkg/services/ngalert/notifier/channels/webhook.go +++ b/pkg/services/ngalert/notifier/channels/webhook.go @@ -120,8 +120,8 @@ func (wn *WebhookNotifier) Notify(ctx context.Context, as ...*types.Alert) (bool // Augment our Alert data with ImageURLs if available. _ = withStoredImages(ctx, wn.log, wn.images, - func(index int, image *ngmodels.Image) error { - if image != nil && len(image.URL) != 0 { + func(index int, image ngmodels.Image) error { + if len(image.URL) != 0 { data.Alerts[index].ImageURL = image.URL } return nil