-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add groupKey to alerts/groups API endpoint #576
add groupKey to alerts/groups API endpoint #576
Conversation
@@ -111,6 +112,7 @@ func (d *Dispatcher) Groups() AlertOverview { | |||
alertGroup, ok := seen[ag.fingerprint()] | |||
if !ok { | |||
alertGroup = &AlertGroup{Labels: ag.labels} | |||
alertGroup.GroupKey = uint64(ag.labels.Fingerprint()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually generated differently: https://github.com/prometheus/alertmanager/blob/master/dispatch/dispatch.go#L332.
Probably best to at a GroupKey() uint64
method to the aggregation group to avoid getting out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad 🙈
…tion for context initialization
Updated the PR as proposed in the comment |
@@ -362,6 +364,10 @@ func (ag *aggrGroup) fingerprint() model.Fingerprint { | |||
return ag.labels.Fingerprint() | |||
} | |||
|
|||
func (ag *aggrGroup) GroupKey() uint64 { | |||
return uint64(ag.labels.Fingerprint() ^ ag.routeFP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but we can just keep the Fingerprint
type here all the way no? Then we don't have to do any casting along the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I dislike the Fingerprint type with a passion – algorithm as well as naming. I'd like to get rid of it everywhere at some point and would rather not propagate it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the same about the Fingerprint, but sticked with the grouopKey
as it was already in the webhook. This is up to you guys :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sticking to the "Fingerprint" type is fine by me as well as long as we stick with the same type. But in the context of this change I guess the way it is is fine then. Nevermind me here :)
LGTM 👍 |
Discussion started at #574
Add the
groupKey
information to thealerts/groups
endpoint.When having configured a webhook receiver it contains the
groupKey
. Adding thegroupKey
to the overview of alertgroups makes it possible to match the incoming webhook with groups reported back from the API.