Improve Slack error handling (#3000)

# What this PR does - Rename `SlackClientWithErrorHandling` to just `SlackClient` - Add more error classes + improve the way errors are raised based on the Slack error code - Add API call retries on Slack server errors (e.g. when Slack returns `5xx` errors) - Refactor some methods working with Slack API + add tests ## Which issue(s) this PR fixes - grafana/oncall-private#1837 - grafana/oncall-private#1840 - grafana/oncall-private#1842 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
grafana · Sep 12, 2023 · 8b2212c · 8b2212c
1 parent 14c32a7
commit 8b2212c
Show file tree

Hide file tree

Showing 35 changed files with 930 additions and 728 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Notify user via Slack/mobile push-notification when their shift swap request is taken by @joeyorlando ([#2992](https://github.com/grafana/oncall/pull/2992))
 
+### Changed
+
+- Improve Slack error handling by @vadimkerr ([#3000](https://github.com/grafana/oncall/pull/3000))
+
 ### Fixed
 
 - Avoid task retries because of missing AlertGroupLogRecord on send_alert_group_signal ([#3001](https://github.com/grafana/oncall/pull/3001))

diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py
@@ -6,7 +6,13 @@
 from django.utils import timezone
 
 from apps.schedules.ical_utils import calculate_shift_diff, parse_event_uid
-from apps.slack.client import SlackAPIException, SlackAPITokenException, SlackClientWithErrorHandling
+from apps.slack.client import SlackClient
+from apps.slack.errors import (
+    SlackAPIChannelArchivedError,
+    SlackAPIChannelNotFoundError,
+    SlackAPIInvalidAuthError,
+    SlackAPITokenError,
+)
 from apps.slack.scenarios import scenario_step
 from common.custom_celery_tasks import shared_dedicated_queue_retry_task
 
@@ -146,7 +152,7 @@ def notify_ical_schedule_shift(schedule_pk):
     if len(new_shifts) > 0 or schedule.empty_oncall:
         task_logger.info(f"new_shifts: {new_shifts}")
         if schedule.notify_oncall_shift_freq != OnCallSchedule.NotifyOnCallShiftFreq.NEVER:
-            slack_client = SlackClientWithErrorHandling(schedule.organization.slack_team_identity.bot_access_token)
+            slack_client = SlackClient(schedule.organization.slack_team_identity)
             step = scenario_step.ScenarioStep.get_step("schedules", "EditScheduleShiftNotifyStep")
             report_blocks = step.get_report_blocks_ical(new_shifts, upcoming_shifts, schedule, schedule.empty_oncall)
 
@@ -156,11 +162,10 @@ def notify_ical_schedule_shift(schedule_pk):
                     blocks=report_blocks,
                     text=f"On-call shift for schedule {schedule.name} has changed",
                 )
-            except SlackAPITokenException:
+            except (
+                SlackAPITokenError,
+                SlackAPIChannelNotFoundError,
+                SlackAPIChannelArchivedError,
+                SlackAPIInvalidAuthError,
+            ):
                 pass
-            except SlackAPIException as e:
-                expected_exceptions = ["channel_not_found", "is_archived", "invalid_auth"]
-                if e.response["error"] in expected_exceptions:
-                    print(e)
-                else:
-                    raise e
diff --git a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py
@@ -100,7 +100,7 @@ def test_next_shift_notification_long_shifts(
 
     with patch("apps.alerts.tasks.notify_ical_schedule_shift.datetime", Mock(wraps=datetime)) as mock_datetime:
         mock_datetime.datetime.now.return_value = datetime.datetime(2021, 9, 29, 12, 0, tzinfo=pytz.UTC)
-        with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+        with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
             notify_ical_schedule_shift(ical_schedule.pk)
 
     slack_blocks = mock_slack_api_call.call_args_list[0][1]["blocks"]
@@ -204,7 +204,7 @@ def test_overrides_changes_no_current_no_triggering_notification(
     schedule.prev_ical_file_overrides = ical_before
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -252,7 +252,7 @@ def test_no_changes_no_triggering_notification(
     schedule.empty_oncall = False
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -300,7 +300,7 @@ def test_current_shift_changes_trigger_notification(
     schedule.empty_oncall = False
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert mock_slack_api_call.called
@@ -364,7 +364,7 @@ def test_current_shift_changes_swap_split(
     schedule.empty_oncall = False
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     text_block = mock_slack_api_call.call_args_list[0][1]["blocks"][1]["text"]["text"]
@@ -433,7 +433,7 @@ def test_next_shift_changes_no_triggering_notification(
     on_call_shift_2.add_rolling_users([[user2]])
     schedule.refresh_ical_file()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -500,7 +500,7 @@ def test_lower_priority_changes_no_triggering_notification(
     on_call_shift_2.add_rolling_users([[user2]])
     schedule.refresh_ical_file()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -630,7 +630,7 @@ def test_vtimezone_changes_no_triggering_notification(
     schedule.cached_ical_file_primary = ical_after
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -687,7 +687,7 @@ def test_no_changes_no_triggering_notification_from_old_to_new_task_version(
     schedule.empty_oncall = False
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -749,7 +749,7 @@ def test_current_shift_changes_trigger_notification_from_old_to_new_task_version
     on_call_shift.add_rolling_users([[user2]])
     schedule.refresh_ical_file()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert mock_slack_api_call.called
@@ -814,7 +814,7 @@ def test_next_shift_notification_long_and_short_shifts(
     schedule.empty_oncall = False
     schedule.save()
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_ical_schedule_shift(schedule.pk)
 
     assert mock_slack_api_call.called

diff --git a/engine/apps/api/tests/conftest.py b/engine/apps/api/tests/conftest.py
@@ -3,7 +3,7 @@
 import pytest
 from django.utils import timezone
 
-from apps.slack.client import SlackClientWithErrorHandling
+from apps.slack.client import SlackClient
 from apps.slack.scenarios.distribute_alerts import AlertShootingStep
 
 
@@ -20,7 +20,7 @@ def _mock_api_call(*args, **kwargs):
             "team": {"name": "TEST_SLACK_TEAM_NAME"},
         }
 
-    monkeypatch.setattr(SlackClientWithErrorHandling, "api_call", _mock_api_call)
+    monkeypatch.setattr(SlackClient, "api_call", _mock_api_call)
 
 
 @pytest.fixture()

diff --git a/engine/apps/integrations/tasks.py b/engine/apps/integrations/tasks.py
@@ -8,7 +8,8 @@
 
 from apps.alerts.models.alert_group_counter import ConcurrentUpdateError
 from apps.alerts.tasks import resolve_alert_group_by_source_if_needed
-from apps.slack.client import SlackAPIException, SlackClientWithErrorHandling
+from apps.slack.client import SlackClient
+from apps.slack.errors import SlackAPIError
 from common.custom_celery_tasks import shared_dedicated_queue_retry_task
 from common.custom_celery_tasks.create_alert_base_task import CreateAlertBaseTask
 
@@ -157,7 +158,7 @@ def notify_about_integration_ratelimit_in_slack(organization_id, text, **kwargs)
         slack_team_identity = organization.slack_team_identity
         if slack_team_identity is not None:
             try:
-                sc = SlackClientWithErrorHandling(slack_team_identity.bot_access_token)
+                sc = SlackClient(slack_team_identity)
                 sc.chat_postMessage(channel=organization.general_log_channel_id, text=text, team=slack_team_identity)
-            except SlackAPIException as e:
+            except SlackAPIError as e:
                 logger.warning(f"Slack exception {e} while sending message for organization {organization_id}")
diff --git a/engine/apps/public_api/helpers.py b/engine/apps/public_api/helpers.py
@@ -1,13 +1,14 @@
 from apps.public_api.constants import VALID_DATE_FOR_DELETE_INCIDENT
-from apps.slack.client import SlackAPITokenException, SlackClientWithErrorHandling
+from apps.slack.client import SlackClient
+from apps.slack.errors import SlackAPITokenError
 
 
 def team_has_slack_token_for_deleting(alert_group):
     if alert_group.slack_message and alert_group.slack_message.slack_team_identity:
-        sc = SlackClientWithErrorHandling(alert_group.slack_message.slack_team_identity.bot_access_token)
+        sc = SlackClient(alert_group.slack_message.slack_team_identity)
         try:
             sc.auth_test()
-        except SlackAPITokenException:
+        except SlackAPITokenError:
             return False
     return True
 

diff --git a/engine/apps/schedules/tests/tasks/shift_swaps/test_slack_followups.py b/engine/apps/schedules/tests/tasks/shift_swaps/test_slack_followups.py
@@ -154,7 +154,7 @@ def test_followup_offsets():
         assert ShiftSwapRequest.FOLLOWUP_OFFSETS[idx] > FOLLOWUP_WINDOW
 
 
-@patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage")
+@patch("apps.slack.client.SlackClient.chat_postMessage")
 @pytest.mark.django_db
 def test_send_shift_swap_request_followup(mock_slack_chat_post_message, shift_swap_request_test_setup):
     shift_swap_request = shift_swap_request_test_setup()

diff --git a/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py b/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py
@@ -49,7 +49,7 @@ def test_no_empty_shifts_no_triggering_notification(
 
     empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_empty_shifts_in_schedule(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -97,7 +97,7 @@ def test_empty_shifts_trigger_notification(
 
     empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_empty_shifts_in_schedule(schedule.pk)
 
     assert mock_slack_api_call.called
@@ -160,7 +160,7 @@ def test_empty_non_empty_shifts_trigger_notification(
 
     empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_empty_shifts_in_schedule(schedule.pk)
 
     assert mock_slack_api_call.called

diff --git a/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py b/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py
@@ -48,7 +48,7 @@ def test_no_gaps_no_triggering_notification(
 
     gaps_report_sent_at = schedule.gaps_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_gaps_in_schedule(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -113,7 +113,7 @@ def test_gaps_in_the_past_no_triggering_notification(
 
     gaps_report_sent_at = schedule.gaps_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_gaps_in_schedule(schedule.pk)
 
     assert not mock_slack_api_call.called
@@ -165,7 +165,7 @@ def test_gaps_now_trigger_notification(
 
     assert schedule.has_gaps is False
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_gaps_in_schedule(schedule.pk)
 
     assert mock_slack_api_call.called
@@ -219,7 +219,7 @@ def test_gaps_near_future_trigger_notification(
 
     assert schedule.has_gaps is False
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_gaps_in_schedule(schedule.pk)
 
     assert mock_slack_api_call.called
@@ -271,7 +271,7 @@ def test_gaps_later_than_7_days_no_triggering_notification(
 
     gaps_report_sent_at = schedule.gaps_report_sent_at
 
-    with patch("apps.slack.client.SlackClientWithErrorHandling.chat_postMessage") as mock_slack_api_call:
+    with patch("apps.slack.client.SlackClient.chat_postMessage") as mock_slack_api_call:
         notify_about_gaps_in_schedule(schedule.pk)
 
     assert not mock_slack_api_call.called