feat(notifier): independent alertmanager sendloops by siavashs · Pull Request #16355 · prometheus/prometheus

siavashs · 2025-03-31T03:10:54Z

Independent Alertmanager sendloops avoid issues with queue overflowing when one or more Alertmanager instances are unavailable which could result in lost alert notifications.
The sendloops are managed per AlertmanagerSet which are dynamically added/removed with service discovery or configuration reload.

The following metrics now include an extra dimention for alertmanager label:

prometheus_notifications_dropped_total
prometheus_notifications_queue_capacity
prometheus_notifications_queue_length

Which issue(s) does the PR fix:

Closes #7676

This PR also includes the test from #14099

Does this PR introduce a user-facing change?

[ENHANCEMENT] independent alertmanager sendloops
[CHANGE] add `alertmanager` dimention to following metrics: `prometheus_notifications_dropped_total`, `prometheus_notifications_queue_capacity`, `prometheus_notifications_queue_length`

Signed-off-by: Siavash Safi siavash@cloudflare.com
Signed-off-by: machine424 ayoubmrini424@gmail.com

OGKevin

lgtm

krajorama

Hi, awesome start! I've done my first pass on the code and gave a couple of comments.

In general I'm not against the refactoring part, but I'm not sure about other reviewers, so I'm looking for some feedback on that. I'd also prefer to have the refactoring done in a separate PR up front, to get it out of the way and make this PR smaller. Let's see what other say on that first.

notifier/queue.go

notifier/manager.go

notifier/alertmanagerset.go

siavashs · 2025-04-01T08:59:05Z

Hi, awesome start! I've done my first pass on the code and gave a couple of comments.

In general I'm not against the refactoring part, but I'm not sure about other reviewers, so I'm looking for some feedback on that. I'd also prefer to have the refactoring done in a separate PR up front, to get it out of the way and make this PR smaller. Let's see what other say on that first.

Hi, thanks for your feedback.

The refactoring commit is kept separate, so we can easily do another PR for that one first.
I can submit that later today.

krajorama · 2025-04-02T07:20:32Z

Hi, awesome start! I've done my first pass on the code and gave a couple of comments.
In general I'm not against the refactoring part, but I'm not sure about other reviewers, so I'm looking for some feedback on that. I'd also prefer to have the refactoring done in a separate PR up front, to get it out of the way and make this PR smaller. Let's see what other say on that first.

Hi, thanks for your feedback.

The refactoring commit is kept separate, so we can easily do another PR for that one first. I can submit that later today.

Hi, we discussed this with @grobinson-grafana and yes we'd like to have the refactoring in a separate PR for easy review and merge. Thanks in advance!

siavashs · 2025-04-03T06:54:36Z

Hi, awesome start! I've done my first pass on the code and gave a couple of comments.
In general I'm not against the refactoring part, but I'm not sure about other reviewers, so I'm looking for some feedback on that. I'd also prefer to have the refactoring done in a separate PR up front, to get it out of the way and make this PR smaller. Let's see what other say on that first.

Hi, thanks for your feedback.
The refactoring commit is kept separate, so we can easily do another PR for that one first. I can submit that later today.

Hi, we discussed this with @grobinson-grafana and yes we'd like to have the refactoring in a separate PR for easy review and merge. Thanks in advance!

Created #16398

krajorama · 2025-04-22T06:51:55Z

hi @siavashs , will you have time to rebase this PR and continue? thanks

notifier/alertmanagerset.go

notifier/metric.go

siavashs · 2025-05-02T10:04:03Z

@krajorama this is ready for another review.

notifier/alertmanagerset.go

krajorama

I have not gone into it super deep again. I was ok with the general way of working already.

I've made a few comments on the tests, nothing blocking you really.

notifier/manager_test.go

siavashs · 2025-11-22T12:46:40Z

I have not gone into it super deep again. I was ok with the general way of working already.

I've made a few comments on the tests, nothing blocking you really.

Thanks for the suggestions @krajorama
I applied them all.

siavashs · 2025-11-22T13:40:27Z

Not sure why random unrelated tests are failing on Windows!

machine424

Thanks again, Siavash, for working on this, and thanks to the other reviewers as well.

This is not an easy change, but we all agree we're getting close to the finish line.

I’ve left some comments.

Because we're touching a battle tested logic, I’ll take another look with fresh eyes tomorrow.
Also I'll try to offload any changes that are not required for the refactor.

notifier/alertmanager_test.go

notifier/metric.go

notifier/manager_test.go

notifier/alert.go

notifier/manager_test.go

notifier/alertmanagerset.go

notifier/util_bench_test.go

siavashs · 2025-11-25T12:14:56Z

I’ve left some comments.

Because we're touching a battle tested logic, I’ll take another look with fresh eyes tomorrow. Also I'll try to offload any changes that are not required for the refactor.

Thanks for the review Ayoub, I think I addressed all the new comments.

siavashs · 2025-11-25T13:00:30Z

~~CI tests are failing but I cannot reproduce the failure locally.~~
Managed to reproduce the error using -count=50.

notifier/sendloop.go

siavashs · 2025-11-25T17:27:51Z

Found and fixed a race condition on drain when updating metrics.

machine424

some final ones.

notifier/manager_test.go

notifier/alertmanagerset.go

notifier/manager_test.go

Ref: prometheus#7676 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Siavash Safi <siavash@cloudflare.com>

Signed-off-by: Siavash Safi <siavash@cloudflare.com>

siavashs · 2025-11-28T11:39:47Z

Github is having a bad day, random stuff are failing.

siavashs · 2025-11-28T15:08:09Z

All checks are green again 🎉

siavashs · 2025-11-28T15:08:54Z

Thanks for the last review @machine424
Let me know how to proceed with the last few items remaining.

notifier/manager_test.go

machine424

once #16355 (comment) is addressed.

let's give this a try, thanks again for working on it!

siavashs · 2026-01-12T12:36:33Z

Thank you @machine424
I resolved that comment.

siavashs · 2026-01-12T13:41:04Z

Hmm, I made a small change and now random unrelated CI checks are failing 🤔

Independent Alertmanager sendloops avoid issues with queue overflowing when one or more Alertmanager instances are unavailable which could result in lost alert notifications. The sendloops are managed per AlertmanagerSet which are dynamically added/removed with service discovery or configuration reload. The following metrics now include an extra dimention for alertmanager label: - prometheus_notifications_dropped_total - prometheus_notifications_queue_capacity - prometheus_notifications_queue_length This change also includes the test from prometheus#14099 Closes prometheus#7676 Signed-off-by: machine424 <ayoubmrini424@gmail.com> Signed-off-by: Siavash Safi <siavash@cloudflare.com>

siavashs · 2026-01-13T11:05:39Z

All green again, let's merge this thing 😄

machine424 · 2026-01-20T09:55:06Z

I was on PTO and just returned today, thanks for taking care of this.

machine424 · 2026-01-20T10:15:12Z

seems TestHandlerSendAll is flaky onboth linux https://github.com/prometheus/prometheus/actions/runs/21166419284/job/60872149084 and windows https://github.com/prometheus/prometheus/actions/runs/21166419284/job/60872149078

maybe it only flakes on the CI, we can add some debug logs and make it run there

siavashs · 2026-01-20T10:43:06Z

seems TestHandlerSendAll is flaky onboth linux https://github.com/prometheus/prometheus/actions/runs/21166419284/job/60872149084 and windows https://github.com/prometheus/prometheus/actions/runs/21166419284/job/60872149078

maybe it only flakes on the CI, we can add some debug logs and make it run there

I could not reproduce this locally. Can we bump the CI machine to a bigger one?

machine424 · 2026-01-20T11:39:15Z

let's try to fix the test/or the code for the current CI first.
You can open a PR with some fmt.Println() around the check to see what's going on and try to reproduce the failure.

(I can look into this if you want when I have some time)

siavashs force-pushed the notifier-queue-per-am branch 3 times, most recently from 299dfc8 to f1ea687 Compare March 31, 2025 07:39

OGKevin reviewed Mar 31, 2025

View reviewed changes

siavashs force-pushed the notifier-queue-per-am branch from f1ea687 to 2969971 Compare April 1, 2025 00:30

krajorama self-requested a review April 1, 2025 07:12

krajorama reviewed Apr 1, 2025

View reviewed changes

notifier/queue.go Outdated Show resolved Hide resolved

notifier/manager.go Outdated Show resolved Hide resolved

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

krajorama requested a review from grobinson-grafana April 1, 2025 08:13

siavashs force-pushed the notifier-queue-per-am branch 6 times, most recently from e6e57c0 to 1605a50 Compare April 8, 2025 19:42

siavashs requested a review from krajorama April 8, 2025 19:42

siavashs force-pushed the notifier-queue-per-am branch from 1605a50 to 410c4f3 Compare April 22, 2025 07:55

siavashs marked this pull request as ready for review April 28, 2025 08:31

krajorama reviewed Apr 30, 2025

View reviewed changes

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

siavashs force-pushed the notifier-queue-per-am branch 3 times, most recently from 70395be to 5438ece Compare May 1, 2025 15:21

siavashs requested a review from krajorama May 1, 2025 15:37

siavashs commented May 2, 2025

View reviewed changes

notifier/metric.go Outdated Show resolved Hide resolved

siavashs force-pushed the notifier-queue-per-am branch from 5438ece to 62c7f17 Compare May 2, 2025 09:59

krajorama reviewed May 6, 2025

View reviewed changes

notifier/alertmanagerset.go Outdated Show resolved Hide resolved

krajorama self-assigned this Nov 20, 2025

krajorama approved these changes Nov 21, 2025

View reviewed changes

siavashs force-pushed the notifier-queue-per-am branch from 7fb7183 to c00afa2 Compare November 22, 2025 12:45

siavashs force-pushed the notifier-queue-per-am branch 2 times, most recently from 694f668 to 9189c9a Compare November 22, 2025 13:08

machine424 reviewed Nov 24, 2025

View reviewed changes

roidelapluie reviewed Nov 25, 2025

View reviewed changes

notifier/sendloop.go Show resolved Hide resolved

machine424 reviewed Nov 27, 2025

View reviewed changes

krajorama and others added 2 commits November 28, 2025 12:16

notifier: unit test for dropping throughput on stuck AM

64a6651

Ref: prometheus#7676 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Siavash Safi <siavash@cloudflare.com>

chore(notifier): remove year from copyrights

0ceb1fa

Signed-off-by: Siavash Safi <siavash@cloudflare.com>

machine424 reviewed Nov 30, 2025

View reviewed changes

notifier/manager_test.go Show resolved Hide resolved

machine424 approved these changes Dec 30, 2025

View reviewed changes

OGKevin mentioned this pull request Jan 13, 2026

feat(notifier): add OpenTelemetry tracing to sendloop #17848

Open

56quarters mentioned this pull request Jan 21, 2026

Flaky TestDefaultMultiTenantManager_WaitsToDrainPendingNotificationsOnShutdown grafana/mimir#14116

Closed

Conversation

siavashs commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does the PR fix:

Does this PR introduce a user-facing change?

Uh oh!

OGKevin left a comment

Choose a reason for hiding this comment

Uh oh!

krajorama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siavashs commented Apr 1, 2025

Uh oh!

krajorama commented Apr 2, 2025

Uh oh!

siavashs commented Apr 3, 2025

Uh oh!

krajorama commented Apr 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siavashs commented May 2, 2025

Uh oh!

Uh oh!

krajorama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siavashs commented Nov 22, 2025

Uh oh!

siavashs commented Nov 22, 2025

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siavashs commented Nov 25, 2025

Uh oh!

siavashs commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

siavashs commented Nov 25, 2025

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siavashs commented Nov 28, 2025

Uh oh!

siavashs commented Nov 28, 2025

Uh oh!

siavashs commented Nov 28, 2025

siavashs commented Mar 31, 2025 •

edited

Loading

siavashs commented Nov 25, 2025 •

edited

Loading