Alerting: Fetch alerts from a remote Alertmanager #75844

santihernandezc · 2023-10-02T18:49:59Z

This PR enables the proxy Alertmanager struct to retrieve alerts from the remote Alertmanager. The methods for enabling this feature are part of the Alertmanager interface:

GetAlerts()
GetAlertGroups()

As we need to communicate to an Alertmanager via HTTP requests, these two methods now take a context.Context as the first argument.

The PutAlerts() method needs to be implemented in the way described in #76692

santihernandezc · 2023-10-02T18:52:11Z

pkg/services/ngalert/notifier/alertmanager.go

@@ -381,7 +381,7 @@ func (am *alertmanager) buildReceiverIntegrations(receiver *alertingNotify.APIRe
 }

 // PutAlerts receives the alerts and then sends them through the corresponding route based on whenever the alert has a receiver embedded or not
-func (am *alertmanager) PutAlerts(postableAlerts apimodels.PostableAlerts) error {
+func (am *alertmanager) PutAlerts(_ context.Context, postableAlerts apimodels.PostableAlerts) error {


We don't use the context.Context in the internal Alertmanager, but these methods need to accept one to implement the Alertmanager interface and be able to swap Alertmanager structs in the future.

santihernandezc · 2023-10-02T18:53:57Z

pkg/services/ngalert/notifier/external_alertmanager.go

@@ -103,12 +110,7 @@ func (am *externalAlertmanager) GetSilence(ctx context.Context, silenceID string
 		return apimodels.GettableSilence{}, err
 	}

-	if res != nil {


We would need to add this guard to almost every method, I think it's overkill. I'd say it's safe to assume that the go-swagger package won't introduce a breaking change like returning a nil value when no error occurred.

pkg/services/ngalert/notifier/external_alertmanager.go

santihernandezc · 2023-10-02T18:57:56Z

pkg/services/ngalert/schedule/schedule.go

@@ -36,7 +36,7 @@ type ScheduleService interface {
 //
 //go:generate mockery --name AlertsSender --structname AlertsSenderMock --inpackage --filename alerts_sender_mock.go --with-expecter
 type AlertsSender interface {
-	Send(key ngmodels.AlertRuleKey, alerts definitions.PostableAlerts)
+	Send(ctx context.Context, key ngmodels.AlertRuleKey, alerts definitions.PostableAlerts)


The only change here is adding a context.Context to the interface and the .Send() calls.
I also updated the mock for this interface.

pkg/services/ngalert/schedule/schedule_unit_test.go

santihernandezc · 2023-10-04T13:47:21Z

pkg/services/ngalert/schedule/schedule_unit_test.go

@@ -59,7 +59,7 @@ func TestProcessTicks(t *testing.T) {
 	mockedClock := clock.NewMock()

 	notifier := &AlertsSenderMock{}
-	notifier.EXPECT().Send(mock.Anything, mock.Anything).Return()
+	notifier.EXPECT().Send(mock.Anything, mock.Anything, mock.Anything).Return()


Changes in this file are:

Adding a first argument corresponding to the context.Context

Checking that the third (not second) argument is of type definitions.PostableAlerts

…ernandezc/forward_alerts_to_remote_alertmanager

…ttps://github.com/grafana/grafana into santihernandezc/forward_alerts_to_remote_alertmanager

…ernandezc/forward_alerts_to_remote_alertmanager

…rtmanager

…https://github.com/grafana/grafana into santihernandezc/forward_alerts_to_remote_alertmanager

…ernandezc/forward_alerts_to_remote_alertmanager

pkg/services/ngalert/schedule/schedule.go

pkg/services/ngalert/notifier/external_alertmanager.go

…ernandezc/forward_alerts_to_remote_alertmanager

gotjosh

Overall LGTM, but please take a look at my comments.

gotjosh · 2023-10-17T10:43:59Z

pkg/services/ngalert/notifier/external_alertmanager_test.go

+	if !ok {
+		t.Skip("No Alertmanager URL provided")
+	}


This is potentially dangerous - if someone makes a mistake and renames the CI variable we wouldn't be able to tell if this test is running or not. I'd rather abort the test and tell the user the devev command they to run to make sure the test runs correctly.

I thought the same thing and started by not doing this but the CI failed in normal integration tests.
I then checked how we handle tests for e.g. Memcached, Redis storage, Redis cache, Postgres and MySQL which are the other integration tests that use docker containers. All of them use this pattern.

Ugh, I don't love this approach but if there's so much precedence I take it it might not be a simple problem. Can you file an issue and attach it in a separate task list to the epic so that can investigate later? There are ways to simply skip these tests if we don't have the right setup in the CI.

Issue created: #76798

gotjosh · 2023-10-17T11:00:55Z

pkg/services/ngalert/notifier/external_alertmanager.go

+	alerts := make(alertingNotify.PostableAlerts, 0, len(postableAlerts.PostableAlerts))
+	for _, pa := range postableAlerts.PostableAlerts {
+		alerts = append(alerts, &alertingNotify.PostableAlert{
+			Annotations: pa.Annotations,
+			EndsAt:      pa.EndsAt,
+			StartsAt:    pa.StartsAt,
+			Alert:       pa.Alert,
+		})
+	}
+
+	params := amalert.NewPostAlertsParamsWithContext(ctx).WithAlerts(alerts)
+	_, err := am.amClient.Alert.PostAlerts(params)
+	return err


This is not the implementation that we want here, I would prefer if we leave it out of this PR and just let this PR handle reads.

I'll leave you with the details so that you can file an issue and add it as a follow-up.

If you look closely at how prometheus handles sending of alerts to the alertmanager, you'll realise it's a combination of two things:

The SendAlerts function

The Notifier struct

This is important because the notifier uses an internal queue to make sure alerts are delivered in batches when possible.

The good news is that we already have this implementation in Grafana, the bad news is that we'll need to adapt it so that it works with or in the externalAlertmanager. Please note that the way that "remote URLs" are configured in the notifier is not straightforward and require some level of finesse to implement as they rely on service discovery, to top that off, the notifier is used in other parts of Grafana (hence the public API).

Issue created #76692

grobinson-grafana · 2023-10-17T11:06:14Z

pkg/services/ngalert/notifier/external_alertmanager.go

 }

-func (am *externalAlertmanager) GetAlertGroups(active, silenced, inhibited bool, filter []string, receiver string) (apimodels.AlertGroups, error) {
-	return apimodels.AlertGroups{}, nil
+func (am *externalAlertmanager) PutAlerts(ctx context.Context, postableAlerts apimodels.PostableAlerts) error {


Should add a recover here as PostAlerts can panic.

grobinson-grafana · 2023-10-17T11:06:52Z

pkg/services/ngalert/notifier/external_alertmanager.go

@@ -121,30 +123,62 @@ func (am *externalAlertmanager) ListSilences(ctx context.Context, filter []strin
 	return res.Payload, nil
 }

-func (am *externalAlertmanager) GetStatus() apimodels.GettableStatus {
-	return apimodels.GettableStatus{}
+func (am *externalAlertmanager) GetAlerts(ctx context.Context, active, silenced, inhibited bool, filter []string, receiver string) (apimodels.GettableAlerts, error) {


Should add a recover here as GetAlerts can panic.

Where exactly can it panic? The res.Payload bit?

GetAlerts(params) panics

grobinson-grafana · 2023-10-17T11:07:29Z

pkg/services/ngalert/notifier/external_alertmanager.go

 }

-func (am *externalAlertmanager) GetAlerts(active, silenced, inhibited bool, filter []string, receiver string) (apimodels.GettableAlerts, error) {
-	return apimodels.GettableAlerts{}, nil
+func (am *externalAlertmanager) GetAlertGroups(ctx context.Context, active, silenced, inhibited bool, filter []string, receiver string) (apimodels.AlertGroups, error) {


Should add a recover here as GetAlertGroups can panic.

…nt that could panic

… does

pkg/services/ngalert/notifier/external_alertmanager.go

gotjosh · 2023-10-17T18:54:27Z

@grobinson-grafana can you elaborate under which conditions one would expect the panic? From reading the client code it seems to be there as a safeguard for code generation issues on upstream - with the amount of coverage we have on this client (all of our integrations test use it) I'm not sure it's worth it the recover.

But perhaps this is customary of using openapi clients?

gotjosh

LGTM - but I'm dubious on wether we need the whole recover as part of the client calls.

Changes addressed

grobinson-grafana · 2023-10-19T09:53:01Z

@grobinson-grafana can you elaborate under which conditions one would expect the panic? From reading the client code it seems to be there as a safeguard for code generation issues on upstream - with the amount of coverage we have on this client (all of our integrations test use it) I'm not sure it's worth it the recover.

If you have 100% confidence in the codegen I'd be very happy to remove it. I figured anywhere there is an explicit panic, codegen or otherwise, better to be defensive and recover then risk one bug bringing down the entire process.

Alerting: post alerts to the remote Alertmanager and fetch them

4ae39d0

santihernandezc added no-backport Skip backport of PR add to changelog labels Oct 2, 2023

grafana-delivery-bot bot added this to the 10.2.x milestone Oct 2, 2023

santihernandezc added the kata:am_unify label Oct 2, 2023

grafana-pr-automation bot added the area/backend label Oct 2, 2023

santihernandezc commented Oct 2, 2023

View reviewed changes

fix broken tests

9d806b0

santihernandezc commented Oct 4, 2023

View reviewed changes

santihernandezc added 13 commits October 10, 2023 09:43

Merge branch 'main' of https://github.com/grafana/grafana into santih…

9aad249

…ernandezc/forward_alerts_to_remote_alertmanager

Alerting: Add Mimir Backend image to devenv (blocks)

1f2bc01

add alerting as code owner for mimir_backend block

cf98d4f

Merge branch 'santihernandezc/add_mimir_backend_image_to_devenv' of h…

258f741

…ttps://github.com/grafana/grafana into santihernandezc/forward_alerts_to_remote_alertmanager

Merge branch 'main' of https://github.com/grafana/grafana into santih…

eb3a20a

…ernandezc/forward_alerts_to_remote_alertmanager

Alerting: Use Mimir image to run integration tests for the remote Ale…

938dd58

…rtmanager

Merge branch 'santihernandezc/use_mimir_backend_image_to_run_tests' of …

03bc6a0

…https://github.com/grafana/grafana into santihernandezc/forward_alerts_to_remote_alertmanager

skip integration test when running all tests

9791762

skipping integration test when no Alertmanager URL is provided

73c20dc

fix bad host for mimir_backend

66629f3

remove basic auth testing until we have an nginx image in our CI

51fa071

Merge branch 'santihernandezc/use_mimir_backend_image_to_run_tests' of …

cbbe173

…https://github.com/grafana/grafana into santihernandezc/forward_alerts_to_remote_alertmanager

add integration tests for alerts

487ebd0

santihernandezc marked this pull request as ready for review October 16, 2023 15:39

santihernandezc requested review from a team, rwwiv, JacobsonMT, yuri-tceretian and grobinson-grafana and removed request for a team October 16, 2023 15:39

grafana-pr-automation bot added the type/build-packaging label Oct 16, 2023

santihernandezc requested a review from a team as a code owner October 16, 2023 20:34

Merge branch 'main' of https://github.com/grafana/grafana into santih…

5c85df3

…ernandezc/forward_alerts_to_remote_alertmanager

santihernandezc force-pushed the santihernandezc/forward_alerts_to_remote_alertmanager branch from 0872944 to 5c85df3 Compare October 16, 2023 20:38

fix tests

0f3224b

santihernandezc force-pushed the santihernandezc/forward_alerts_to_remote_alertmanager branch from be72f88 to 0f3224b Compare October 16, 2023 21:12

grobinson-grafana reviewed Oct 17, 2023

View reviewed changes

pkg/services/ngalert/schedule/schedule.go Outdated Show resolved Hide resolved

grobinson-grafana reviewed Oct 17, 2023

View reviewed changes

pkg/services/ngalert/notifier/external_alertmanager.go Show resolved Hide resolved

santihernandezc added 2 commits October 17, 2023 12:25

Merge branch 'main' of https://github.com/grafana/grafana into santih…

7342f03

…ernandezc/forward_alerts_to_remote_alertmanager

change SendCtx -> Send, add context.Context to Send, fix CI

6f01d30

gotjosh reviewed Oct 17, 2023

View reviewed changes

grobinson-grafana previously requested changes Oct 17, 2023

View reviewed changes

santihernandezc added 2 commits October 17, 2023 15:51

add reover() for functions from the Prometheus Alertmanager HTTP clie…

29eb2ff

…nt that could panic

add TODO to implement PutAlerts in a way that mimicks what Prometheus…

9aef308

… does

santihernandezc changed the title ~~Alerting: Post alerts to a remote Alertmanager and fetch them~~ Alerting: Fetch alerts from a remote Alertmanager Oct 17, 2023

santihernandezc requested review from grobinson-grafana and gotjosh October 17, 2023 14:09

grobinson-grafana reviewed Oct 17, 2023

View reviewed changes

pkg/services/ngalert/notifier/external_alertmanager.go Outdated Show resolved Hide resolved

fix log format

8be94c7

santihernandezc requested a review from grobinson-grafana October 17, 2023 17:57

gotjosh approved these changes Oct 17, 2023

View reviewed changes

santihernandezc merged commit 61cb267 into main Oct 19, 2023
15 checks passed

santihernandezc deleted the santihernandezc/forward_alerts_to_remote_alertmanager branch October 19, 2023 09:27

grafana-delivery-bot bot modified the milestones: 10.2.x, 10.3.x Oct 19, 2023

aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Fetch alerts from a remote Alertmanager #75844

Alerting: Fetch alerts from a remote Alertmanager #75844

santihernandezc commented Oct 2, 2023 •

edited

Loading

santihernandezc Oct 2, 2023

santihernandezc Oct 2, 2023

santihernandezc Oct 2, 2023

santihernandezc Oct 4, 2023

gotjosh left a comment

gotjosh Oct 17, 2023

santihernandezc Oct 17, 2023 •

edited

Loading

gotjosh Oct 17, 2023 •

edited

Loading

santihernandezc Oct 19, 2023

gotjosh Oct 17, 2023

santihernandezc Oct 17, 2023

grobinson-grafana Oct 17, 2023

grobinson-grafana Oct 17, 2023

santihernandezc Oct 17, 2023

grobinson-grafana Oct 17, 2023

grobinson-grafana Oct 17, 2023

gotjosh commented Oct 17, 2023

gotjosh left a comment

grobinson-grafana commented Oct 19, 2023

Alerting: Fetch alerts from a remote Alertmanager #75844

Alerting: Fetch alerts from a remote Alertmanager #75844

Conversation

santihernandezc commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santihernandezc Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

gotjosh Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh commented Oct 17, 2023

gotjosh left a comment

Choose a reason for hiding this comment

grobinson-grafana commented Oct 19, 2023

santihernandezc commented Oct 2, 2023 •

edited

Loading

santihernandezc Oct 17, 2023 •

edited

Loading

gotjosh Oct 17, 2023 •

edited

Loading