diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 0d069ea63..b67d3e61b 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -25,19 +25,17 @@ from app.dao.jobs_dao import ( ) from app.dao.jobs_dao import dao_update_job from app.dao.notifications_dao import ( - is_delivery_slow_for_provider, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, notifications_not_yet_sent, dao_precompiled_letters_still_pending_virus_check, dao_old_letters_with_created_status, - letters_missing_from_sending_bucket -) -from app.dao.provider_details_dao import ( - get_current_provider, - dao_reduce_sms_provider_priority + letters_missing_from_sending_bucket, + is_delivery_slow_for_providers, ) +from app.dao.provider_details_dao import dao_reduce_sms_provider_priority from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.models import ( Job, JOB_STATUS_IN_PROGRESS, @@ -109,27 +107,30 @@ def delete_invitations(): @statsd(namespace="tasks") def switch_current_sms_provider_on_slow_delivery(): """ - Switch providers if at least 30% of notifications took more than four minutes to be delivered - in the last ten minutes. Search from the time we last switched to the current provider. + Reduce provider's priority if at least 30% of notifications took more than four minutes to be delivered + in the last ten minutes. If both providers are slow, don't do anything. If we changed the providers in the + last ten minutes, then don't update them again either. """ - current_provider = get_current_provider('sms') - if current_provider.updated_at > datetime.utcnow() - timedelta(minutes=10): + providers = get_provider_details_by_notification_type('sms') + # if something updated recently, don't update again. If the updated_at is null, set it to min time + # just to prevent errors + if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in providers): current_app.logger.info("Slow delivery notifications provider switched less than 10 minutes ago.") return + 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[current_provider]: - current_app.logger.warning( - 'Slow delivery notifications detected for provider {}'.format( - current_provider.identifier - ) - ) - - dao_reduce_sms_provider_priority(current_provider.identifier) + # only adjust if some values are true and some are false - ie, don't adjust if all providers are fast or + # all providers are slow + if len(set(slow_delivery_notifications.values())) != 1: + for provider_name, is_slow in slow_delivery_notifications.items(): + if is_slow: + current_app.logger.warning('Slow delivery notifications detected for provider {}'.format(provider_name)) + dao_reduce_sms_provider_priority(provider_name) @notify_celery.task(name='check-job-status') diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 0c35c32fe..5fa11eebb 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -15,11 +15,13 @@ from app.celery.scheduled_tasks import ( replay_created_notifications, check_precompiled_letter_state, check_templated_letter_state, - check_for_missing_rows_in_completed_jobs + check_for_missing_rows_in_completed_jobs, + switch_current_sms_provider_on_slow_delivery, ) from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id from app.dao.notifications_dao import dao_get_scheduled_notifications +from app.dao.provider_details_dao import get_provider_details_by_notification_type, get_provider_details_by_identifier from app.models import ( JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, @@ -98,13 +100,61 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(sample_template, mock ]) -def test_switch_providers_on_slow_delivery_switches_once_then_does_not_switch_if_already_switched( - notify_api, - mocker, - sample_user, - sample_template +@freeze_time('2017-05-01 14:00:00') +def test_switch_current_sms_provider_on_slow_delivery_switches_when_one_provider_is_slow( + mocker, + restore_provider_details, ): - raise NotImplementedError # TODO + is_slow_dict = {'mmg': False, 'firetext': True} + mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers', return_value=is_slow_dict) + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + # updated_at times are older than the 10 minute window + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 49) + get_provider_details_by_identifier('firetext').updated_at = None + + switch_current_sms_provider_on_slow_delivery() + + mock_is_slow.assert_called_once_with( + threshold=0.3, + created_at=datetime(2017, 5, 1, 13, 50), + delivery_time=timedelta(minutes=4) + ) + mock_reduce.assert_called_once_with('firetext') + + +@freeze_time('2017-05-01 14:00:00') +def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_recent_changes( + mocker, + restore_provider_details, +): + mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers') + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) + + switch_current_sms_provider_on_slow_delivery() + + assert mock_is_slow.called is False + assert mock_reduce.called is False + + +@freeze_time('2017-05-01 14:00:00') +@pytest.mark.parametrize('is_slow_dict', [ + {'mmg': False, 'firetext': False}, + {'mmg': True, 'firetext': True}, +]) +def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_no_need( + mocker, + restore_provider_details, + is_slow_dict +): + mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers', return_value=is_slow_dict) + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) + + + switch_current_sms_provider_on_slow_delivery() + + assert mock_reduce.called is False @freeze_time("2017-05-01 14:00:00")