From dcc473b4915d3682b338badb964390712a8f7d1d Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 25 Jul 2023 15:46:59 +0100 Subject: [PATCH 1/5] fix step --- .../escalation_snapshot/escalation_snapshot_mixin.py | 12 ++---------- engine/apps/alerts/tasks/distribute_alert.py | 11 ++++++----- .../alerts/tests/test_escalation_snapshot_mixin.py | 2 +- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py b/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py index 6196506ad4..7af4e8c68a 100644 --- a/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py +++ b/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py @@ -198,9 +198,7 @@ def _deserialize_escalation_snapshot(self, raw_escalation_snapshot) -> Escalatio @property def escalation_chain_exists(self) -> bool: - if self.pause_escalation: - return False - elif not self.channel_filter: + if not self.channel_filter: return False return self.channel_filter.escalation_chain is not None @@ -232,17 +230,11 @@ def start_escalation_if_needed(self, countdown=START_ESCALATION_DELAY, eta=None) is_on_maintenace_or_debug_mode = self.channel.maintenance_mode is not None - if ( - self.is_restricted - or is_on_maintenace_or_debug_mode - or self.pause_escalation - or not self.escalation_chain_exists - ): + if self.is_restricted or is_on_maintenace_or_debug_mode or not self.escalation_chain_exists: logger.debug( f"Not escalating alert group w/ pk: {self.pk}\n" f"is_restricted: {self.is_restricted}\n" f"is_on_maintenace_or_debug_mode: {is_on_maintenace_or_debug_mode}\n" - f"pause_escalation: {self.pause_escalation}\n" f"escalation_chain_exists: {self.escalation_chain_exists}" ) return diff --git a/engine/apps/alerts/tasks/distribute_alert.py b/engine/apps/alerts/tasks/distribute_alert.py index 38e1db5bb0..84412119e0 100644 --- a/engine/apps/alerts/tasks/distribute_alert.py +++ b/engine/apps/alerts/tasks/distribute_alert.py @@ -14,17 +14,18 @@ def distribute_alert(alert_id): """ We need this task to make task processing async and to make sure the task is delivered. """ - from apps.alerts.models import Alert, AlertGroup + from apps.alerts.models import Alert alert = Alert.objects.get(pk=alert_id) task_logger.debug(f"Start distribute_alert for alert {alert_id} from alert_group {alert.group_id}") send_alert_create_signal.apply_async((alert_id,)) - # If it's the first alert, let's launch the escalation! + + if alert.is_the_first_alert_in_group or alert.group.pause_escalation: + alert.group.start_escalation_if_needed(countdown=TASK_DELAY_SECONDS) + if alert.is_the_first_alert_in_group: - alert_group = AlertGroup.objects.filter(pk=alert.group_id).get() - alert_group.start_escalation_if_needed(countdown=TASK_DELAY_SECONDS) - alert_group_escalation_snapshot_built.send(sender=distribute_alert, alert_group=alert_group) + alert_group_escalation_snapshot_built.send(sender=distribute_alert, alert_group=alert.group) updated_rows = Alert.objects.filter(pk=alert_id, delivered=True).update(delivered=True) if updated_rows != 1: diff --git a/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py b/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py index 4aa984e09b..02578cbeb2 100644 --- a/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py +++ b/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py @@ -77,7 +77,7 @@ def test_build_raw_escalation_snapshot_escalation_chain_does_not_exist_escalatio channel_filter = make_channel_filter(alert_receive_channel, escalation_chain=escalation_chain) alert_group = make_alert_group(alert_receive_channel, channel_filter=channel_filter) - assert alert_group.build_raw_escalation_snapshot() == EMPTY_RAW_ESCALATION_SNAPSHOT + assert alert_group.build_raw_escalation_snapshot() != EMPTY_RAW_ESCALATION_SNAPSHOT @pytest.mark.django_db From 31adc7cee09c34043b9129deb14e75f99d24a791 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 25 Jul 2023 16:45:46 +0100 Subject: [PATCH 2/5] add tests --- engine/apps/alerts/tests/test_alert.py | 91 +++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/engine/apps/alerts/tests/test_alert.py b/engine/apps/alerts/tests/test_alert.py index 3490107531..3ada0367f6 100644 --- a/engine/apps/alerts/tests/test_alert.py +++ b/engine/apps/alerts/tests/test_alert.py @@ -1,6 +1,9 @@ +from unittest.mock import PropertyMock, patch + import pytest -from apps.alerts.models import Alert +from apps.alerts.models import Alert, EscalationPolicy +from apps.alerts.tasks import distribute_alert, escalate_alert_group @pytest.mark.django_db @@ -41,3 +44,89 @@ def test_alert_create_custom_channel_filter(make_organization, make_alert_receiv ) assert alert.group.channel_filter == other_channel_filter + + +@pytest.mark.django_db +def test_distribute_alert_escalate_alert_group( + make_organization, + make_alert_receive_channel, + make_channel_filter, + make_alert_group, + make_alert, + make_escalation_chain, + make_escalation_policy, +): + organization = make_organization() + escalation_chain = make_escalation_chain(organization) + make_escalation_policy( + escalation_chain=escalation_chain, + escalation_policy_step=EscalationPolicy.STEP_NOTIFY_MULTIPLE_USERS, + ) + alert_receive_channel = make_alert_receive_channel(organization) + channel_filter = make_channel_filter(alert_receive_channel, escalation_chain=escalation_chain) + alert_group = make_alert_group(alert_receive_channel, channel_filter=channel_filter) + + # Check escalate_alert_group is called for the first alert in the group + alert_1 = make_alert( + alert_group=alert_group, + is_the_first_alert_in_group=True, + raw_request_data=alert_receive_channel.config.example_payload, + ) + with patch.object(escalate_alert_group, "apply_async") as mock_escalate_alert_group_1: + distribute_alert(alert_1.pk) + mock_escalate_alert_group_1.assert_called_once() + + # Check escalate_alert_group is not called for the second alert in the group + alert_2 = make_alert( + alert_group=alert_group, + is_the_first_alert_in_group=False, + raw_request_data=alert_receive_channel.config.example_payload, + ) + with patch.object(escalate_alert_group, "apply_async") as mock_escalate_alert_group_2: + distribute_alert(alert_2.pk) + mock_escalate_alert_group_2.assert_not_called() + + +@pytest.mark.django_db +def test_distribute_alert_escalate_alert_group_when_escalation_paused( + make_organization, + make_alert_receive_channel, + make_channel_filter, + make_alert_group, + make_alert, + make_escalation_chain, + make_escalation_policy, +): + organization = make_organization() + escalation_chain = make_escalation_chain(organization) + make_escalation_policy( + escalation_chain=escalation_chain, + escalation_policy_step=EscalationPolicy.STEP_NOTIFY_MULTIPLE_USERS, + ) + alert_receive_channel = make_alert_receive_channel(organization) + channel_filter = make_channel_filter(alert_receive_channel, escalation_chain=escalation_chain) + alert_group = make_alert_group(alert_receive_channel, channel_filter=channel_filter) + + # Check escalate_alert_group is called for the first alert in the group + alert_1 = make_alert( + alert_group=alert_group, + is_the_first_alert_in_group=True, + raw_request_data=alert_receive_channel.config.example_payload, + ) + with patch.object(escalate_alert_group, "apply_async") as mock_escalate_alert_group_1: + distribute_alert(alert_1.pk) + mock_escalate_alert_group_1.assert_called_once() + + # Check escalate_alert_group is called for the second alert in the group when escalation is paused + alert_2 = make_alert( + alert_group=alert_group, + is_the_first_alert_in_group=False, + raw_request_data=alert_receive_channel.config.example_payload, + ) + with patch( + "apps.alerts.escalation_snapshot.escalation_snapshot_mixin.EscalationSnapshotMixin.pause_escalation", + new_callable=PropertyMock(return_value=True), + ): + with patch.object(escalate_alert_group, "apply_async") as mock_escalate_alert_group_2: + distribute_alert(alert_2.pk) + mock_escalate_alert_group_2.assert_called_once() From 3d63e4ff80410df40ca1905a310e34e800fd4c97 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 25 Jul 2023 17:12:27 +0100 Subject: [PATCH 3/5] Add comment --- engine/apps/alerts/tasks/distribute_alert.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/apps/alerts/tasks/distribute_alert.py b/engine/apps/alerts/tasks/distribute_alert.py index 84412119e0..5186fd0b45 100644 --- a/engine/apps/alerts/tasks/distribute_alert.py +++ b/engine/apps/alerts/tasks/distribute_alert.py @@ -21,6 +21,9 @@ def distribute_alert(alert_id): send_alert_create_signal.apply_async((alert_id,)) + # Launch escalation for the group if it's the first alert, or if the group is paused. + # "paused" means that the current escalation step is "Continue escalation if >X alerts per Y minutes" and there are + # not enough alerts to trigger the escalation further. if alert.is_the_first_alert_in_group or alert.group.pause_escalation: alert.group.start_escalation_if_needed(countdown=TASK_DELAY_SECONDS) From 181254861bfa1f47c661c2f7d6c430f9d2293820 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 25 Jul 2023 17:18:47 +0100 Subject: [PATCH 4/5] Add comments --- engine/apps/alerts/tasks/distribute_alert.py | 4 +++- engine/apps/alerts/tests/test_alert.py | 7 +++++++ engine/apps/alerts/tests/test_escalation_snapshot_mixin.py | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/engine/apps/alerts/tasks/distribute_alert.py b/engine/apps/alerts/tasks/distribute_alert.py index 5186fd0b45..f1d146a450 100644 --- a/engine/apps/alerts/tasks/distribute_alert.py +++ b/engine/apps/alerts/tasks/distribute_alert.py @@ -23,7 +23,9 @@ def distribute_alert(alert_id): # Launch escalation for the group if it's the first alert, or if the group is paused. # "paused" means that the current escalation step is "Continue escalation if >X alerts per Y minutes" and there are - # not enough alerts to trigger the escalation further. + # not enough alerts to trigger the escalation further. Launching escalation for a paused group will re-evaluate + # the threshold and advance the escalation if needed, or go back to the same "paused" state if the threshold is + # still not reached. if alert.is_the_first_alert_in_group or alert.group.pause_escalation: alert.group.start_escalation_if_needed(countdown=TASK_DELAY_SECONDS) diff --git a/engine/apps/alerts/tests/test_alert.py b/engine/apps/alerts/tests/test_alert.py index 3ada0367f6..77926a4461 100644 --- a/engine/apps/alerts/tests/test_alert.py +++ b/engine/apps/alerts/tests/test_alert.py @@ -56,6 +56,9 @@ def test_distribute_alert_escalate_alert_group( make_escalation_chain, make_escalation_policy, ): + """ + Check escalate_alert_group is called for the first alert in the group and not called for the second alert in the group. + """ organization = make_organization() escalation_chain = make_escalation_chain(organization) make_escalation_policy( @@ -97,6 +100,10 @@ def test_distribute_alert_escalate_alert_group_when_escalation_paused( make_escalation_chain, make_escalation_policy, ): + """ + Check escalate_alert_group is called for the first alert in the group and for the second alert in the group when + escalation is paused. + """ organization = make_organization() escalation_chain = make_escalation_chain(organization) make_escalation_policy( diff --git a/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py b/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py index 02578cbeb2..db229db943 100644 --- a/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py +++ b/engine/apps/alerts/tests/test_escalation_snapshot_mixin.py @@ -77,6 +77,7 @@ def test_build_raw_escalation_snapshot_escalation_chain_does_not_exist_escalatio channel_filter = make_channel_filter(alert_receive_channel, escalation_chain=escalation_chain) alert_group = make_alert_group(alert_receive_channel, channel_filter=channel_filter) + # Check that setting pause_escalation to True doesn't make the snapshot empty assert alert_group.build_raw_escalation_snapshot() != EMPTY_RAW_ESCALATION_SNAPSHOT From 0927711adb8614f21bec35bb71f83adb60ff6cfe Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 25 Jul 2023 17:40:28 +0100 Subject: [PATCH 5/5] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc2ca2702f..df63ad935f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed + +- Fix "Continue escalation if >X alerts per Y minutes" escalation step by @vadimkerr ([#2636](https://github.com/grafana/oncall/pull/2636)) + ## v1.3.17 (2023-07-25) ### Added