Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Only send status checks when payload has changed #74

Merged
merged 4 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion services/notification/notifiers/status/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
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

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
Expand Down Expand Up @@ -209,7 +211,47 @@ 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the payload is JSON serializable (not sure if it is), can we use the payload as the key?
It would save us the last_payload != payload check... if the payload is there (as a key) we know we sent that before (in the last TTL seconds)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that would work as expected. Imagine the following sequence of events...

  1. notify with payload X -> hashes to ABC (not in cache so we send notification)
  2. notify with payload X -> hashes to ABC (already cached, skip notification)
  3. notify with payload Y -> hashes to DEF (not in cache so we send notification)
  4. notify with payload X -> hashes to ABC (already in cache, skip notification)

We need to make sure that the last item there actually DOES send a notification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see... Then indeed my understanding was incorrect.
I thought that case 4 there would indicate a task that got delayed for whatever reason (had to retry for example) and was "stale" data (because something more recent, in this case payload Y) was already delivered.

But indeed I see that case 4 would skip notification and it's not what we want.
Thanks for taking the time to explain.

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:
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,
explanation="payload_unchanged",
data_sent=None,
)
except TorngitClientError:
log.warning(
"Unable to send status notification to user due to a client-side error",
Expand Down
75 changes: 75 additions & 0 deletions services/notification/notifiers/tests/unit/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
Loading