From 51b71927508eb3e4766a16c8755688fb2dc67165 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 23 Dec 2020 15:33:27 +0000 Subject: [PATCH 1/3] Cache provider lookups for 2 seconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For every email or text message we send we have to work out which provider to send it with. Every time we do this we go and load the list of providers from the database. For emails, the result will always be the same. For text messages the result is randomly chosen to balance the load between the providers. For international text messages the result is always the same (we only have one international text message provider). This commit adds an in-memory cache with a 2 second TTL so that we’re not fetching the providers from the database every time, which should speed things up a bit. This does mean that, for text messages, the random choice will ‘stick’ for two seconds on each instance, before being re-chosen. I think this is OK because it will even out to the same distribution over time. I really don’t like having to clear the cache in the tests, so would welcome suggestions on a better way of doing this… --- app/delivery/send_to_providers.py | 6 +++++- tests/app/delivery/test_send_to_providers.py | 21 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index b22855e08..1c780e801 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,7 +1,7 @@ import random from urllib import parse from datetime import datetime, timedelta - +from cachetools import TTLCache, cached from flask import current_app from notifications_utils.recipients import ( validate_and_format_phone_number, @@ -148,6 +148,10 @@ def update_notification_to_sending(notification, provider): dao_update_notification(notification) +provider_cache = TTLCache(maxsize=8, ttl=2) + + +@cached(cache=provider_cache) 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 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 6cf1013a4..ffde4d39b 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -47,6 +47,23 @@ def test_provider_to_use_should_return_random_provider(mocker, notify_db_session assert ret.get_name() == 'mmg' +def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session): + mock_choices = mocker.patch( + 'app.delivery.send_to_providers.random.choices', + wraps=send_to_providers.random.choices, + ) + + send_to_providers.provider_cache.clear() + + results = [ + send_to_providers.provider_to_use('sms', international=False) + for _ in range(10) + ] + + assert all(result == results[0] for result in results) + assert len(mock_choices.call_args_list) == 1 + + def test_provider_to_use_should_only_return_mmg_for_international(mocker, notify_db_session): mmg = get_provider_details_by_identifier('mmg') mock_choices = mocker.patch('app.delivery.send_to_providers.random.choices', return_value=[mmg]) @@ -73,6 +90,8 @@ def test_provider_to_use_raises_if_no_active_providers(mocker, restore_provider_ mmg = get_provider_details_by_identifier('mmg') mmg.active = False + send_to_providers.provider_cache.clear() + with pytest.raises(Exception): send_to_providers.provider_to_use('sms', international=True) @@ -624,6 +643,7 @@ def test_should_send_sms_to_international_providers( international=True, reply_to_text=sample_template.service.get_default_sms_sender() ) + send_to_providers.provider_cache.clear() send_to_providers.send_sms_to_provider( notification_uk ) @@ -674,6 +694,7 @@ def test_should_handle_sms_sender_and_prefix_message( template = create_template(service, content='bar') notification = create_notification(template, reply_to_text=sms_sender) + send_to_providers.provider_cache.clear() send_to_providers.send_sms_to_provider(notification) mmg_client.send_sms.assert_called_once_with( From 55afc9a401d4768599c4ef578e9e663436a7302a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 31 Dec 2020 09:36:55 +0000 Subject: [PATCH 2/3] Increase provider lookup cache TTL to 10 seconds Tested locally with TTL values of: - 2 seconds - 5 seconds - 10 seconds The benefit really started showing at 10 seconds, where >50% of lookups hit the cache rather than the database. For graphs see https://github.com/alphagov/notifications-api/pull/3075#issuecomment-750836404 --- app/delivery/send_to_providers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 1c780e801..71a8c8d02 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -148,7 +148,7 @@ def update_notification_to_sending(notification, provider): dao_update_notification(notification) -provider_cache = TTLCache(maxsize=8, ttl=2) +provider_cache = TTLCache(maxsize=8, ttl=10) @cached(cache=provider_cache) From 624bd1d12e27ca98e74fe492bada7285c1f07002 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 31 Dec 2020 09:36:55 +0000 Subject: [PATCH 3/3] Make function-level setup fixture clear cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This means that anyone adding a new test to this file doesn’t have to remember to clear the cache in their test, or forget to and have a hard-to-debug test failure. Using `setup_function` means we don’t have to convert this module into using class-based tests. --- tests/app/delivery/test_send_to_providers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index ffde4d39b..4ddfa0563 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -34,6 +34,12 @@ from tests.app.db import ( ) +def setup_function(_function): + # pytest will run this function before each test. It makes sure the + # state of the cache is not shared between tests. + send_to_providers.provider_cache.clear() + + def test_provider_to_use_should_return_random_provider(mocker, notify_db_session): mmg = get_provider_details_by_identifier('mmg') firetext = get_provider_details_by_identifier('firetext') @@ -53,8 +59,6 @@ def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session): wraps=send_to_providers.random.choices, ) - send_to_providers.provider_cache.clear() - results = [ send_to_providers.provider_to_use('sms', international=False) for _ in range(10) @@ -90,8 +94,6 @@ def test_provider_to_use_raises_if_no_active_providers(mocker, restore_provider_ mmg = get_provider_details_by_identifier('mmg') mmg.active = False - send_to_providers.provider_cache.clear() - with pytest.raises(Exception): send_to_providers.provider_to_use('sms', international=True) @@ -643,7 +645,6 @@ def test_should_send_sms_to_international_providers( international=True, reply_to_text=sample_template.service.get_default_sms_sender() ) - send_to_providers.provider_cache.clear() send_to_providers.send_sms_to_provider( notification_uk ) @@ -694,7 +695,6 @@ def test_should_handle_sms_sender_and_prefix_message( template = create_template(service, content='bar') notification = create_notification(template, reply_to_text=sms_sender) - send_to_providers.provider_cache.clear() send_to_providers.send_sms_to_provider(notification) mmg_client.send_sms.assert_called_once_with(