diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index d9639e86a..30f9a09bf 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -116,14 +116,13 @@ def switch_current_sms_provider_on_slow_delivery(): if current_provider.updated_at > datetime.utcnow() - timedelta(minutes=10): current_app.logger.info("Slow delivery notifications provider switched less than 10 minutes ago.") return - slow_delivery_notifications = is_delivery_slow_for_provider( - provider=current_provider.identifier, + slow_delivery_notifications = is_delivery_slow_for_providers( threshold=0.3, created_at=datetime.utcnow() - timedelta(minutes=10), delivery_time=timedelta(minutes=4), ) - if slow_delivery_notifications: + if slow_delivery_notifications[current_provider]: current_app.logger.warning( 'Slow delivery notifications detected for provider {}'.format( current_provider.identifier diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 025b355e5..ddab42d65 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,5 +1,7 @@ import functools import string +from itertools import groupby +from operator import attrgetter from datetime import ( datetime, timedelta, @@ -15,7 +17,7 @@ from notifications_utils.recipients import ( ) from notifications_utils.statsd_decorators import statsd from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst -from sqlalchemy import (desc, func, asc) +from sqlalchemy import (desc, func, asc, and_) from sqlalchemy.orm import joinedload from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.sql import functions @@ -31,6 +33,7 @@ from app.letters.utils import get_letter_pdf_filename from app.models import ( Notification, NotificationHistory, + ProviderDetails, ScheduledNotification, KEY_TYPE_NORMAL, KEY_TYPE_TEST, @@ -486,40 +489,63 @@ def dao_timeout_notifications(timeout_period_in_seconds): return technical_failure_notifications, temporary_failure_notifications -def is_delivery_slow_for_provider( +def is_delivery_slow_for_providers( created_at, - provider, threshold, delivery_time, ): - count = db.session.query( + """ + Returns a dict of providers and whether they are currently slow or not. eg: + { + 'mmg': True, + 'firetext': False + } + """ + slow_notification_counts = db.session.query( + ProviderDetails.identifier, case( [( Notification.status == NOTIFICATION_DELIVERED, (Notification.updated_at - Notification.sent_at) >= delivery_time )], else_=(datetime.utcnow() - Notification.sent_at) >= delivery_time - ).label("slow"), func.count() - + ).label("slow"), + func.count().label('count') + ).select_from( + ProviderDetails + ).outerjoin( + Notification, and_( + Notification.sent_by == ProviderDetails.identifier, + Notification.created_at >= created_at, + Notification.sent_at.isnot(None), + Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_SENDING]), + Notification.key_type != KEY_TYPE_TEST + ) ).filter( - Notification.created_at >= created_at, - Notification.sent_at.isnot(None), - Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_SENDING]), - Notification.sent_by == provider, - Notification.key_type != KEY_TYPE_TEST - ).group_by("slow").all() + ProviderDetails.notification_type == 'sms', + ProviderDetails.active + ).order_by( + ProviderDetails.identifier + ).group_by( + ProviderDetails.identifier, + "slow" + ) + print(slow_notification_counts) + print([x for x in slow_notification_counts]) - counts = {c[0]: c[1] for c in count} - total_notifications = sum(counts.values()) - slow_notifications = counts.get(True, 0) + slow_providers = {} + for provider, rows in groupby(slow_notification_counts, key=attrgetter('identifier')): + rows = list(rows) + total_notifications = sum(row.count for row in rows) + slow_notifications = sum(row.count for row in rows if row.slow) + + slow_providers[provider] = (slow_notifications / total_notifications >= threshold) - if total_notifications: current_app.logger.info("Slow delivery notifications count for provider {}: {} out of {}. Ratio {}".format( provider, slow_notifications, total_notifications, slow_notifications / total_notifications )) - return slow_notifications / total_notifications >= threshold - else: - return False + + return slow_providers @statsd(namespace="dao") diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 8f8a09764..657939415 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,3 +1,4 @@ +import random from urllib import parse from datetime import datetime @@ -39,7 +40,7 @@ def send_sms_to_provider(notification): return if notification.status == 'created': - provider = provider_to_use(SMS_TYPE, notification.id, notification.international) + provider = provider_to_use(SMS_TYPE, notification.international) template_model = dao_get_template_by_id(notification.template_id, notification.template_version) @@ -81,7 +82,7 @@ def send_email_to_provider(notification): technical_failure(notification=notification) return if notification.status == 'created': - provider = provider_to_use(EMAIL_TYPE, notification.id) + provider = provider_to_use(EMAIL_TYPE) template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ @@ -128,18 +129,20 @@ def update_notification_to_sending(notification, provider): dao_update_notification(notification) -def provider_to_use(notification_type, notification_id, international=False): - active_providers_in_order = [ +def provider_to_use(notification_type, international=False): + active_providers = [ p for p in get_provider_details_by_notification_type(notification_type, international) if p.active ] - if not active_providers_in_order: + if not active_providers: current_app.logger.error( - "{} {} failed as no active providers".format(notification_type, notification_id) + "{} failed as no active providers".format(notification_type) ) raise Exception("No active {} providers".format(notification_type)) - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + chosen_provider = random.choices(active_providers, weights=[p.priority for p in active_providers])[0] + + return clients.get_client_by_name_and_type(chosen_provider.identifier, notification_type) def get_logo_url(base_url, logo_file): diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 420966b88..8f56d2c56 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -26,7 +26,7 @@ from app.dao.notifications_dao import ( get_notification_with_personalisation, get_notifications_for_job, get_notifications_for_service, - is_delivery_slow_for_provider, + is_delivery_slow_for_providers, set_scheduled_notification_to_processed, update_notification_status_by_id, update_notification_status_by_reference, @@ -957,7 +957,7 @@ def test_should_exclude_test_key_notifications_by_default( ] ) @freeze_time("2018-12-04 12:00:00.000000") -def test_is_delivery_slow_for_provider( +def test_is_delivery_slow_for_providers( notify_db_session, sample_template, normal_sending, @@ -992,7 +992,11 @@ def test_is_delivery_slow_for_provider( for _ in range(slow_delivered): slow_notification(status='delivered') - assert is_delivery_slow_for_provider(datetime.utcnow(), "mmg", threshold, timedelta(minutes=4)) is expected_result + result = is_delivery_slow_for_providers(datetime.utcnow(), threshold, timedelta(minutes=4)) + assert result == { + 'firetext': False, + 'mmg': expected_result + } @pytest.mark.parametrize("options,expected_result", [ @@ -1008,22 +1012,23 @@ def test_is_delivery_slow_for_provider( ]) @freeze_time("2018-12-04 12:00:00.000000") -def test_delivery_is_delivery_slow_for_provider_filters_out_notifications_it_should_not_count( +def test_delivery_is_delivery_slow_for_providers_filters_out_notifications_it_should_not_count( notify_db_session, sample_template, options, expected_result ): - create_notification_with = { + create_slow_notification_with = { "template": sample_template, "sent_at": datetime.now() - timedelta(minutes=5), "updated_at": datetime.now(), } - create_notification_with.update(options) + create_slow_notification_with.update(options) create_notification( - **create_notification_with + **create_slow_notification_with ) - assert is_delivery_slow_for_provider(datetime.utcnow(), "mmg", 0.1, timedelta(minutes=4)) is expected_result + result = is_delivery_slow_for_providers(datetime.utcnow(), 0.1, timedelta(minutes=4)) + assert result['mmg'] == expected_result def test_dao_get_notifications_by_to_field(sample_template): diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 92c6f8fed..131ee89d6 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -40,7 +40,7 @@ def test_should_return_highest_priority_active_provider(restore_provider_details first = providers[0] second = providers[1] - assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier + assert send_to_providers.provider_to_use('sms').name == first.identifier first.priority = 20 second.priority = 10 @@ -48,7 +48,7 @@ def test_should_return_highest_priority_active_provider(restore_provider_details provider_details_dao.dao_update_provider_details(first) provider_details_dao.dao_update_provider_details(second) - assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier + assert send_to_providers.provider_to_use('sms').name == second.identifier first.priority = 10 first.active = False @@ -57,12 +57,12 @@ def test_should_return_highest_priority_active_provider(restore_provider_details provider_details_dao.dao_update_provider_details(first) provider_details_dao.dao_update_provider_details(second) - assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier + assert send_to_providers.provider_to_use('sms').name == second.identifier first.active = True provider_details_dao.dao_update_provider_details(first) - assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier + assert send_to_providers.provider_to_use('sms').name == first.identifier def test_should_send_personalised_template_to_correct_sms_provider_and_persist(