-
Notifications
You must be signed in to change notification settings - Fork 2.4k
add silence annotations #4965
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
base: main
Are you sure you want to change the base?
add silence annotations #4965
Conversation
39f20ee to
10a14d3
Compare
SoloJacobs
left a comment
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 think a bit more test coverage would go here, most importantly a round trip test:
post silence (using amtool) -> get silence -> compare annotations.
cli/silence_add.go and api/v2/compat.go could also be tested.
I also assume that we expect amtool to not have an error, if you post to an older alertmanager version?
Sure, I'll add a test case for the round trip, that's a good idea.
Ah yeah, let me also make sure that works - I think if you set annotations and try to post to an old alertmanager it should be expected to break. If you don't, I think it's reasonable to make it work. We don't really have a version skew policy, but I guess at some point we'll need to think about guidelines for which versions of |
Signed-off-by: Ethan Hunter <[email protected]>
297dd02 to
ea7f90e
Compare
Signed-off-by: Ethan Hunter <[email protected]>
ea7f90e to
ce8d6c3
Compare
|
For a users perspective the optimal behaviour would be |
Agreed this would be the nicest behavior. However, it's a bit of work to implement because I don't know what semantic version of alertmanager will start supporting silence annotations... I could think of something to make this work, but given that this is a bit of a corner case (the user is using a new version of |
siavashs
left a comment
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.
LGTM
Co-authored-by: Siavash Safi <[email protected]> Signed-off-by: Ethan Hunter <[email protected]>
|
I quite like the new test! Thanks for adding it. Regarding the |
Silence annotations are... annotations... for silences!
Just like alert annotations, silence annotations are an optional
LabelSetwhich provides structured key->value data on silences. Annotations are added to the underlying cluster model for silences so they're synced properly between nodes in HA mode.This is a simple change that adds a lot of value: with structured fields, it's possible to add both machine and human readble metadata for silences. In HRT, we use annotations to indicate the purpose of silences, associate them with machine processes, and encode conventions. The format is flexible so it's possible to encode any number of things.