Merge pull request #824 from alphagov/feat-switch-providers-on-slow-delivery

Auto-switch providers on slow delivery of notifications
This commit is contained in:
imdadahad
2017-02-27 16:25:15 +00:00
committed by GitHub
11 changed files with 457 additions and 76 deletions

View File

@@ -1,4 +1,7 @@
from datetime import datetime
from datetime import (
datetime,
timedelta
)
from flask import current_app
from sqlalchemy.exc import SQLAlchemyError
@@ -10,7 +13,12 @@ from app.dao.invited_user_dao import delete_invitations_created_more_than_two_da
from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than
from app.dao.notifications_dao import (
delete_notifications_created_more_than_a_week_ago,
dao_timeout_notifications
dao_timeout_notifications,
is_delivery_slow_for_provider
)
from app.dao.provider_details_dao import (
get_current_provider,
dao_toggle_sms_provider
)
from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago
from app.statsd_decorators import statsd
@@ -141,3 +149,34 @@ def send_daily_performance_platform_stats():
email_sent_count,
'day'
)
@notify_celery.task(name='switch-current-sms-provider-on-slow-delivery')
@statsd(namespace="tasks")
def switch_current_sms_provider_on_slow_delivery():
"""
Switch providers if there are at least two slow delivery notifications (more than four minutes)
in the last ten minutes. Search from the time we last switched to the current provider.
"""
functional_test_provider_service_id = current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID')
functional_test_provider_template_id = current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID')
if functional_test_provider_service_id and functional_test_provider_template_id:
current_provider = get_current_provider('sms')
slow_delivery_notifications = is_delivery_slow_for_provider(
provider=current_provider.identifier,
threshold=2,
sent_at=max(datetime.utcnow() - timedelta(minutes=10), current_provider.updated_at),
delivery_time=timedelta(minutes=4),
service_id=functional_test_provider_service_id,
template_id=functional_test_provider_template_id
)
if slow_delivery_notifications:
current_app.logger.warning(
'Slow delivery notifications detected for provider {}'.format(
current_provider.identifier
)
)
dao_toggle_sms_provider(current_provider.identifier)

View File

@@ -131,6 +131,11 @@ class Config(object):
'schedule': crontab(minute=30, hour=0), # 00:30
'options': {'queue': 'periodic'}
},
'switch-current-sms-provider-on-slow-delivery': {
'task': 'switch-current-sms-provider-on-slow-delivery',
'schedule': crontab(), # Every minute
'options': {'queue': 'periodic'}
},
'timeout-sending-notifications': {
'task': 'timeout-sending-notifications',
'schedule': crontab(minute=0, hour='0,1,2'),
@@ -166,6 +171,9 @@ class Config(object):
SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222')
FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = None
FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = None
######################
# Config overrides ###
@@ -231,6 +239,8 @@ class Live(Config):
CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload'
STATSD_ENABLED = True
FROM_NUMBER = '40604'
FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649'
FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = 'ba9e1789-a804-40b8-871f-cc60d4c1286f'
class CloudFoundryConfig(Config):

View File

@@ -18,6 +18,7 @@ from app.models import (
NotificationStatistics,
Template,
NOTIFICATION_CREATED,
NOTIFICATION_DELIVERED,
NOTIFICATION_SENDING,
NOTIFICATION_PENDING,
NOTIFICATION_TECHNICAL_FAILURE,
@@ -421,3 +422,22 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio
).scalar()
return result or 0
def is_delivery_slow_for_provider(
sent_at,
provider,
threshold,
delivery_time,
service_id,
template_id
):
count = db.session.query(Notification).filter(
Notification.service_id == service_id,
Notification.template_id == template_id,
Notification.sent_at >= sent_at,
Notification.status == NOTIFICATION_DELIVERED,
Notification.sent_by == provider,
(Notification.updated_at - Notification.sent_at) >= delivery_time,
).count()
return count >= threshold

View File

@@ -110,11 +110,11 @@ def dao_fetch_service_by_id_and_user(service_id, user_id):
@transactional
@version_class(Service)
def dao_create_service(service, user):
def dao_create_service(service, user, service_id=None):
from app.dao.permissions_dao import permission_dao
service.users.append(user)
permission_dao.add_default_service_permissions_for_user(user, service)
service.id = uuid.uuid4() # must be set now so version history model can use same id
service.id = service_id or uuid.uuid4() # must be set now so version history model can use same id
service.active = True
service.research_mode = False
db.session.add(service)

View File

@@ -13,8 +13,8 @@ from app.dao.dao_utils import (
@transactional
@version_class(Template, TemplateHistory)
def dao_create_template(template):
template.id = uuid.uuid4() # must be set now so version history model can use same id
def dao_create_template(template, template_id=None):
template.id = template_id or uuid.uuid4() # must be set now so version history model can use same id
template.archived = False
db.session.add(template)

View File

@@ -1,3 +1,5 @@
import pytest
from datetime import datetime, timedelta
from functools import partial
@@ -13,19 +15,57 @@ from app.celery.scheduled_tasks import (
delete_invitations,
timeout_notifications,
run_scheduled_jobs,
send_daily_performance_platform_stats
send_daily_performance_platform_stats,
switch_current_sms_provider_on_slow_delivery
)
from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient
from app.dao.jobs_dao import dao_get_job_by_id
from app.dao.provider_details_dao import (
dao_update_provider_details,
get_current_provider
)
from app.models import Service, Template
from app.utils import get_london_midnight_in_utc
from tests.app.db import create_notification, create_service, create_template
from tests.app.conftest import (
sample_notification as create_sample_notification,
sample_job as create_sample_job,
sample_notification_history as create_notification_history
)
from tests.conftest import set_config_values
from unittest.mock import call, patch, PropertyMock
def _create_slow_delivery_notification(provider='mmg'):
now = datetime.utcnow()
five_minutes_from_now = now + timedelta(minutes=5)
service = Service.query.get(current_app.config['FUNCTIONAL_TEST_PROVIDER_SERVICE_ID'])
if not service:
service = create_service(
service_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID')
)
template = Template.query.get(current_app.config['FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID'])
if not template:
template = create_template(
template_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID'),
service=service
)
create_notification(
template=template,
status='delivered',
sent_by=provider,
updated_at=five_minutes_from_now
)
@pytest.fixture(scope='function')
def prepare_current_provider(restore_provider_details):
initial_provider = get_current_provider('sms')
initial_provider.updated_at = datetime.utcnow() - timedelta(minutes=30)
dao_update_provider_details(initial_provider)
def test_should_have_decorated_tasks_functions():
assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes'
assert delete_successful_notifications.__wrapped__.__name__ == 'delete_successful_notifications'
@@ -35,6 +75,8 @@ def test_should_have_decorated_tasks_functions():
assert run_scheduled_jobs.__wrapped__.__name__ == 'run_scheduled_jobs'
assert remove_csv_files.__wrapped__.__name__ == 'remove_csv_files'
assert send_daily_performance_platform_stats.__wrapped__.__name__ == 'send_daily_performance_platform_stats'
assert switch_current_sms_provider_on_slow_delivery.__wrapped__.__name__ == \
'switch_current_sms_provider_on_slow_delivery'
def test_should_call_delete_successful_notifications_more_than_week_in_task(notify_api, mocker):
@@ -68,26 +110,17 @@ def test_update_status_of_notifications_after_timeout(notify_api,
sample_template,
mmg_provider):
with notify_api.test_request_context():
not1 = create_sample_notification(
notify_db,
notify_db_session,
service=sample_service,
not1 = create_notification(
template=sample_template,
status='sending',
created_at=datetime.utcnow() - timedelta(
seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10))
not2 = create_sample_notification(
notify_db,
notify_db_session,
service=sample_service,
not2 = create_notification(
template=sample_template,
status='created',
created_at=datetime.utcnow() - timedelta(
seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10))
not3 = create_sample_notification(
notify_db,
notify_db_session,
service=sample_service,
not3 = create_notification(
template=sample_template,
status='pending',
created_at=datetime.utcnow() - timedelta(
@@ -105,10 +138,7 @@ def test_not_update_status_of_notification_before_timeout(notify_api,
sample_template,
mmg_provider):
with notify_api.test_request_context():
not1 = create_sample_notification(
notify_db,
notify_db_session,
service=sample_service,
not1 = create_notification(
template=sample_template,
status='sending',
created_at=datetime.utcnow() - timedelta(
@@ -244,3 +274,122 @@ def test_send_daily_performance_stats_calls_with_correct_totals(
call(get_london_midnight_in_utc(yesterday), 'sms', 2, 'day'),
call(get_london_midnight_in_utc(yesterday), 'email', 3, 'day')
])
def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_unset(
notify_api,
mocker
):
get_notifications_mock = mocker.patch(
'app.celery.scheduled_tasks.is_delivery_slow_for_provider'
)
toggle_sms_mock = mocker.patch('app.celery.scheduled_tasks.dao_toggle_sms_provider')
with set_config_values(notify_api, {
'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': None,
'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': None
}):
switch_current_sms_provider_on_slow_delivery()
assert get_notifications_mock.called is False
assert toggle_sms_mock.called is False
def test_switch_providers_on_slow_delivery_runs_if_config_set(
notify_api,
mocker,
prepare_current_provider
):
get_notifications_mock = mocker.patch(
'app.celery.scheduled_tasks.is_delivery_slow_for_provider',
return_value=[]
)
with set_config_values(notify_api, {
'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'
}):
switch_current_sms_provider_on_slow_delivery()
assert get_notifications_mock.called is True
def test_switch_providers_triggers_on_slow_notification_delivery(
notify_api,
prepare_current_provider
):
starting_provider = get_current_provider('sms')
with set_config_values(notify_api, {
'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'
}):
_create_slow_delivery_notification(starting_provider.identifier)
_create_slow_delivery_notification(starting_provider.identifier)
switch_current_sms_provider_on_slow_delivery()
new_provider = get_current_provider('sms')
assert new_provider.identifier != starting_provider.identifier
assert new_provider.priority < starting_provider.priority
def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched(
notify_api,
prepare_current_provider
):
starting_provider = get_current_provider('sms')
with set_config_values(notify_api, {
'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'
}):
_create_slow_delivery_notification()
_create_slow_delivery_notification()
switch_current_sms_provider_on_slow_delivery()
switch_current_sms_provider_on_slow_delivery()
new_provider = get_current_provider('sms')
assert new_provider.identifier != starting_provider.identifier
assert new_provider.priority < starting_provider.priority
def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications(
notify_api,
prepare_current_provider
):
"""
Assume we have three slow delivery notifications for the current provider x. This triggers
a switch to provider y. If we experience some slow delivery notifications on this provider,
we switch back to provider x.
Provider x had three slow deliveries initially, but we do not want to trigger another switch
based on these as they are old. We only want to look for slow notifications after the point at
which we switched back to provider x.
"""
starting_provider = get_current_provider('sms')
with set_config_values(notify_api, {
'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'
}):
# Provider x -> y
_create_slow_delivery_notification(starting_provider.identifier)
_create_slow_delivery_notification(starting_provider.identifier)
_create_slow_delivery_notification(starting_provider.identifier)
switch_current_sms_provider_on_slow_delivery()
current_provider = get_current_provider('sms')
assert current_provider.identifier != starting_provider.identifier
# Provider y -> x
_create_slow_delivery_notification(current_provider.identifier)
_create_slow_delivery_notification(current_provider.identifier)
switch_current_sms_provider_on_slow_delivery()
new_provider = get_current_provider('sms')
assert new_provider.identifier != current_provider.identifier
# Expect to stay on provider x
switch_current_sms_provider_on_slow_delivery()
current_provider = get_current_provider('sms')
assert starting_provider.identifier == current_provider.identifier

View File

@@ -117,13 +117,15 @@ def sample_sms_code(notify_db,
@pytest.fixture(scope='function')
def sample_service(notify_db,
notify_db_session,
service_name="Sample service",
user=None,
restricted=False,
limit=1000,
email_from=None):
def sample_service(
notify_db,
notify_db_session,
service_name="Sample service",
user=None,
restricted=False,
limit=1000,
email_from=None
):
if user is None:
user = create_user()
if email_from is None:
@@ -146,17 +148,19 @@ def sample_service(notify_db,
@pytest.fixture(scope='function')
def sample_template(notify_db,
notify_db_session,
template_name="Template Name",
template_type="sms",
content="This is a template:\nwith a newline",
archived=False,
subject_line='Subject',
user=None,
service=None,
created_by=None,
process_type='normal'):
def sample_template(
notify_db,
notify_db_session,
template_name="Template Name",
template_type="sms",
content="This is a template:\nwith a newline",
archived=False,
subject_line='Subject',
user=None,
service=None,
created_by=None,
process_type='normal'
):
if user is None:
user = create_user()
if service is None:
@@ -421,23 +425,25 @@ def sample_notification_with_job(
@pytest.fixture(scope='function')
def sample_notification(notify_db,
notify_db_session,
service=None,
template=None,
job=None,
job_row_number=None,
to_field=None,
status='created',
reference=None,
created_at=None,
sent_at=None,
billable_units=1,
personalisation=None,
api_key_id=None,
key_type=KEY_TYPE_NORMAL,
sent_by=None,
client_reference=None):
def sample_notification(
notify_db,
notify_db_session,
service=None,
template=None,
job=None,
job_row_number=None,
to_field=None,
status='created',
reference=None,
created_at=None,
sent_at=None,
billable_units=1,
personalisation=None,
api_key_id=None,
key_type=KEY_TYPE_NORMAL,
sent_by=None,
client_reference=None
):
if created_at is None:
created_at = datetime.utcnow()
if service is None:

View File

@@ -40,9 +40,12 @@ from app.dao.notifications_dao import (
dao_delete_notifications_and_history_by_id,
dao_timeout_notifications,
get_financial_year,
get_april_fools)
get_april_fools,
is_delivery_slow_for_provider
)
from app.dao.services_dao import dao_update_service
from tests.app.db import create_notification
from tests.app.conftest import (
sample_notification,
sample_template,
@@ -1422,3 +1425,130 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts(
total_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email')
assert total_count == 2
@freeze_time("2016-01-10 12:00:00.000000")
def test_slow_provider_delivery_returns_for_sent_notifications(
sample_template
):
now = datetime.utcnow()
one_minute_from_now = now + timedelta(minutes=1)
five_minutes_from_now = now + timedelta(minutes=5)
notification_five_minutes_to_deliver = partial(
create_notification,
template=sample_template,
status='delivered',
sent_by='mmg',
updated_at=five_minutes_from_now
)
notification_five_minutes_to_deliver(sent_at=now)
notification_five_minutes_to_deliver(sent_at=one_minute_from_now)
notification_five_minutes_to_deliver(sent_at=one_minute_from_now)
slow_delivery = is_delivery_slow_for_provider(
sent_at=one_minute_from_now,
provider='mmg',
threshold=2,
delivery_time=timedelta(minutes=3),
service_id=sample_template.service.id,
template_id=sample_template.id
)
assert slow_delivery
@freeze_time("2016-01-10 12:00:00.000000")
def test_slow_provider_delivery_observes_threshold(
sample_template
):
now = datetime.utcnow()
five_minutes_from_now = now + timedelta(minutes=5)
notification_five_minutes_to_deliver = partial(
create_notification,
template=sample_template,
status='delivered',
sent_at=now,
sent_by='mmg',
updated_at=five_minutes_from_now
)
notification_five_minutes_to_deliver()
notification_five_minutes_to_deliver()
slow_delivery = is_delivery_slow_for_provider(
sent_at=now,
provider='mmg',
threshold=3,
delivery_time=timedelta(minutes=5),
service_id=sample_template.service.id,
template_id=sample_template.id
)
assert not slow_delivery
@freeze_time("2016-01-10 12:00:00.000000")
def test_slow_provider_delivery_returns_for_delivered_notifications_only(
sample_template
):
now = datetime.utcnow()
five_minutes_from_now = now + timedelta(minutes=5)
notification_five_minutes_to_deliver = partial(
create_notification,
template=sample_template,
sent_at=now,
sent_by='firetext',
created_at=now,
updated_at=five_minutes_from_now
)
notification_five_minutes_to_deliver(status='sending')
notification_five_minutes_to_deliver(status='delivered')
notification_five_minutes_to_deliver(status='delivered')
slow_delivery = is_delivery_slow_for_provider(
sent_at=now,
provider='firetext',
threshold=2,
delivery_time=timedelta(minutes=5),
service_id=sample_template.service.id,
template_id=sample_template.id
)
assert slow_delivery
@freeze_time("2016-01-10 12:00:00.000000")
def test_slow_provider_delivery_does_not_return_for_standard_delivery_time(
sample_template
):
now = datetime.utcnow()
five_minutes_from_now = now + timedelta(minutes=5)
notification = partial(
create_notification,
template=sample_template,
created_at=now,
sent_at=now,
sent_by='mmg',
status='delivered'
)
notification(updated_at=five_minutes_from_now - timedelta(seconds=1))
notification(updated_at=five_minutes_from_now - timedelta(seconds=1))
notification(updated_at=five_minutes_from_now)
slow_delivery = is_delivery_slow_for_provider(
sent_at=now,
provider='mmg',
threshold=2,
delivery_time=timedelta(minutes=5),
service_id=sample_template.service.id,
template_id=sample_template.id
)
assert not slow_delivery

View File

@@ -19,7 +19,7 @@ from app.dao.provider_details_dao import (
)
def set_primary_sms_provider(identifier='mmg'):
def set_primary_sms_provider(identifier):
primary_provider = get_provider_details_by_identifier(identifier)
secondary_provider = get_alternative_sms_provider(identifier)
@@ -153,22 +153,28 @@ def test_toggle_sms_provider_switches_provider(
):
dao_toggle_sms_provider(current_sms_provider.identifier)
new_provider = get_current_provider('sms')
assert new_provider.identifier != current_sms_provider.identifier
assert new_provider.priority < current_sms_provider.priority
old_starting_provider = get_provider_details_by_identifier(current_sms_provider.identifier)
assert new_provider.identifier != old_starting_provider.identifier
assert new_provider.priority < old_starting_provider.priority
def test_toggle_sms_provider_switches_when_provider_priorities_are_equal(
restore_provider_details,
current_sms_provider
restore_provider_details
):
new_provider = get_alternative_sms_provider(current_sms_provider.identifier)
current_sms_provider.priority = new_provider.priority
dao_update_provider_details(current_sms_provider)
current_provider = get_current_provider('sms')
new_provider = get_alternative_sms_provider(current_provider.identifier)
dao_toggle_sms_provider(current_sms_provider.identifier)
current_provider.priority = new_provider.priority
dao_update_provider_details(current_provider)
assert new_provider.identifier != current_sms_provider.identifier
assert new_provider.priority < current_sms_provider.priority
dao_toggle_sms_provider(current_provider.identifier)
old_starting_provider = get_provider_details_by_identifier(current_provider.identifier)
assert new_provider.identifier != old_starting_provider.identifier
assert new_provider.priority < old_starting_provider.priority
def test_toggle_sms_provider_updates_provider_history(

View File

@@ -23,7 +23,7 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off
return user
def create_service(user=None, service_name="Sample service"):
def create_service(user=None, service_name="Sample service", service_id=None):
service = Service(
name=service_name,
message_limit=1000,
@@ -31,14 +31,15 @@ def create_service(user=None, service_name="Sample service"):
email_from=service_name.lower().replace(' ', '.'),
created_by=user or create_user()
)
dao_create_service(service, service.created_by)
dao_create_service(service, service.created_by, service_id)
return service
def create_template(
service,
template_type=SMS_TYPE,
content='Dear Sir/Madam, Hello. Yours Truly, The Government.'
content='Dear Sir/Madam, Hello. Yours Truly, The Government.',
template_id=None
):
data = {
'name': '{} Template Name'.format(template_type),
@@ -50,7 +51,7 @@ def create_template(
if template_type != SMS_TYPE:
data['subject'] = 'Template subject'
template = Template(**data)
dao_create_template(template)
dao_create_template(template, template_id)
return template
@@ -63,6 +64,7 @@ def create_notification(
reference=None,
created_at=None,
sent_at=None,
updated_at=None,
billable_units=1,
personalisation=None,
api_key_id=None,
@@ -72,6 +74,11 @@ def create_notification(
):
if created_at is None:
created_at = datetime.utcnow()
if status != 'created':
sent_at = sent_at or datetime.utcnow()
updated_at = updated_at or datetime.utcnow()
data = {
'id': uuid.uuid4(),
'to': to_field,
@@ -91,7 +98,7 @@ def create_notification(
'api_key_id': api_key_id,
'key_type': key_type,
'sent_by': sent_by,
'updated_at': None,
'updated_at': updated_at,
'client_reference': client_reference,
'job_row_number': job_row_number
}

View File

@@ -113,3 +113,17 @@ def set_config(app, name, value):
app.config[name] = value
yield
app.config[name] = old_val
@contextmanager
def set_config_values(app, dict):
old_values = {}
for key in dict:
old_values[key] = app.config.get(key)
app.config[key] = dict[key]
yield
for key in dict:
app.config[key] = old_values[key]