fix(metric_alerts): Fix bug where transitioning from critical -> warning can still trigger a resolve action #29345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing can still trigger a resolve action Was investigating #27161, although I suspect that it's already resolved since it's from back in July and we pushed out a fix in Auguest. I happened to figure out the error we see on our own alert in #team-engineering though. Just to cover this issue again: - Metric goes above the critical threshold, so we fire a critical alert - Metric goes below critical but remains above warning. This should result in firing the warning threshold again, but actually it triggers a resolve. There are two parts to this issue. 1. Our metric alert is misconfigured, and doesn't have an action set up for warning. So warning thresholds will literally never fire or do anything. I have fixed this. 2. If the critical/warning actions are different, then the de-duping that we have in place won't work. In our case, since critical has an action and warning doesn't, we end up just using the critical action, which is tied to the resolve. This results in us showing a resolve notification. To fix this, when we transition from critical -> warning I only include the incident trigger associated with the active warning trigger in `fired_incident_triggers`, and leave the critical trigger out. This means that if someone configures an alert with both warning and critical triggers, but fails to set up an action on the warning trigger, then the only time the alert will fire is when the critical alert initially fires. I think this is most correct and realistically it doesn't make sense to set up a threshold without an action. We should probably prevent alerts from being created like this in the UI.
019b853 to
3812d80
Compare
wedamija
commented
Oct 15, 2021
| @@ -167,8 +165,7 @@ def find_and_fire_active_warning_trigger(self, alert_operator, aggregation_value | |||
| and alert_operator(aggregation_value, current_trigger.alert_threshold) | |||
| ): | |||
| metrics.incr("incidents.alert_rules.threshold", tags={"type": "alert"}) | |||
| active_warning_it = self.trigger_alert_threshold(current_trigger, aggregation_value) | |||
There was a problem hiding this comment.
afaict we don't actually need to do this. All this logic already happens when we resolve the critical trigger. We just want to find the active warning trigger if available and return it so that we can use it to perform any actions
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Was investigating #27161, although I suspect that it's
already resolved since it's from back in July and we pushed out a fix in August.
I happened to figure out the error we see on our own alert in #team-engineering though. Just to
cover this issue again:
threshold again, but actually it triggers a resolve.
There are two parts to this issue.
thresholds will literally never fire or do anything. I have fixed this.
work. In our case, since critical has an action and warning doesn't, we end up just using the
critical action, which is tied to the resolve. This results in us showing a resolve notification.
But this could also happen if the user set up a critical action on slack and a warning action on email.
To fix this, when we transition from critical -> warning I only include the incident trigger
associated with the active warning trigger in
fired_incident_triggers, and leave the criticaltrigger out. This means that if someone configures an alert with both warning and critical triggers,
but fails to set up an action on the warning trigger, then the only time the alert will fire is when
the critical alert initially fires.
I think this is most correct and realistically it doesn't make sense to set up a threshold without
an action. We should probably prevent alerts from being created like this in the UI.