option to resolve alerts that fired inhibited #3034
Conversation
edevil
force-pushed
the
resolve_inhibited
branch
from
6eae351 to
2ccf81a
Compare
|
We are in the same situation (like many other people I guess!) and desperately need this PR to be merged or a way to resolve inhibited alerts :-) |
|
Any movement on this? A feature we desperately need! |
Added new option to let resolve notifications flow from inhibited alerts, if they fired unmuted. Signed-off-by: André Cruz <[email protected]>
edevil
force-pushed
the
resolve_inhibited
branch
from
2ccf81a to
cfd55c2
Compare
|
Adding onto this, would really really love to see this get merged into a release. Real bummer this has been stale for so long, this would be a huge feature for my org. @edevil If you aren't interested in cleaning up the merge conflicts here, I'm happy to take a stab at this. |
At this point I have resolved conflicts several times. I would like some signal from the maintainer that there is a path forward otherwise it is time wasted. |
|
I reached out in the #prometheus-alertmanager channel on the CNCF Slack asking for some maintainer eyes. |
|
@gotjosh sorry to bug ya but any chance we can push this forward? |
|
@gotjosh could you please give your opinion as this is crucial for me too (as many others) and I'd like to understand what is your take on this. if it's just an issue with risky change, specific implementation or principles I'd just like to understand if I should create my own fork too with this PR merged or could contribute somehow to this being merged. Thank you for your continuous work |
|
@bwplotka maybe your opinion would also be great |
was there a response on this? i don't understand what's blocking this PR from being merged. in #226 (comment), @brian-brazil seemed to object to having a configuration option for this in the first place, and there were lots of different ideas in that thread... i'm just not sure exactly what the way forward is, but i do know that it's pretty confusing that inhibited/silenced alerts don't show resolution, as the recipient of those notifications. |
| isMuted := n.muter.Mutes(a.Labels) | ||
| if !isMuted && !a.Resolved() && n.resolveInhibited { | ||
| a.FiredUnmuted = true | ||
| } | ||
| // Do not send the alert if muted. | ||
| if n.muter.Mutes(a.Labels) { | ||
| muted = append(muted, a) | ||
| } else { | ||
| // Do send resolved notifications if configured | ||
| // to do so and the alert fired unmuted. | ||
| if !isMuted || (a.Resolved() && a.FiredUnmuted && n.resolveInhibited) { | ||
| filtered = append(filtered, a) | ||
| } else { | ||
| muted = append(muted, a) |
There was a problem hiding this comment.
i actually find this hunk a little confusing. why are we flipping a.FiredUnmuted here? the conditional looks *really* similar to the second one there, and, what's worse, the a.FireUnmuted` is reused there which, effectively, makes the second conditional look like:
!isMuted || (a.Resolved() && (!isMuted && !a.Resolved() && n.resolveInhibited) && n.resolveInhibited) which i think is equivalent to:
!isMuted || (!a.FiredUnmuted && a.Resolved() && && n.resolveInhibited) before a.FiredUnmuted is reset. i wonder if there's a way to make this code more readable, perhaps with a comment explaing why the FiredUnmuted flag is changed there?
unfortunately it's been crickets. not sure what the best path forward here is :/ |
Added new option to let resolve notifications flow from inhibited
alerts, if they fired unmuted.
Attempts to address #226