From a2af8b052a0737ea4fb93ca3792e033911314758 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Apr 2021 16:06:13 +0100 Subject: [PATCH] Split up authorisation vs. sequencing checks While both of these are integrity errors (since we should never reach this point in the code + data), this just means the original method comment is still relevant to what immediately follows it. --- app/celery/broadcast_message_tasks.py | 43 ++++++++++--------- .../celery/test_broadcast_message_tasks.py | 10 ++--- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 84173f94d..211e0bcee 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -37,25 +37,7 @@ def get_retry_delay(retry_count): return min(delay, 240) -def check_provider_message_should_send(broadcast_event, provider): - """ - If any previous event hasn't sent yet for that provider, then we shouldn't send the current event. Instead, fail and - raise a P1 - so that a notify team member can assess the state of the previous messages, and if necessary, can - replay the `send_broadcast_provider_message` task if the previous message has now been sent. - - Note: This is called before the new broadcast_provider_message is created. - - # Help, I've come across this code following a pagerduty alert, what should I do? - - 1. Find the failing broadcast_provider_message associated with the previous event that caused this to trip. - 2. If that provider message is still failing to send, fix the issue causing that. The task to send that previous - message might still be retrying in the background - look for logs related to that task. - 3. If that provider message has sent succesfully, you might need to send this task off depending on context. This - might not always be true though, for example, it may not be necessary to send a cancel if the original alert has - already expired. - 4. If you need to re-send this task off again, you'll need to run the following command on paas: - `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)` - """ +def check_event_is_authorised_to_be_sent(broadcast_event, provider): if not broadcast_event.service.active: raise BroadcastIntegrityError( f'Cannot send broadcast_event {broadcast_event.id} ' + @@ -74,6 +56,26 @@ def check_provider_message_should_send(broadcast_event, provider): f'to provider {provider}: the broadcast message is stubbed' ) + +def check_event_makes_sense_in_sequence(broadcast_event, provider): + """ + If any previous event hasn't sent yet for that provider, then we shouldn't send the current event. Instead, fail and + raise a P1 - so that a notify team member can assess the state of the previous messages, and if necessary, can + replay the `send_broadcast_provider_message` task if the previous message has now been sent. + + Note: This is called before the new broadcast_provider_message is created. + + # Help, I've come across this code following a pagerduty alert, what should I do? + + 1. Find the failing broadcast_provider_message associated with the previous event that caused this to trip. + 2. If that provider message is still failing to send, fix the issue causing that. The task to send that previous + message might still be retrying in the background - look for logs related to that task. + 3. If that provider message has sent succesfully, you might need to send this task off depending on context. This + might not always be true though, for example, it may not be necessary to send a cancel if the original alert has + already expired. + 4. If you need to re-send this task off again, you'll need to run the following command on paas: + `send_broadcast_provider_message.apply_async(args=(broadcast_event_id, provider), queue=QueueNames.BROADCASTS)` + """ current_provider_message = broadcast_event.get_provider_message(provider) # if this is the first time a task is being executed, it won't have a provider message yet if current_provider_message and current_provider_message.status != BroadcastProviderMessageStatus.SENDING: @@ -168,7 +170,8 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) - check_provider_message_should_send(broadcast_event, provider) + check_event_is_authorised_to_be_sent(broadcast_event, provider) + check_event_makes_sense_in_sequence(broadcast_event, provider) # the broadcast_provider_message may already exist if we retried previously broadcast_provider_message = broadcast_event.get_provider_message(provider) diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 69673302d..8d65108b6 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -8,7 +8,7 @@ from freezegun import freeze_time from app.celery.broadcast_message_tasks import ( BroadcastIntegrityError, - check_provider_message_should_send, + check_event_makes_sense_in_sequence, get_retry_delay, send_broadcast_event, send_broadcast_provider_message, @@ -600,14 +600,14 @@ def test_get_retry_delay_has_capped_backoff(retry_count, expected_delay): @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_doesnt_raise_if_event_hasnt_expired_yet(sample_template): +def test_check_event_makes_sense_in_sequence_doesnt_raise_if_event_hasnt_expired_yet(sample_template): broadcast_message = create_broadcast_message(sample_template) current_event = create_broadcast_event( broadcast_message, transmitted_starts_at=datetime(2021, 1, 1, 0, 0), transmitted_finishes_at=datetime(2021, 1, 1, 12, 1), ) - check_provider_message_should_send(current_event, 'ee') + check_event_makes_sense_in_sequence(current_event, 'ee') @freeze_time('2021-01-01 12:00') @@ -688,7 +688,7 @@ def test_send_broadcast_provider_message_raises_if_older_event_hasnt_started_sen @freeze_time('2021-01-01 12:00') -def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acked_yet(sample_template): +def test_check_event_makes_sense_in_sequence_doesnt_raise_if_newer_event_not_acked_yet(sample_template): broadcast_message = create_broadcast_message(sample_template) # event approved at midnight current_event = create_broadcast_event( @@ -705,7 +705,7 @@ def test_check_provider_message_should_send_doesnt_raise_if_newer_event_not_acke # this doesn't raise, because the alert event got an ack. The cancel doesn't have an event yet # but this task is only interested in the current task (the update) so doesn't worry about that - check_provider_message_should_send(current_event, 'ee') + check_event_makes_sense_in_sequence(current_event, 'ee') @pytest.mark.parametrize('existing_message_status', [