Replace Intervener with ActiveIntervener and MuteIntervener #3562
Conversation
This commit replaces the Intervener in timeinterval package with an ActiveIntervener and a MuteIntervener for active time intervals and mute time intervals. It avoids having a double-negative in TimeActiveStage where Mute returns true if its not muted rather than if it is muted, and will also enable us to mark alerts as muted or not within ActiveIntervener and MuteIntervener. Signed-off-by: George Robinson <[email protected]>
grobinson-grafana
force-pushed
the
grobinson/active-and-mute-intervener
branch
from
fd45cea to
e8159a8
Compare
Signed-off-by: George Robinson <[email protected]>
| intervals map[string][]TimeInterval | ||
| } | ||
|
|
||
| func (i *ActiveIntervener) Mutes(names []string, now time.Time) (bool, error) { |
There was a problem hiding this comment.
I'm not a fan of this - this is pretty much the same logic as the other but with a flipped conditional. I'd rather keep it as is and continue to have the !muted on the stage itself.
This seems like a lot of code to change/add for the sake of a flipping a conditional.
There was a problem hiding this comment.
That's OK. We'll then need to put the marker back into TimeMuteStage and TimeActiveStage though as otherwise Intervener doesn't know if it's activing or muting.
This seems like a lot of code to change/add for the sake of a flipping a conditional.
Don't forget we'll also be adding the marker, so it won't just be the conditional that changes, but how alerts are marked as muted too.
There was a problem hiding this comment.
Maybe we should introduce it then? It's hard for me to see the benefit of this as is. 😭
There was a problem hiding this comment.
I do think making time active intervals adhere to the contract of Mute is a good win in of itself. Mute no longer returns false when it should be muted, and true when it shouldn't be muted.
There was a problem hiding this comment.
I think my suggestion here would be to keep everything under 1 single interface but add an additional method to it.
There was a problem hiding this comment.
That still doesn't work I'm afraid, because Intervener doesn't know if the intervals in names are active time intervals or mute time intervals.
This commit replaces the Intervener in timeinterval package with an ActiveIntervener and a MuteIntervener for active time intervals and mute time intervals. It avoids having a double-negative in TimeActiveStage where Mute returns true if its not muted rather than if it is muted, and will also enable us to mark alerts as muted or not within ActiveIntervener and MuteIntervener.