From 41021bfb81e0e0bc861a9cbd4437426c97f9e987 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 28 Nov 2024 10:41:26 -0500 Subject: [PATCH 1/3] chore: random slack code cleanup --- engine/apps/alerts/models/alert_group.py | 9 +- .../alerts/models/alert_receive_channel.py | 30 +-- .../apps/alerts/tasks/compare_escalations.py | 4 - .../apps/alerts/tasks/escalate_alert_group.py | 3 +- engine/apps/alerts/tasks/notify_user.py | 3 +- engine/apps/alerts/tasks/unsilence.py | 6 +- engine/apps/alerts/tests/test_alert_group.py | 4 +- engine/apps/alerts/tests/test_maintenance.py | 14 +- engine/apps/alerts/tests/test_notify_user.py | 94 +++++--- engine/apps/alerts/tests/test_silence.py | 28 +-- engine/apps/api/tests/conftest.py | 11 +- .../api/tests/test_alert_receive_channel.py | 15 +- .../apps/slack/alert_group_slack_service.py | 10 +- engine/apps/slack/models/slack_message.py | 18 +- .../apps/slack/models/slack_user_identity.py | 5 +- .../alert_group_representative.py | 6 +- .../slack/scenarios/alertgroup_appearance.py | 5 +- .../apps/slack/scenarios/distribute_alerts.py | 211 +++++++++++------- .../slack/scenarios/notification_delivery.py | 6 +- .../apps/slack/scenarios/resolution_note.py | 31 ++- .../slack/scenarios/shift_swap_requests.py | 5 +- engine/apps/slack/tasks.py | 3 +- .../scenario_steps/test_distribute_alerts.py | 16 +- .../scenario_steps/test_resolution_note.py | 8 +- .../test_slack_channel_integration.py | 7 +- engine/conftest.py | 9 - engine/requirements.in | 1 - engine/requirements.txt | 3 - 28 files changed, 304 insertions(+), 261 deletions(-) delete mode 100644 engine/apps/alerts/tasks/compare_escalations.py diff --git a/engine/apps/alerts/models/alert_group.py b/engine/apps/alerts/models/alert_group.py index 54a5b5e204..442ab61e68 100644 --- a/engine/apps/alerts/models/alert_group.py +++ b/engine/apps/alerts/models/alert_group.py @@ -674,7 +674,7 @@ def acknowledge_by_user_or_backsync( organization_id = user.organization_id if user else self.channel.organization_id logger.debug(f"Started acknowledge_by_user_or_backsync for alert_group {self.pk}") - # if incident was silenced or resolved, unsilence/unresolve it without starting escalation + # if alert group was silenced or resolved, unsilence/unresolve it without starting escalation if self.silenced: self.un_silence() self.log_records.create( @@ -1990,6 +1990,13 @@ def slack_channel_id(self) -> str | None: @property def slack_message(self) -> typing.Optional["SlackMessage"]: + """ + `slack_message` property returns the first `SlackMessage` for the `AlertGroup`. This corresponds to the + Slack message representing the main message in Slack (ie. not a message in a thread). + + This should not be confused with `slack_messages`, which is a `RelatedManager` that returns all `SlackMessage` + instances for the `AlertGroup`. + """ try: # prefetched_slack_messages could be set in apps.api.serializers.alert_group.AlertGroupListSerializer return self.prefetched_slack_messages[0] if self.prefetched_slack_messages else None diff --git a/engine/apps/alerts/models/alert_receive_channel.py b/engine/apps/alerts/models/alert_receive_channel.py index 7a351d2aad..91a7db6a12 100644 --- a/engine/apps/alerts/models/alert_receive_channel.py +++ b/engine/apps/alerts/models/alert_receive_channel.py @@ -43,7 +43,7 @@ from apps.alerts.models import AlertGroup, ChannelFilter from apps.labels.models import AlertReceiveChannelAssociatedLabel - from apps.user_management.models import Organization, Team + from apps.user_management.models import Organization, Team, User logger = logging.getLogger(__name__) @@ -391,7 +391,7 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) - def change_team(self, team_id, user): + def change_team(self, team_id: int, user: "User") -> None: if team_id == self.team_id: raise TeamCanNotBeChangedError("Integration is already in this team") @@ -409,26 +409,26 @@ def grafana_alerting_sync_manager(self): return GrafanaAlertingSyncManager(self) @property - def is_alerting_integration(self): + def is_alerting_integration(self) -> bool: return self.integration in { AlertReceiveChannel.INTEGRATION_GRAFANA_ALERTING, AlertReceiveChannel.INTEGRATION_LEGACY_GRAFANA_ALERTING, } @cached_property - def team_name(self): + def team_name(self) -> str: return self.team.name if self.team else "No team" @cached_property - def team_id_or_no_team(self): + def team_id_or_no_team(self) -> str: return self.team_id if self.team else "no_team" @cached_property - def emojized_verbal_name(self): + def emojized_verbal_name(self) -> str: return emoji.emojize(self.verbal_name, language="alias") @property - def new_incidents_web_link(self): + def new_incidents_web_link(self) -> str: from apps.alerts.models import AlertGroup return UIURLBuilder(self.organization).alert_groups( @@ -436,7 +436,7 @@ def new_incidents_web_link(self): ) @property - def is_rate_limited_in_slack(self): + def is_rate_limited_in_slack(self) -> bool: return ( self.rate_limited_in_slack_at is not None and self.rate_limited_in_slack_at + SLACK_RATE_LIMIT_TIMEOUT > timezone.now() @@ -450,11 +450,11 @@ def start_send_rate_limit_message_task(self, delay=SLACK_RATE_LIMIT_DELAY): post_slack_rate_limit_message.apply_async((self.pk,), countdown=delay, task_id=task_id) @property - def alert_groups_count(self): + def alert_groups_count(self) -> int: return self.alert_groups.count() @property - def alerts_count(self): + def alerts_count(self) -> int: from apps.alerts.models import Alert return Alert.objects.filter(group__channel=self).count() @@ -464,7 +464,7 @@ def is_able_to_autoresolve(self) -> bool: return self.config.is_able_to_autoresolve @property - def is_demo_alert_enabled(self): + def is_demo_alert_enabled(self) -> bool: return self.config.is_demo_alert_enabled @property @@ -513,7 +513,7 @@ def get_or_create_manual_integration(cls, defaults, **kwargs): return alert_receive_channel @property - def short_name(self): + def short_name(self) -> str: if self.verbal_name is None: return self.created_name + "" if self.deleted_at is None else "(Deleted)" elif self.verbal_name == self.created_name: @@ -548,14 +548,14 @@ def integration_url(self) -> str | None: return create_engine_url(f"integrations/v1/{slug}/{self.token}/") @property - def inbound_email(self): + def inbound_email(self) -> typing.Optional[str]: if self.integration != AlertReceiveChannel.INTEGRATION_INBOUND_EMAIL: return None return f"{self.token}@{live_settings.INBOUND_EMAIL_DOMAIN}" @property - def default_channel_filter(self): + def default_channel_filter(self) -> typing.Optional["ChannelFilter"]: return self.channel_filters.filter(is_default=True).first() # Templating @@ -590,7 +590,7 @@ def templates(self): } @property - def is_available_for_custom_templates(self): + def is_available_for_custom_templates(self) -> bool: return True # Maintenance diff --git a/engine/apps/alerts/tasks/compare_escalations.py b/engine/apps/alerts/tasks/compare_escalations.py deleted file mode 100644 index 6187fa6565..0000000000 --- a/engine/apps/alerts/tasks/compare_escalations.py +++ /dev/null @@ -1,4 +0,0 @@ -def compare_escalations(request_id, active_escalation_id): - if request_id == active_escalation_id: - return True - return False diff --git a/engine/apps/alerts/tasks/escalate_alert_group.py b/engine/apps/alerts/tasks/escalate_alert_group.py index 2a98cc17e2..e47e0ca81b 100644 --- a/engine/apps/alerts/tasks/escalate_alert_group.py +++ b/engine/apps/alerts/tasks/escalate_alert_group.py @@ -6,7 +6,6 @@ from common.custom_celery_tasks import shared_dedicated_queue_retry_task -from .compare_escalations import compare_escalations from .task_logger import task_logger @@ -29,7 +28,7 @@ def escalate_alert_group(alert_group_pk): except IndexError: return f"Alert group with pk {alert_group_pk} doesn't exist" - if not compare_escalations(escalate_alert_group.request.id, alert_group.active_escalation_id): + if escalate_alert_group.request.id != alert_group.active_escalation_id: return "Active escalation ID mismatch. Duplication or non-active escalation triggered. Active: {}".format( alert_group.active_escalation_id ) diff --git a/engine/apps/alerts/tasks/notify_user.py b/engine/apps/alerts/tasks/notify_user.py index 97e3787268..9e7e9e548c 100644 --- a/engine/apps/alerts/tasks/notify_user.py +++ b/engine/apps/alerts/tasks/notify_user.py @@ -17,7 +17,6 @@ from apps.phone_notifications.phone_backend import PhoneBackend from common.custom_celery_tasks import shared_dedicated_queue_retry_task -from .compare_escalations import compare_escalations from .task_logger import task_logger if typing.TYPE_CHECKING: @@ -618,7 +617,7 @@ def send_bundled_notification(user_notification_bundle_id: int): ) return - if not compare_escalations(send_bundled_notification.request.id, user_notification_bundle.notification_task_id): + if send_bundled_notification.request.id != user_notification_bundle.notification_task_id: task_logger.info( f"send_bundled_notification: notification_task_id mismatch. " f"Duplication or non-active notification triggered. " diff --git a/engine/apps/alerts/tasks/unsilence.py b/engine/apps/alerts/tasks/unsilence.py index e970f91441..145b8e5a6d 100644 --- a/engine/apps/alerts/tasks/unsilence.py +++ b/engine/apps/alerts/tasks/unsilence.py @@ -5,7 +5,6 @@ from common.custom_celery_tasks import shared_dedicated_queue_retry_task -from .compare_escalations import compare_escalations from .send_alert_group_signal import send_alert_group_signal from .task_logger import task_logger @@ -17,17 +16,20 @@ def unsilence_task(alert_group_pk): from apps.alerts.models import AlertGroup, AlertGroupLogRecord task_logger.info(f"Start unsilence_task for alert_group {alert_group_pk}") + with transaction.atomic(): try: alert_group = AlertGroup.objects.filter(pk=alert_group_pk).select_for_update()[0] # Lock alert_group: except IndexError: task_logger.info(f"unsilence_task. alert_group {alert_group_pk} doesn't exist") return - if not compare_escalations(unsilence_task.request.id, alert_group.unsilence_task_uuid): + + if unsilence_task.request.id != alert_group.unsilence_task_uuid: task_logger.info( f"unsilence_task. alert_group {alert_group.pk}.ID mismatch.Active: {alert_group.unsilence_task_uuid}" ) return + if alert_group.status == AlertGroup.SILENCED and alert_group.is_root_alert_group: initial_state = alert_group.state task_logger.info(f"unsilence alert_group {alert_group_pk} and start escalation if needed") diff --git a/engine/apps/alerts/tests/test_alert_group.py b/engine/apps/alerts/tests/test_alert_group.py index 5bcab73b88..e837516f86 100644 --- a/engine/apps/alerts/tests/test_alert_group.py +++ b/engine/apps/alerts/tests/test_alert_group.py @@ -152,11 +152,11 @@ def test_delete( # Check that appropriate Slack API calls are made assert mock_chat_delete.call_count == 2 assert mock_chat_delete.call_args_list[0] == call( - channel=resolution_note_1.slack_channel_id, ts=resolution_note_1.ts + channel=resolution_note_1.slack_channel.slack_id, ts=resolution_note_1.ts ) assert mock_chat_delete.call_args_list[1] == call(channel=slack_message.channel.slack_id, ts=slack_message.slack_id) mock_reactions_remove.assert_called_once_with( - channel=resolution_note_2.slack_channel_id, name="memo", timestamp=resolution_note_2.ts + channel=resolution_note_2.slack_channel.slack_id, name="memo", timestamp=resolution_note_2.ts ) diff --git a/engine/apps/alerts/tests/test_maintenance.py b/engine/apps/alerts/tests/test_maintenance.py index eed1993b06..38b0f8b15f 100644 --- a/engine/apps/alerts/tests/test_maintenance.py +++ b/engine/apps/alerts/tests/test_maintenance.py @@ -16,9 +16,7 @@ def maintenance_test_setup( @pytest.mark.django_db -def test_start_maintenance_integration( - maintenance_test_setup, make_alert_receive_channel, mock_start_disable_maintenance_task -): +def test_start_maintenance_integration(maintenance_test_setup, make_alert_receive_channel): organization, user = maintenance_test_setup alert_receive_channel = make_alert_receive_channel( @@ -37,9 +35,7 @@ def test_start_maintenance_integration( @pytest.mark.django_db -def test_start_maintenance_integration_multiple_previous_instances( - maintenance_test_setup, make_alert_receive_channel, mock_start_disable_maintenance_task -): +def test_start_maintenance_integration_multiple_previous_instances(maintenance_test_setup, make_alert_receive_channel): organization, user = maintenance_test_setup alert_receive_channel = make_alert_receive_channel( @@ -64,9 +60,7 @@ def test_start_maintenance_integration_multiple_previous_instances( @pytest.mark.django_db -def test_maintenance_integration_will_not_start_twice( - maintenance_test_setup, make_alert_receive_channel, mock_start_disable_maintenance_task -): +def test_maintenance_integration_will_not_start_twice(maintenance_test_setup, make_alert_receive_channel): organization, user = maintenance_test_setup alert_receive_channel = make_alert_receive_channel( @@ -91,7 +85,6 @@ def test_alert_attached_to_maintenance_incident_integration( maintenance_test_setup, make_alert_receive_channel, make_alert_with_custom_create_method, - mock_start_disable_maintenance_task, ): organization, user = maintenance_test_setup @@ -122,7 +115,6 @@ def test_stop_maintenance( maintenance_test_setup, make_alert_receive_channel, make_alert_with_custom_create_method, - mock_start_disable_maintenance_task, ): organization, user = maintenance_test_setup alert_receive_channel = make_alert_receive_channel( diff --git a/engine/apps/alerts/tests/test_notify_user.py b/engine/apps/alerts/tests/test_notify_user.py index d40ac5e812..4a4b92a309 100644 --- a/engine/apps/alerts/tests/test_notify_user.py +++ b/engine/apps/alerts/tests/test_notify_user.py @@ -530,47 +530,73 @@ def test_send_bundle_notification( alert_group_1 = make_alert_group(alert_receive_channel=alert_receive_channel) alert_group_2 = make_alert_group(alert_receive_channel=alert_receive_channel) alert_group_3 = make_alert_group(alert_receive_channel=alert_receive_channel) + + task_id = "test_task_id" notification_bundle = make_user_notification_bundle( - user, UserNotificationPolicy.NotificationChannel.SMS, notification_task_id="test_task_id", eta=timezone.now() + user, UserNotificationPolicy.NotificationChannel.SMS, notification_task_id=task_id, eta=timezone.now() ) + notification_bundle.append_notification(alert_group_1, notification_policy) notification_bundle.append_notification(alert_group_2, notification_policy) notification_bundle.append_notification(alert_group_3, notification_policy) + assert notification_bundle.notifications.filter(bundle_uuid__isnull=True).count() == 3 + alert_group_3.resolve() - with patch("apps.alerts.tasks.notify_user.compare_escalations", return_value=True): - # send notification for 2 active alert groups - send_bundled_notification(notification_bundle.id) - assert f"alert_group {alert_group_3.id} is not active, skip notification" in caplog.text - assert "perform bundled notification for alert groups with ids:" in caplog.text - # check bundle_uuid was set, notification for resolved alert group was deleted - assert notification_bundle.notifications.filter(bundle_uuid__isnull=True).count() == 0 - assert notification_bundle.notifications.all().count() == 2 - assert not notification_bundle.notifications.filter(alert_group=alert_group_3).exists() - - # send notification for 1 active alert group - notification_bundle.notifications.update(bundle_uuid=None) - alert_group_2.resolve() - send_bundled_notification(notification_bundle.id) - assert f"alert_group {alert_group_2.id} is not active, skip notification" in caplog.text - assert ( - f"there is only one alert group in bundled notification, perform regular notification. " - f"alert_group {alert_group_1.id}" - ) in caplog.text - # check bundle_uuid was set - assert notification_bundle.notifications.filter(bundle_uuid__isnull=True).count() == 0 - assert notification_bundle.notifications.all().count() == 1 - # cleanup notifications - notification_bundle.notifications.all().delete() - - # send notification for 0 active alert group - notification_bundle.append_notification(alert_group_1, notification_policy) - alert_group_1.resolve() - send_bundled_notification(notification_bundle.id) - assert f"alert_group {alert_group_1.id} is not active, skip notification" in caplog.text - assert f"no alert groups to notify about or notification is not allowed for user {user.id}" in caplog.text - # check all notifications were deleted - assert notification_bundle.notifications.all().count() == 0 + + # send notification for 2 active alert groups + send_bundled_notification.apply((notification_bundle.id,), task_id=task_id) + + assert f"alert_group {alert_group_3.id} is not active, skip notification" in caplog.text + assert "perform bundled notification for alert groups with ids:" in caplog.text + + # check bundle_uuid was set, notification for resolved alert group was deleted + assert notification_bundle.notifications.filter(bundle_uuid__isnull=True).count() == 0 + assert notification_bundle.notifications.all().count() == 2 + assert not notification_bundle.notifications.filter(alert_group=alert_group_3).exists() + + # send notification for 1 active alert group + notification_bundle.notifications.update(bundle_uuid=None) + + # since we're calling send_bundled_notification several times within this test, we need to reset task_id + # because it gets set to None after the first call + notification_bundle.notification_task_id = task_id + notification_bundle.save() + + alert_group_2.resolve() + + send_bundled_notification.apply((notification_bundle.id,), task_id=task_id) + + assert f"alert_group {alert_group_2.id} is not active, skip notification" in caplog.text + assert ( + f"there is only one alert group in bundled notification, perform regular notification. " + f"alert_group {alert_group_1.id}" + ) in caplog.text + + # check bundle_uuid was set + assert notification_bundle.notifications.filter(bundle_uuid__isnull=True).count() == 0 + assert notification_bundle.notifications.all().count() == 1 + + # cleanup notifications + notification_bundle.notifications.all().delete() + + # send notification for 0 active alert group + notification_bundle.append_notification(alert_group_1, notification_policy) + + # since we're calling send_bundled_notification several times within this test, we need to reset task_id + # because it gets set to None after the first call + notification_bundle.notification_task_id = task_id + notification_bundle.save() + + alert_group_1.resolve() + + send_bundled_notification.apply((notification_bundle.id,), task_id=task_id) + + assert f"alert_group {alert_group_1.id} is not active, skip notification" in caplog.text + assert f"no alert groups to notify about or notification is not allowed for user {user.id}" in caplog.text + + # check all notifications were deleted + assert notification_bundle.notifications.all().count() == 0 @pytest.mark.django_db diff --git a/engine/apps/alerts/tests/test_silence.py b/engine/apps/alerts/tests/test_silence.py index d72d860212..07dcd6b238 100644 --- a/engine/apps/alerts/tests/test_silence.py +++ b/engine/apps/alerts/tests/test_silence.py @@ -5,14 +5,8 @@ @pytest.mark.django_db -def test_silence_alert_group( - make_organization_and_user, - make_alert_receive_channel, - make_alert_group, - make_alert, - mock_start_disable_maintenance_task, -): - organization, user = make_organization_and_user() +def test_silence_alert_group(make_organization_and_user, make_alert_receive_channel, make_alert_group): + organization, _ = make_organization_and_user() alert_receive_channel = make_alert_receive_channel( organization, integration=AlertReceiveChannel.INTEGRATION_GRAFANA ) @@ -24,14 +18,8 @@ def test_silence_alert_group( @pytest.mark.django_db -def test_silence_by_user_alert_group( - make_organization_and_user, - make_alert_receive_channel, - make_alert_group, - make_alert, - mock_start_disable_maintenance_task, -): - organization, user = make_organization_and_user() +def test_silence_by_user_alert_group(make_organization_and_user, make_alert_receive_channel, make_alert_group): + organization, _ = make_organization_and_user() alert_receive_channel = make_alert_receive_channel( organization, integration=AlertReceiveChannel.INTEGRATION_GRAFANA ) @@ -43,13 +31,7 @@ def test_silence_by_user_alert_group( @pytest.mark.django_db -def test_unsilence_alert_group( - make_organization_and_user, - make_alert_receive_channel, - make_alert_group, - make_alert, - mock_start_disable_maintenance_task, -): +def test_unsilence_alert_group(make_organization_and_user, make_alert_receive_channel, make_alert_group): organization, user = make_organization_and_user() alert_receive_channel = make_alert_receive_channel( organization, integration=AlertReceiveChannel.INTEGRATION_GRAFANA diff --git a/engine/apps/api/tests/conftest.py b/engine/apps/api/tests/conftest.py index cfd3ddafd0..29797876bd 100644 --- a/engine/apps/api/tests/conftest.py +++ b/engine/apps/api/tests/conftest.py @@ -2,7 +2,6 @@ from django.utils import timezone from apps.slack.client import SlackClient -from apps.slack.scenarios.distribute_alerts import AlertShootingStep @pytest.fixture() @@ -22,7 +21,7 @@ def _mock_api_call(*args, **kwargs): @pytest.fixture() -def make_resolved_ack_new_silenced_alert_groups(make_alert_group, make_alert_receive_channel, make_alert): +def make_resolved_ack_new_silenced_alert_groups(make_alert_group, make_alert): def _make_alert_groups_all_statuses(alert_receive_channel, channel_filter, alert_raw_request_data, **kwargs): resolved_alert_group = make_alert_group( alert_receive_channel, @@ -56,11 +55,3 @@ def _make_alert_groups_all_statuses(alert_receive_channel, channel_filter, alert return resolved_alert_group, ack_alert_group, new_alert_group, silenced_alert_group return _make_alert_groups_all_statuses - - -@pytest.fixture() -def mock_alert_shooting_step_post_alert_group_to_slack(monkeypatch): - def mock_post_alert_group_to_slack(*args, **kwargs): - return None - - monkeypatch.setattr(AlertShootingStep, "_post_alert_group_to_slack", mock_post_alert_group_to_slack) diff --git a/engine/apps/api/tests/test_alert_receive_channel.py b/engine/apps/api/tests/test_alert_receive_channel.py index 4569cc2ec8..14d59275e9 100644 --- a/engine/apps/api/tests/test_alert_receive_channel.py +++ b/engine/apps/api/tests/test_alert_receive_channel.py @@ -413,12 +413,7 @@ def test_update_alert_receive_channel(alert_receive_channel_internal_api_setup, @pytest.mark.django_db -def test_integration_filter_by_maintenance( - alert_receive_channel_internal_api_setup, - make_user_auth_headers, - mock_start_disable_maintenance_task, - mock_alert_shooting_step_post_alert_group_to_slack, -): +def test_integration_filter_by_maintenance(alert_receive_channel_internal_api_setup, make_user_auth_headers): user, token, alert_receive_channel = alert_receive_channel_internal_api_setup client = APIClient() mode = AlertReceiveChannel.MAINTENANCE @@ -436,12 +431,7 @@ def test_integration_filter_by_maintenance( @pytest.mark.django_db -def test_integration_filter_by_debug( - alert_receive_channel_internal_api_setup, - make_user_auth_headers, - mock_start_disable_maintenance_task, - mock_alert_shooting_step_post_alert_group_to_slack, -): +def test_integration_filter_by_debug(alert_receive_channel_internal_api_setup, make_user_auth_headers): user, token, alert_receive_channel = alert_receive_channel_internal_api_setup client = APIClient() mode = AlertReceiveChannel.DEBUG_MAINTENANCE @@ -1091,7 +1081,6 @@ def test_start_maintenance_integration( @pytest.mark.django_db def test_stop_maintenance_integration( - mock_start_disable_maintenance_task, make_user_auth_headers, make_organization_and_user_with_plugin_token, make_escalation_chain, diff --git a/engine/apps/slack/alert_group_slack_service.py b/engine/apps/slack/alert_group_slack_service.py index 12c1a1de60..6ff514040f 100644 --- a/engine/apps/slack/alert_group_slack_service.py +++ b/engine/apps/slack/alert_group_slack_service.py @@ -39,7 +39,10 @@ def update_alert_group_slack_message(self, alert_group: "AlertGroup") -> None: try: self._slack_client.chat_update( - channel=alert_group.slack_message.channel.slack_id, + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=alert_group.slack_message.channel.slack_id, + channel=alert_group.slack_message._channel_id, ts=alert_group.slack_message.slack_id, attachments=alert_group.render_slack_attachments(), blocks=alert_group.render_slack_blocks(), @@ -78,7 +81,10 @@ def publish_message_to_alert_group_thread( try: result = self._slack_client.chat_postMessage( - channel=slack_message.channel.slack_id, + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=slack_message.channel.slack_id, + channel=slack_message._channel_id, text=text, attachments=attachments, thread_ts=slack_message.slack_id, diff --git a/engine/apps/slack/models/slack_message.py b/engine/apps/slack/models/slack_message.py index 83eaafca7e..37fa6f01e8 100644 --- a/engine/apps/slack/models/slack_message.py +++ b/engine/apps/slack/models/slack_message.py @@ -70,10 +70,9 @@ class SlackMessage(models.Model): ack_reminder_message_ts = models.CharField(max_length=100, null=True, default=None) - created_at = models.DateTimeField(auto_now_add=True) - cached_permalink = models.URLField(max_length=250, null=True, default=None) + created_at = models.DateTimeField(auto_now_add=True) last_updated = models.DateTimeField(null=True, default=None) alert_group = models.ForeignKey( @@ -84,8 +83,10 @@ class SlackMessage(models.Model): related_name="slack_messages", ) - # ID of a latest celery task to update the message active_update_task_id = models.CharField(max_length=100, null=True, default=None) + """ + ID of the latest celery task to update the message + """ class Meta: # slack_id is unique within the context of a channel or conversation @@ -105,7 +106,11 @@ def permalink(self) -> typing.Optional[str]: try: result = SlackClient(self.slack_team_identity).chat_getPermalink( - channel=self.channel.slack_id, message_ts=self.slack_id + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=self.channel.slack_id, + channel=self._channel_id, + message_ts=self.slack_id, ) except SlackAPIError: return None @@ -117,7 +122,9 @@ def permalink(self) -> typing.Optional[str]: @property def deep_link(self) -> str: - return f"https://slack.com/app_redirect?channel={self.channel.slack_id}&team={self.slack_team_identity.slack_id}&message={self.slack_id}" + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + return f"https://slack.com/app_redirect?channel={self._channel_id}&team={self.slack_team_identity.slack_id}&message={self.slack_id}" @classmethod def send_slack_notification( @@ -131,7 +138,6 @@ def send_slack_notification( Still some more investigation needed to confirm this, but for now, we'll pass in the `alert_group` as an argument """ - from apps.base.models import UserNotificationPolicyLogRecord slack_message = alert_group.slack_message diff --git a/engine/apps/slack/models/slack_user_identity.py b/engine/apps/slack/models/slack_user_identity.py index 5788253634..7d04090b53 100644 --- a/engine/apps/slack/models/slack_user_identity.py +++ b/engine/apps/slack/models/slack_user_identity.py @@ -132,9 +132,12 @@ def send_link_to_slack_message(self, slack_message: "SlackMessage"): "elements": [ { "type": "mrkdwn", + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # f"<#{slack_message.channel.slack_id}>.\n" "text": ( f"You received this message because you're not a member of " - f"<#{slack_message.channel.slack_id}>.\n" + f"<#{slack_message._channel_id}>.\n" "Please join the channel to get notified right in the alert group thread." ), } diff --git a/engine/apps/slack/representatives/alert_group_representative.py b/engine/apps/slack/representatives/alert_group_representative.py index 77d995e6bc..1d9be1d568 100644 --- a/engine/apps/slack/representatives/alert_group_representative.py +++ b/engine/apps/slack/representatives/alert_group_representative.py @@ -43,8 +43,8 @@ def on_create_alert_slack_representative_async(alert_pk): logger.debug( f"Process on_create_alert_slack_representative for alert {alert_pk} from alert_group {alert.group_id}" ) - AlertShootingStep = ScenarioStep.get_step("distribute_alerts", "AlertShootingStep") - step = AlertShootingStep(organization.slack_team_identity, organization) + IncomingAlertStep = ScenarioStep.get_step("distribute_alerts", "IncomingAlertStep") + step = IncomingAlertStep(organization.slack_team_identity, organization) step.process_signal(alert) else: logger.debug( @@ -91,7 +91,7 @@ def on_alert_group_action_triggered_async(log_record_id): class AlertGroupSlackRepresentative(AlertGroupAbstractRepresentative): - def __init__(self, log_record): + def __init__(self, log_record: "AlertGroupLogRecord"): self.log_record = log_record def is_applicable(self): diff --git a/engine/apps/slack/scenarios/alertgroup_appearance.py b/engine/apps/slack/scenarios/alertgroup_appearance.py index 2f7a0113fe..1a201ecd39 100644 --- a/engine/apps/slack/scenarios/alertgroup_appearance.py +++ b/engine/apps/slack/scenarios/alertgroup_appearance.py @@ -87,7 +87,10 @@ def process_scenario( slack_message = alert_group.slack_message self._slack_client.chat_update( - channel=slack_message.channel.slack_id, + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=slack_message.channel.slack_id, + channel=slack_message._channel_id, ts=slack_message.slack_id, attachments=alert_group.render_slack_attachments(), blocks=alert_group.render_slack_blocks(), diff --git a/engine/apps/slack/scenarios/distribute_alerts.py b/engine/apps/slack/scenarios/distribute_alerts.py index 8e11fafc67..68741e5edb 100644 --- a/engine/apps/slack/scenarios/distribute_alerts.py +++ b/engine/apps/slack/scenarios/distribute_alerts.py @@ -50,69 +50,136 @@ logger.setLevel(logging.DEBUG) -class AlertShootingStep(scenario_step.ScenarioStep): +class IncomingAlertStep(scenario_step.ScenarioStep): def process_signal(self, alert: Alert) -> None: + """ + Here lay dragons. Below is some added context as to why we have the explicit + `AlertGroup.objects.filter(...).update(...)` calls. + + For example: + ``` + num_updated_rows = AlertGroup.objects.filter( + pk=alert.group.pk, slack_message_sent=False, + ).update(slack_message_sent=True) + + if num_updated_rows == 1: + # post new message + else: + # update existing message + ``` + + This piece of code guarantees that when 2 alerts are created concurrently we don't end up posting a + message twice. we rely on the fact that the `UPDATE` SQL statements are atomic. this is not the same as: + + ``` + if not alert_group.slack_message_sent: + # post new message + alert_group.slack_message_sent = True + else: + # update existing message + ``` + + This would end up posting multiple Slack messages for a single alert group (classic race condition). And then + all kinds of unexpected behaviours would arise, because some parts of the codebase assume there can only be 1 + `SlackMessage` per `AlertGroup`. + """ + + alert_group = alert.group + + if not alert_group: + # this case should hypothetically never happen, it's mostly to appease mypy with the + # fact that alert.group can "technically" be None + logger.warning( + f"Skip IncomingAlertStep.process_signal because alert {alert.pk} doesn't actually " + "have an alert group associated with it" + ) + return + + alert_group_pk = alert_group.pk + alert_receive_channel = alert_group.channel + should_skip_escalation_in_slack = alert_group.skip_escalation_in_slack + slack_team_identity = self.slack_team_identity + slack_team_identity_pk = slack_team_identity.pk + # do not try to post alert group message to slack if its channel is rate limited - if alert.group.channel.is_rate_limited_in_slack: + if alert_receive_channel.is_rate_limited_in_slack: logger.info("Skip posting or updating alert_group in Slack due to rate limit") AlertGroup.objects.filter( - pk=alert.group.pk, + pk=alert_group_pk, slack_message_sent=False, ).update(slack_message_sent=True, reason_to_skip_escalation=AlertGroup.RATE_LIMITED) return - num_updated_rows = AlertGroup.objects.filter(pk=alert.group.pk, slack_message_sent=False).update( + num_updated_rows = AlertGroup.objects.filter(pk=alert_group_pk, slack_message_sent=False).update( slack_message_sent=True ) if num_updated_rows == 1: + # this will be the case in the event that we haven't yet created a Slack message for this alert group + + slack_channel = ( + alert_group.channel_filter.slack_channel + if alert_group.channel_filter + # if channel filter is deleted mid escalation, use default Slack channel + else alert_receive_channel.organization.default_slack_channel + ) + slack_channel_id = slack_channel.slack_id + try: - slack_channel = ( - alert.group.channel_filter.slack_channel - if alert.group.channel_filter - # if channel filter is deleted mid escalation, use default Slack channel - else alert.group.channel.organization.default_slack_channel + self._post_alert_group_to_slack( + slack_team_identity=slack_team_identity, + alert_group=alert_group, + alert=alert, + attachments=alert_group.render_slack_attachments(), + slack_channel=slack_channel, + blocks=alert_group.render_slack_blocks(), ) - self._send_first_alert(alert, slack_channel) except (SlackAPIError, TimeoutError): - AlertGroup.objects.filter(pk=alert.group.pk).update(slack_message_sent=False) + AlertGroup.objects.filter(pk=alert_group_pk).update(slack_message_sent=False) raise - if alert.group.channel.maintenance_mode == AlertReceiveChannel.DEBUG_MAINTENANCE: - self._send_debug_mode_notice(alert.group, slack_channel) + if alert_receive_channel.maintenance_mode == AlertReceiveChannel.DEBUG_MAINTENANCE: + # send debug mode notice + text = "Escalations are silenced due to Debug mode" + self._slack_client.chat_postMessage( + channel=slack_channel_id, + text=text, + attachments=[], + thread_ts=alert_group.slack_message.slack_id, + mrkdwn=True, + blocks=[ + { + "type": "section", + "text": { + "type": "mrkdwn", + "text": text, + }, + }, + ], + ) + + if not alert_group.is_maintenance_incident and not should_skip_escalation_in_slack: + send_message_to_thread_if_bot_not_in_channel.apply_async( + (alert_group_pk, slack_team_identity_pk, slack_channel_id), + countdown=1, # delay for message so that the log report is published first + ) - if alert.group.is_maintenance_incident: - # not sending log report message for maintenance incident - pass - else: - # check if alert group was posted to slack before posting message to thread - if not alert.group.skip_escalation_in_slack: - self._send_message_to_thread_if_bot_not_in_channel(alert.group, slack_channel) else: - # check if alert group was posted to slack before updating its message - if not alert.group.skip_escalation_in_slack: + # if a new alert comes in, and is grouped to an alert group that has already been posted to Slack, + # then we will update that existing Slack message + if not should_skip_escalation_in_slack: update_task_id = update_incident_slack_message.apply_async( - (self.slack_team_identity.pk, alert.group.pk), + (self.slack_team_identity.pk, alert_group_pk), countdown=10, ) cache.set( - get_cache_key_update_incident_slack_message(alert.group.pk), + get_cache_key_update_incident_slack_message(alert_group_pk), update_task_id, timeout=CACHE_UPDATE_INCIDENT_SLACK_MESSAGE_LIFETIME, ) else: logger.info("Skip updating alert_group in Slack due to rate limit") - def _send_first_alert(self, alert: Alert, slack_channel: typing.Optional[SlackChannel]) -> None: - self._post_alert_group_to_slack( - slack_team_identity=self.slack_team_identity, - alert_group=alert.group, - alert=alert, - attachments=alert.group.render_slack_attachments(), - slack_channel=slack_channel, - blocks=alert.group.render_slack_blocks(), - ) - def _post_alert_group_to_slack( self, slack_team_identity: SlackTeamIdentity, @@ -182,31 +249,6 @@ def _post_alert_group_to_slack( finally: alert.save() - def _send_debug_mode_notice(self, alert_group: AlertGroup, slack_channel: SlackChannel) -> None: - blocks: Block.AnyBlocks = [] - - text = "Escalations are silenced due to Debug mode" - blocks.append({"type": "section", "text": {"type": "mrkdwn", "text": text}}) - - self._slack_client.chat_postMessage( - channel=slack_channel.slack_id, - text=text, - attachments=[], - thread_ts=alert_group.slack_message.slack_id, - mrkdwn=True, - blocks=blocks, - ) - - def _send_message_to_thread_if_bot_not_in_channel( - self, - alert_group: AlertGroup, - slack_channel: SlackChannel, - ) -> None: - send_message_to_thread_if_bot_not_in_channel.apply_async( - (alert_group.pk, self.slack_team_identity.pk, slack_channel.slack_id), - countdown=1, # delay for message so that the log report is published first - ) - def process_scenario( self, slack_user_identity: SlackUserIdentity, @@ -472,8 +514,11 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: if slack_user_identity: self._slack_client.chat_postEphemeral( + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=alert_group.slack_message.channel.slack_id, + channel=alert_group.slack_message._channel_id, user=slack_user_identity.slack_id, - channel=alert_group.slack_message.channel.slack_id, text="{}{}".format(ephemeral_text[:1].upper(), ephemeral_text[1:]), unfurl_links=True, ) @@ -714,7 +759,10 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: if slack_message.ack_reminder_message_ts: try: self._slack_client.chat_update( - channel=slack_message.channel.slack_id, + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=slack_message.channel.slack_id, + channel=slack_message._channel_id, ts=slack_message.ack_reminder_message_ts, text=text, attachments=message_attachments, @@ -788,12 +836,14 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: from apps.user_management.models import Organization alert_group = log_record.alert_group - slack_channel = alert_group.slack_message.channel + organization = alert_group.channel.organization + slack_message = alert_group.slack_message + slack_channel = slack_message.channel user_verbal = log_record.author.get_username_with_slack_verbal(mention=True) text = f"{user_verbal}, please confirm that you're still working on this Alert Group." - if alert_group.channel.organization.unacknowledge_timeout != Organization.UNACKNOWLEDGE_TIMEOUT_NEVER: + if organization.unacknowledge_timeout != Organization.UNACKNOWLEDGE_TIMEOUT_NEVER: try: response = self._slack_client.chat_postMessage( channel=slack_channel.slack_id, @@ -815,14 +865,12 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: "text": "Confirm", "type": "button", "style": "primary", - "value": make_value( - {"alert_group_pk": alert_group.pk}, alert_group.channel.organization - ), + "value": make_value({"alert_group_pk": alert_group.pk}, organization), }, ], } ], - thread_ts=alert_group.slack_message.slack_id, + thread_ts=slack_message.slack_id, ) except (SlackAPITokenError, SlackAPIChannelArchivedError, SlackAPIChannelNotFoundError): pass @@ -831,13 +879,13 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p1732555465144099 alert_group.slack_messages.create( slack_id=response["ts"], - organization=alert_group.channel.organization, + organization=organization, _channel_id=slack_channel.slack_id, channel=slack_channel, ) - alert_group.slack_message.ack_reminder_message_ts = response["ts"] - alert_group.slack_message.save(update_fields=["ack_reminder_message_ts"]) + slack_message.ack_reminder_message_ts = response["ts"] + slack_message.save(update_fields=["ack_reminder_message_ts"]) else: text = f"This is a reminder that the Alert Group is still acknowledged by {user_verbal}" self.alert_group_slack_service.publish_message_to_alert_group_thread(alert_group, text=text) @@ -846,9 +894,12 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: class WipeGroupStep(scenario_step.ScenarioStep): def process_signal(self, log_record: AlertGroupLogRecord) -> None: alert_group = log_record.alert_group - user_verbal = log_record.author.get_username_with_slack_verbal() - text = f"Wiped by {user_verbal}" - self.alert_group_slack_service.publish_message_to_alert_group_thread(alert_group, text=text) + + self.alert_group_slack_service.publish_message_to_alert_group_thread( + alert_group, + text=f"Wiped by {log_record.author.get_username_with_slack_verbal()}", + ) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) @@ -859,7 +910,9 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: # Remove "memo" emoji from resolution note messages for message in alert_group.resolution_note_slack_messages.filter(added_to_resolution_note=True): try: - self._slack_client.reactions_remove(channel=message.slack_channel_id, name="memo", timestamp=message.ts) + self._slack_client.reactions_remove( + channel=message.slack_channel_slack_id, name="memo", timestamp=message.ts + ) except SlackAPIRatelimitError: # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group raise @@ -870,7 +923,7 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: # Remove resolution note messages posted by OnCall bot for message in alert_group.resolution_note_slack_messages.filter(posted_by_bot=True): try: - self._slack_client.chat_delete(channel=message.slack_channel_id, ts=message.ts) + self._slack_client.chat_delete(channel=message.slack_channel_slack_id, ts=message.ts) except SlackAPIRatelimitError: # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group raise @@ -881,7 +934,13 @@ def process_signal(self, log_record: AlertGroupLogRecord) -> None: # Remove alert group Slack messages for message in alert_group.slack_messages.all(): try: - self._slack_client.chat_delete(channel=message.channel.slack_id, ts=message.slack_id) + self._slack_client.chat_delete( + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # channel=message.channel.slack_id, + channel=message._channel_id, + ts=message.slack_id, + ) except SlackAPIRatelimitError: # retries on ratelimit are handled in apps.alerts.tasks.delete_alert_group.delete_alert_group raise diff --git a/engine/apps/slack/scenarios/notification_delivery.py b/engine/apps/slack/scenarios/notification_delivery.py index d77286ad62..fc3992311d 100644 --- a/engine/apps/slack/scenarios/notification_delivery.py +++ b/engine/apps/slack/scenarios/notification_delivery.py @@ -18,7 +18,11 @@ def process_signal(self, log_record: "AlertGroupLogRecord") -> None: user = log_record.author alert_group = log_record.alert_group - slack_channel_id = alert_group.slack_message.channel.slack_id + + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # slack_channel_id = alert_group_slack_message.channel.slack_id + slack_channel_id = alert_group.slack_message._channel_id user_verbal_with_mention = user.get_username_with_slack_verbal(mention=True) diff --git a/engine/apps/slack/scenarios/resolution_note.py b/engine/apps/slack/scenarios/resolution_note.py index 046967b175..130a102f85 100644 --- a/engine/apps/slack/scenarios/resolution_note.py +++ b/engine/apps/slack/scenarios/resolution_note.py @@ -236,9 +236,7 @@ def process_signal(self, alert_group: "AlertGroup", resolution_note: "Resolution else: self.post_or_update_resolution_note_in_thread(resolution_note) - self.update_alert_group_resolution_note_button( - alert_group=alert_group, - ) + self.update_alert_group_resolution_note_button(alert_group) def remove_resolution_note_slack_message(self, resolution_note: "ResolutionNote") -> None: if (resolution_note_slack_message := resolution_note.resolution_note_slack_message) is not None: @@ -263,9 +261,13 @@ def post_or_update_resolution_note_in_thread(self, resolution_note: "ResolutionN resolution_note_slack_message = resolution_note.resolution_note_slack_message alert_group = resolution_note.alert_group alert_group_slack_message = alert_group.slack_message - slack_channel_id = alert_group_slack_message.channel.slack_id blocks = self.get_resolution_note_blocks(resolution_note) + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 + # slack_channel_id = alert_group_slack_message.channel.slack_id + slack_channel_id = alert_group_slack_message._channel_id + slack_channel = SlackChannel.objects.get( slack_id=slack_channel_id, slack_team_identity=self.slack_team_identity ) @@ -655,11 +657,6 @@ def get_invite_bot_tip_blocks(self, channel: str) -> Block.AnyBlocks: ] -class ReadEditPostmortemStep(ResolutionNoteModalStep): - # Left for backward compatibility with slack messages created before postmortems -> resolution note change - pass - - class AddRemoveThreadMessageStep(UpdateResolutionNoteStep, scenario_step.ScenarioStep): def process_scenario( self, @@ -687,6 +684,7 @@ def process_scenario( if add_to_resolution_note and slack_thread_message is not None: slack_thread_message.added_to_resolution_note = True slack_thread_message.save(update_fields=["added_to_resolution_note"]) + if resolution_note is None: ResolutionNote( alert_group=alert_group, @@ -696,6 +694,7 @@ def process_scenario( ).save() else: resolution_note.recreate() + self.add_resolution_note_reaction(slack_thread_message) elif not add_to_resolution_note: # Check if resolution_note can be removed @@ -716,14 +715,16 @@ def process_scenario( else: if resolution_note_pk is not None and resolution_note is None: # old version of step resolution_note = ResolutionNote.objects.get(pk=resolution_note_pk) + resolution_note.delete() + if slack_thread_message: slack_thread_message.added_to_resolution_note = False slack_thread_message.save(update_fields=["added_to_resolution_note"]) self.remove_resolution_note_reaction(slack_thread_message) - self.update_alert_group_resolution_note_button( - alert_group, - ) + + self.update_alert_group_resolution_note_button(alert_group) + resolution_note_data = json.loads(payload["actions"][0]["value"]) resolution_note_data["resolution_note_window_action"] = "edit_update" ResolutionNoteModalStep(slack_team_identity, self.organization, self.user).process_scenario( @@ -735,12 +736,6 @@ def process_scenario( STEPS_ROUTING: ScenarioRoute.RoutingSteps = [ - { - "payload_type": PayloadType.BLOCK_ACTIONS, - "block_action_type": BlockActionType.BUTTON, - "block_action_id": ReadEditPostmortemStep.routing_uid(), - "step": ReadEditPostmortemStep, - }, { "payload_type": PayloadType.BLOCK_ACTIONS, "block_action_type": BlockActionType.BUTTON, diff --git a/engine/apps/slack/scenarios/shift_swap_requests.py b/engine/apps/slack/scenarios/shift_swap_requests.py index 33c29ea920..48637131b7 100644 --- a/engine/apps/slack/scenarios/shift_swap_requests.py +++ b/engine/apps/slack/scenarios/shift_swap_requests.py @@ -186,8 +186,11 @@ def post_message_to_thread( if not shift_swap_request.slack_message: return + # TODO: once _channel_id has been fully migrated to channel, remove _channel_id + # see https://raintank-corp.slack.com/archives/C06K1MQ07GS/p173255546 self._slack_client.chat_postMessage( - channel=shift_swap_request.slack_message.channel.slack_id, + # channel=shift_swap_request.slack_message.channel.slack_id, + channel=shift_swap_request.slack_message._channel_id, thread_ts=shift_swap_request.slack_message.slack_id, reply_broadcast=reply_broadcast, blocks=blocks, diff --git a/engine/apps/slack/tasks.py b/engine/apps/slack/tasks.py index 84e5ca7653..13b26bbeda 100644 --- a/engine/apps/slack/tasks.py +++ b/engine/apps/slack/tasks.py @@ -9,7 +9,6 @@ from django.core.cache import cache from django.utils import timezone -from apps.alerts.tasks.compare_escalations import compare_escalations from apps.slack.alert_group_slack_service import AlertGroupSlackService from apps.slack.client import SlackClient from apps.slack.constants import CACHE_UPDATE_INCIDENT_SLACK_MESSAGE_LIFETIME, SLACK_BOT_ID @@ -296,7 +295,7 @@ def post_slack_rate_limit_message(integration_id): logger.warning(f"AlertReceiveChannel {integration_id} doesn't exist") return - if not compare_escalations(post_slack_rate_limit_message.request.id, integration.rate_limit_message_task_id): + if post_slack_rate_limit_message.request.id != integration.rate_limit_message_task_id: logger.info( f"post_slack_rate_limit_message. integration {integration_id}. ID mismatch. " f"Active: {integration.rate_limit_message_task_id}" diff --git a/engine/apps/slack/tests/scenario_steps/test_distribute_alerts.py b/engine/apps/slack/tests/scenario_steps/test_distribute_alerts.py index 4844121027..12943f9682 100644 --- a/engine/apps/slack/tests/scenario_steps/test_distribute_alerts.py +++ b/engine/apps/slack/tests/scenario_steps/test_distribute_alerts.py @@ -5,7 +5,7 @@ from apps.alerts.models import AlertGroup from apps.slack.errors import get_error_class from apps.slack.models import SlackMessage -from apps.slack.scenarios.distribute_alerts import AlertShootingStep +from apps.slack.scenarios.distribute_alerts import IncomingAlertStep from apps.slack.scenarios.scenario_step import ScenarioStep from apps.slack.tests.conftest import build_slack_response @@ -28,7 +28,7 @@ def test_skip_escalations_error( reason, slack_error, ): - SlackAlertShootingStep = ScenarioStep.get_step("distribute_alerts", "AlertShootingStep") + SlackIncomingAlertStep = ScenarioStep.get_step("distribute_alerts", "IncomingAlertStep") organization, _, slack_team_identity, _ = make_organization_and_user_with_slack_identities() alert_receive_channel = make_alert_receive_channel(organization) alert_group = make_alert_group(alert_receive_channel) @@ -36,7 +36,7 @@ def test_skip_escalations_error( slack_channel = make_slack_channel(slack_team_identity) - step = SlackAlertShootingStep(slack_team_identity) + step = SlackIncomingAlertStep(slack_team_identity) with patch.object(step._slack_client, "api_call") as mock_slack_api_call: error_response = build_slack_response({"error": slack_error}) @@ -66,7 +66,7 @@ def test_timeout_error( make_alert_group, make_alert, ): - SlackAlertShootingStep = ScenarioStep.get_step("distribute_alerts", "AlertShootingStep") + SlackIncomingAlertStep = ScenarioStep.get_step("distribute_alerts", "IncomingAlertStep") slack_team_identity = make_slack_team_identity() slack_channel = make_slack_channel(slack_team_identity) organization = make_organization(slack_team_identity=slack_team_identity, default_slack_channel=slack_channel) @@ -74,7 +74,7 @@ def test_timeout_error( alert_group = make_alert_group(alert_receive_channel) alert = make_alert(alert_group, raw_request_data="{}") - step = SlackAlertShootingStep(slack_team_identity) + step = SlackIncomingAlertStep(slack_team_identity) with pytest.raises(TimeoutError): with patch.object(step._slack_client, "api_call") as mock_slack_api_call: @@ -89,9 +89,9 @@ def test_timeout_error( assert not alert.delivered -@patch.object(AlertShootingStep, "_post_alert_group_to_slack") +@patch.object(IncomingAlertStep, "_post_alert_group_to_slack") @pytest.mark.django_db -def test_alert_shooting_no_channel_filter( +def test_incoming_alert_no_channel_filter( mock_post_alert_group_to_slack, make_slack_team_identity, make_slack_channel, @@ -109,7 +109,7 @@ def test_alert_shooting_no_channel_filter( alert_group = make_alert_group(alert_receive_channel, channel_filter=None) alert = make_alert(alert_group, raw_request_data={}) - step = AlertShootingStep(slack_team_identity, organization) + step = IncomingAlertStep(slack_team_identity, organization) step.process_signal(alert) assert mock_post_alert_group_to_slack.call_args[1]["slack_channel"] == slack_channel diff --git a/engine/apps/slack/tests/scenario_steps/test_resolution_note.py b/engine/apps/slack/tests/scenario_steps/test_resolution_note.py index 6fc62816ae..0f1598041c 100644 --- a/engine/apps/slack/tests/scenario_steps/test_resolution_note.py +++ b/engine/apps/slack/tests/scenario_steps/test_resolution_note.py @@ -347,17 +347,18 @@ def test_resolution_notes_modal_closed_before_update( assert call_args[0] == "views.update" +@patch.object(SlackClient, "reactions_add") @patch.object(SlackClient, "chat_getPermalink", return_value={"permalink": "https://example.com"}) @pytest.mark.django_db def test_add_to_resolution_note( - _, + _mock_chat_getPermalink, + mock_reactions_add, make_organization_and_user_with_slack_identities, make_alert_receive_channel, make_alert_group, make_alert, make_slack_message, make_slack_channel, - settings, ): organization, user, slack_team_identity, slack_user_identity = make_organization_and_user_with_slack_identities() alert_receive_channel = make_alert_receive_channel(organization) @@ -382,8 +383,7 @@ def test_add_to_resolution_note( AddToResolutionNoteStep = ScenarioStep.get_step("resolution_note", "AddToResolutionNoteStep") step = AddToResolutionNoteStep(organization=organization, user=user, slack_team_identity=slack_team_identity) - with patch.object(SlackClient, "reactions_add") as mock_reactions_add: - step.process_scenario(slack_user_identity, slack_team_identity, payload) + step.process_scenario(slack_user_identity, slack_team_identity, payload) mock_reactions_add.assert_called_once() assert alert_group.resolution_notes.get().text == "Test resolution note" diff --git a/engine/apps/slack/tests/scenario_steps/test_slack_channel_integration.py b/engine/apps/slack/tests/scenario_steps/test_slack_channel_integration.py index 45384f5316..993692d6bc 100644 --- a/engine/apps/slack/tests/scenario_steps/test_slack_channel_integration.py +++ b/engine/apps/slack/tests/scenario_steps/test_slack_channel_integration.py @@ -386,12 +386,7 @@ def test_save_thread_message_for_resolution_note( def test_delete_thread_message_from_resolution_note_no_slack_user_identity( self, MockResolutionNoteSlackMessage, make_organization_and_user_with_slack_identities ) -> None: - ( - organization, - user, - slack_team_identity, - slack_user_identity, - ) = make_organization_and_user_with_slack_identities() + organization, user, slack_team_identity, _ = make_organization_and_user_with_slack_identities() step = SlackChannelMessageEventStep(slack_team_identity, organization, user) step.delete_thread_message_from_resolution_note(None, {}) diff --git a/engine/conftest.py b/engine/conftest.py index 7ef2555835..bcd7c47cc7 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -19,7 +19,6 @@ Alert, AlertGroupLogRecord, AlertReceiveChannel, - MaintainableObject, ResolutionNote, listen_for_alertgrouplogrecord, listen_for_alertreceivechannel_model_save, @@ -825,14 +824,6 @@ def _make_slack_channel(slack_team_identity, **kwargs): return _make_slack_channel -@pytest.fixture() -def mock_start_disable_maintenance_task(monkeypatch): - def mocked_start_disable_maintenance_task(*args, **kwargs): - return uuid.uuid4() - - monkeypatch.setattr(MaintainableObject, "start_disable_maintenance_task", mocked_start_disable_maintenance_task) - - @pytest.fixture() def make_organization_and_user_with_plugin_token(make_organization_and_user, make_token_for_organization): def _make_organization_and_user_with_plugin_token(role: typing.Optional[LegacyAccessControlRole] = None): diff --git a/engine/requirements.in b/engine/requirements.in index 4ce7179e79..1ecf5d5195 100644 --- a/engine/requirements.in +++ b/engine/requirements.in @@ -9,7 +9,6 @@ django-cors-headers==3.7.0 # pyroscope-io==0.8.1 django-dbconn-retry==0.1.7 django-debug-toolbar==4.1 -django-deprecate-fields==0.1.1 django-filter==2.4.0 django-ipware==4.0.2 django-log-request-id==1.6.0 diff --git a/engine/requirements.txt b/engine/requirements.txt index 5c60801e2e..7ac3a6ea28 100644 --- a/engine/requirements.txt +++ b/engine/requirements.txt @@ -82,7 +82,6 @@ django==4.2.16 # django-anymail # django-cors-headers # django-debug-toolbar - # django-deprecate-fields # django-filter # django-log-request-id # django-migration-linter @@ -106,8 +105,6 @@ django-dbconn-retry==0.1.7 # via -r requirements.in django-debug-toolbar==4.1.0 # via -r requirements.in -django-deprecate-fields==0.1.1 - # via -r requirements.in django-filter==2.4.0 # via -r requirements.in django-ipware==4.0.2 From ce34e1a1dc72d1fb32b603c4a95997dbc9d61bac Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 28 Nov 2024 10:50:58 -0500 Subject: [PATCH 2/3] update comment --- engine/apps/slack/scenarios/distribute_alerts.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/engine/apps/slack/scenarios/distribute_alerts.py b/engine/apps/slack/scenarios/distribute_alerts.py index 68741e5edb..a08d396577 100644 --- a/engine/apps/slack/scenarios/distribute_alerts.py +++ b/engine/apps/slack/scenarios/distribute_alerts.py @@ -53,8 +53,9 @@ class IncomingAlertStep(scenario_step.ScenarioStep): def process_signal(self, alert: Alert) -> None: """ - Here lay dragons. Below is some added context as to why we have the explicit - `AlertGroup.objects.filter(...).update(...)` calls. + 🐉 Here lay dragons 🐉 + + Below is some added context as to why we have the explicit `AlertGroup.objects.filter(...).update(...)` calls. For example: ``` @@ -82,6 +83,8 @@ def process_signal(self, alert: Alert) -> None: This would end up posting multiple Slack messages for a single alert group (classic race condition). And then all kinds of unexpected behaviours would arise, because some parts of the codebase assume there can only be 1 `SlackMessage` per `AlertGroup`. + + See this conversation for more context https://raintank-corp.slack.com/archives/C06K1MQ07GS/p1732800180834819?thread_ts=1732748893.183939&cid=C06K1MQ07GS """ alert_group = alert.group From 11f44a9d03dc86499018d55452ae476aea699738 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 28 Nov 2024 10:59:55 -0500 Subject: [PATCH 3/3] update comment --- engine/apps/slack/scenarios/distribute_alerts.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/engine/apps/slack/scenarios/distribute_alerts.py b/engine/apps/slack/scenarios/distribute_alerts.py index a08d396577..ad327bdcd8 100644 --- a/engine/apps/slack/scenarios/distribute_alerts.py +++ b/engine/apps/slack/scenarios/distribute_alerts.py @@ -70,7 +70,7 @@ def process_signal(self, alert: Alert) -> None: ``` This piece of code guarantees that when 2 alerts are created concurrently we don't end up posting a - message twice. we rely on the fact that the `UPDATE` SQL statements are atomic. this is not the same as: + message twice. We rely on the fact that the `UPDATE` SQL statements are atomic. This is NOT the same as: ``` if not alert_group.slack_message_sent: @@ -84,6 +84,14 @@ def process_signal(self, alert: Alert) -> None: all kinds of unexpected behaviours would arise, because some parts of the codebase assume there can only be 1 `SlackMessage` per `AlertGroup`. + ``` + AlertGroup.objects.filter(pk=alert.group.pk, slack_message_sent=False).update(slack_message_sent=True) + ``` + + The power of this 👆, is that it doesn't actually do `SELECT ...;` and then `UPDATE ...;` it actually does a + single `UPDATE alerts.alert_group WHERE id= AND slack_message_sent IS FALSE`; + which makes it atomic and concurrency-safe. + See this conversation for more context https://raintank-corp.slack.com/archives/C06K1MQ07GS/p1732800180834819?thread_ts=1732748893.183939&cid=C06K1MQ07GS """