From 9cf9264822f77eeccd2d2f67ae7920f4af0a45a9 Mon Sep 17 00:00:00 2001 From: Scott Nelson Date: Mon, 28 Aug 2023 09:35:39 -0400 Subject: [PATCH 1/3] feat: Only send status checks when payload has changed --- .../notification/notifiers/status/base.py | 28 ++++++- .../notifiers/tests/unit/test_status.py | 75 +++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/services/notification/notifiers/status/base.py b/services/notification/notifiers/status/base.py index 262b0ac41..5920dfe8a 100644 --- a/services/notification/notifiers/status/base.py +++ b/services/notification/notifiers/status/base.py @@ -6,6 +6,7 @@ from shared.torngit.exceptions import TorngitClientError, TorngitError from shared.utils.sessions import SessionType +from helpers.cache import DEFAULT_TTL, NO_VALUE, cache, make_hash_sha256 from helpers.environment import is_enterprise from helpers.match import match from helpers.metrics import metrics @@ -209,7 +210,32 @@ async def notify(self, comparison: Comparison): payload["url"] = get_pull_url(comparison.pull) else: payload["url"] = get_commit_url(comparison.head.commit) - return await self.send_notification(comparison, payload) + + base_commit = comparison.base.commit if comparison.base else None + head_commit = comparison.head.commit if comparison.head else None + + cache_key = make_hash_sha256( + dict( + type="status_check_notification", + repoid=head_commit.repoid, + base_commitid=base_commit.commitid if base_commit else None, + head_commitid=head_commit.commitid if head_commit else None, + notifier_name=self.name, + notifier_title=self.title, + ) + ) + + last_payload = cache.get_backend().get(cache_key) + if last_payload is NO_VALUE or last_payload != payload: + cache.get_backend().set(cache_key, 300, payload) # 5 min expiration + return await self.send_notification(comparison, payload) + else: + return NotificationResult( + notification_attempted=False, + notification_successful=None, + explanation="payload_unchanged", + data_sent=None, + ) except TorngitClientError: log.warning( "Unable to send status notification to user due to a client-side error", diff --git a/services/notification/notifiers/tests/unit/test_status.py b/services/notification/notifiers/tests/unit/test_status.py index 626a51964..7d8ca4635 100644 --- a/services/notification/notifiers/tests/unit/test_status.py +++ b/services/notification/notifiers/tests/unit/test_status.py @@ -20,6 +20,7 @@ ProjectStatusNotifier, ) from services.notification.notifiers.status.base import StatusNotifier +from services.urls import get_pull_url def test_notification_type(mocker): @@ -487,6 +488,80 @@ async def test_notify_no_base( } assert result.data_received == {"id": "some_id"} + @pytest.mark.asyncio + async def test_notify_uncached( + self, + sample_comparison, + mocker, + ): + comparison = sample_comparison + payload = { + "message": "something to say", + "state": "success", + "url": get_pull_url(comparison.pull), + } + + class TestNotifier(StatusNotifier): + async def build_payload(self, comparison): + return payload + + notifier = TestNotifier( + repository=comparison.head.commit.repository, + title="title", + notifier_yaml_settings={}, + notifier_site_settings=True, + current_yaml=UserYaml({}), + ) + notifier.context = "fake" + + send_notification = mocker.patch.object(TestNotifier, "send_notification") + await notifier.notify(comparison) + send_notification.assert_called_once + + @pytest.mark.asyncio + async def test_notify_cached( + self, + sample_comparison, + mocker, + ): + comparison = sample_comparison + + payload = { + "message": "something to say", + "state": "success", + "url": get_pull_url(comparison.pull), + } + + class TestNotifier(StatusNotifier): + async def build_payload(self, comparison): + return payload + + notifier = TestNotifier( + repository=comparison.head.commit.repository, + title="title", + notifier_yaml_settings={}, + notifier_site_settings=True, + current_yaml=UserYaml({}), + ) + notifier.context = "fake" + + mocker.patch( + "helpers.cache.NullBackend.get", + return_value=payload, + ) + + send_notification = mocker.patch.object(TestNotifier, "send_notification") + result = await notifier.notify(comparison) + assert result == NotificationResult( + notification_attempted=False, + notification_successful=None, + explanation="payload_unchanged", + data_sent=None, + ) + + # payload was cached - we do not send the notification + assert not send_notification.called + @pytest.mark.asyncio async def test_send_notification( self, sample_comparison, mocker, mock_repo_provider From f735d2c98ab765c13ec0acbbfad0c0f2628fb015 Mon Sep 17 00:00:00 2001 From: Scott Nelson Date: Mon, 28 Aug 2023 14:26:34 -0400 Subject: [PATCH 2/3] Make cache TTL a config option --- services/notification/notifiers/status/base.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/notification/notifiers/status/base.py b/services/notification/notifiers/status/base.py index 5920dfe8a..19401fada 100644 --- a/services/notification/notifiers/status/base.py +++ b/services/notification/notifiers/status/base.py @@ -3,6 +3,7 @@ from typing import Dict from shared.analytics_tracking import track_event +from shared.config import get_config from shared.torngit.exceptions import TorngitClientError, TorngitError from shared.utils.sessions import SessionType @@ -227,7 +228,12 @@ async def notify(self, comparison: Comparison): last_payload = cache.get_backend().get(cache_key) if last_payload is NO_VALUE or last_payload != payload: - cache.get_backend().set(cache_key, 300, payload) # 5 min expiration + ttl = int( + get_config( + "setup", "cache", "send_status_notification", default=600 + ) + ) # 10 min default + cache.get_backend().set(cache_key, ttl, payload) return await self.send_notification(comparison, payload) else: return NotificationResult( From 348d6389b50876e1d5693bd6757fef51ccecd855 Mon Sep 17 00:00:00 2001 From: Scott Nelson Date: Mon, 28 Aug 2023 14:37:05 -0400 Subject: [PATCH 3/3] Add log line when skipping notification --- services/notification/notifiers/status/base.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/services/notification/notifiers/status/base.py b/services/notification/notifiers/status/base.py index 19401fada..109eb482a 100644 --- a/services/notification/notifiers/status/base.py +++ b/services/notification/notifiers/status/base.py @@ -236,6 +236,16 @@ async def notify(self, comparison: Comparison): cache.get_backend().set(cache_key, ttl, payload) return await self.send_notification(comparison, payload) else: + log.info( + "Notification payload unchanged. Skipping notification.", + extra=dict( + repoid=head_commit.repoid, + base_commitid=base_commit.commitid if base_commit else None, + head_commitid=head_commit.commitid if head_commit else None, + notifier_name=self.name, + notifier_title=self.title, + ), + ) return NotificationResult( notification_attempted=False, notification_successful=None,