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

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

siavashs force-pushed the notifier-queue-per-am branch 2 times, most recently from 3e7680c to 12352ae Compare November 28, 2025 11:06

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>

siavashs force-pushed the notifier-queue-per-am branch 2 times, most recently from 0417be9 to 78b490d Compare November 28, 2025 11:28

siavashs force-pushed the notifier-queue-per-am branch 2 times, most recently from ffe57b9 to 38f95f2 Compare November 28, 2025 14:02

machine424 reviewed Nov 30, 2025

View reviewed changes

notifier/manager_test.go Show resolved Hide resolved

siavashs force-pushed the notifier-queue-per-am branch from 38f95f2 to ea05297 Compare December 2, 2025 12:49

siavashs requested a review from machine424 December 2, 2025 12:51

machine424 approved these changes Dec 30, 2025

View reviewed changes

siavashs force-pushed the notifier-queue-per-am branch from ea05297 to 63c4b7c Compare January 12, 2026 12:35

siavashs force-pushed the notifier-queue-per-am branch from 63c4b7c to 5bcb4dd Compare January 12, 2026 14:43

siavashs force-pushed the notifier-queue-per-am branch from 5bcb4dd to 206e76a Compare January 13, 2026 10:35

OGKevin mentioned this pull request Jan 13, 2026

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

Open

SuperQ merged commit a89c665 into prometheus:main Jan 20, 2026
32 checks passed

siavashs deleted the notifier-queue-per-am branch January 20, 2026 10:01

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!

siavashs commented Nov 28, 2025

Uh oh!

siavashs commented Nov 28, 2025

Uh oh!

siavashs commented Nov 28, 2025

Uh oh!

Uh oh!

machine424 left a comment

Choose a reason for hiding this comment

Uh oh!

siavashs commented Jan 12, 2026

Uh oh!

siavashs commented Jan 12, 2026

Uh oh!

siavashs commented Jan 13, 2026

Uh oh!

Uh oh!

machine424 commented Jan 20, 2026

Uh oh!

machine424 commented Jan 20, 2026

Uh oh!

siavashs commented Jan 20, 2026

Uh oh!

machine424 commented Jan 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

siavashs commented Mar 31, 2025 •

edited

Loading