From e692e06b7fc3d13b4d274ad15a212af397dbf344 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 22 Mar 2018 16:18:22 +0000 Subject: [PATCH 1/3] add new template stat read pseudocode we can't use this new code until the new redis keys have been popualted for the last seven days --- app/template_statistics/rest.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 950b5ecd6..e6fbcb924 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -79,7 +79,22 @@ def get_template_statistics_for_7_days(limit_days, service_id): if cache_values: redis_store.set_hash_and_expire(cache_key, cache_values, - current_app.config.get('EXPIRE_CACHE_IN_SECONDS', 600)) + current_app.config['EXPIRE_CACHE_IN_SECONDS']) else: stats = dao_get_templates_for_cache(template_stats_by_id.items()) return stats + + # TODO: can only switch to this code when redis has been populated (either through time passing or a manual step) + # from collections import Counter + # from notifications_utils.redis_client import RedisException + # template_stats_by_id = Counter() + # for day in last_7_days: + # # "-template-usage-{YYYY-MM-DD}" + # key = cache_key_for_service_templates_used_per_day(service_id, limit_days) + # try: + # template_stats_by_id += Counter(redis_store.get_all_from_hash(key, raise_exception=True)) + # except RedisException: + # # TODO: ???? + # + # # TODO: streamline db query and avoid weird unions if possible. + # return dao_get_templates_for_cache(template_stats_by_id.items()) From 8e73961f65e65f80d883a41490f111060a84ac46 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 29 Mar 2018 13:55:22 +0100 Subject: [PATCH 2/3] add new redis template usage per day key We've run into issues with redis expiring keys while we try and write to them - short lived redis TTLs aren't really sustainable for keys where we mutate the state. Template usage is a hash contained in redis where we increment a count keyed by template_id each time a message is sent for that template. But if the key expires, hincrby (redis command for incrementing a value in a hash) will re-create an empty hash. This is no good, as we need the hash to be populated with the last seven days worth of data, which we then increment further. We can't tell whether the hincrby created the key, so a different approach entirely was needed: * New redis key: -template-usage-. Note: This YYYY-MM-DD is BTC time so it lines up nicely with ft_billing table * Incremented to from process_notification - if it doesn't exist yet, it'll be created then. * Expiry set to 8 days every time it's incremented to. Then, at read time, we'll just read the last eight days of keys from Redis, and sum them up. This works because we're only ever incrementing from that one place - never setting wholesale, never recreating the data from scratch. So we know that if the data is in redis, then it is good and accurate data. One thing we *don't* know and *cannot* reason about is what no key in redis means. It could be either of: * This is the first message that the service has sent today. * The key was deleted from redis for some reason. Since we set the TTL to so long, we'll never be writing to a key that previously expired. But if there is a redis (or operator) error and the key is deleted, then we'll have bad data - after any data loss we'll have to rebuild the data. --- app/config.py | 3 +- app/notifications/process_notifications.py | 20 +++++- app/template_statistics/rest.py | 2 +- app/utils.py | 4 ++ requirements.txt | 2 +- .../test_process_notification.py | 64 +++++++++++++++++-- 6 files changed, 86 insertions(+), 9 deletions(-) diff --git a/app/config.py b/app/config.py index 315845a5d..b9c6b9d9b 100644 --- a/app/config.py +++ b/app/config.py @@ -95,7 +95,8 @@ class Config(object): # URL of redis instance REDIS_URL = os.getenv('REDIS_URL') REDIS_ENABLED = os.getenv('REDIS_ENABLED') == '1' - EXPIRE_CACHE_IN_SECONDS = 600 + EXPIRE_CACHE_TEN_MINUTES = 600 + EXPIRE_CACHE_EIGHT_DAYS = 8 * 24 * 60 * 60 # Performance platform PERFORMANCE_PLATFORM_ENABLED = False diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 41378f8da..100790f83 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -29,7 +29,13 @@ from app.dao.notifications_dao import ( ) from app.v2.errors import BadRequestError -from app.utils import get_template_instance, cache_key_for_service_template_counter, convert_bst_to_utc +from app.utils import ( + cache_key_for_service_template_counter, + cache_key_for_service_template_usage_per_day, + convert_bst_to_utc, + convert_utc_to_bst, + get_template_instance, +) def create_content_for_notification(template, personalisation): @@ -108,12 +114,24 @@ def persist_notification( redis_store.incr(redis.daily_limit_cache_key(service.id)) if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) + + increment_template_usage_cache(service.id, template_id, notification_created_at) + current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) ) return notification +def increment_template_usage_cache(service_id, template_id, created_at): + key = cache_key_for_service_template_usage_per_day(service_id, convert_utc_to_bst(created_at)) + redis_store.increment_hash_value(key, template_id) + # set key to expire in eight days - we don't know if we've just created the key or not, so must assume that we + # have and reset the expiry. Eight days is longer than any notification is in the notifications table, so we'll + # always capture the full week's numbers + redis_store.expire(key, current_app.config['EXPIRE_CACHE_EIGHT_DAYS']) + + def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index e6fbcb924..71667860e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -79,7 +79,7 @@ def get_template_statistics_for_7_days(limit_days, service_id): if cache_values: redis_store.set_hash_and_expire(cache_key, cache_values, - current_app.config['EXPIRE_CACHE_IN_SECONDS']) + current_app.config['EXPIRE_CACHE_TEN_MINUTES']) else: stats = dao_get_templates_for_cache(template_stats_by_id.items()) return stats diff --git a/app/utils.py b/app/utils.py index b1367cc7d..a929e1a4f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -79,6 +79,10 @@ def cache_key_for_service_template_counter(service_id, limit_days=7): return "{}-template-counter-limit-{}-days".format(service_id, limit_days) +def cache_key_for_service_template_usage_per_day(service_id, datetime): + return "{}-template-usage-{}".format(service_id, datetime.date().isoformat()) + + def get_public_notify_type_text(notify_type, plural=False): from app.models import SMS_TYPE notify_type_text = notify_type diff --git a/requirements.txt b/requirements.txt index 5e85a803a..5f7c747aa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,6 +23,6 @@ notifications-python-client==4.8.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@25.2.3#egg=notifications-utils==25.2.3 +git+https://github.com/alphagov/notifications-utils.git@25.3.0#egg=notifications-utils==25.3.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 342a17987..dc8b3c2b0 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,11 +1,13 @@ import datetime import uuid -import pytest +from unittest.mock import call +import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple +from flask import current_app from app.models import ( Notification, @@ -205,9 +207,11 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) @freeze_time("2016-01-01 11:09:00.061258") -def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): +def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker): mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') + mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) + mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) persist_notification( template_id=sample_template.id, @@ -221,11 +225,20 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa reference="ref" ) mock_incr.assert_not_called() - mock_incr_hash_value.assert_not_called() + mock_incr_hash_value.assert_called_once_with( + str(sample_template.service_id) + "-template-usage-2016-01-01", + sample_template.id + ) + +@freeze_time("2016-01-01 11:09:00.061258") +def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): + mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') + mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value={sample_template.id, 1}) + persist_notification( template_id=sample_template.id, template_version=sample_template.version, @@ -236,9 +249,12 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa api_key_id=sample_api_key.id, key_type=sample_api_key.key_type, reference="ref2") + mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) - mock_incr_hash_value.assert_called_once_with(cache_key_for_service_template_counter(sample_template.service_id), - sample_template.id) + assert mock_incr_hash_value.mock_calls == [ + call(str(sample_template.service_id) + "-template-counter-limit-7-days", sample_template.id), + call(str(sample_template.service_id) + "-template-usage-2016-01-01", sample_template.id), + ] @pytest.mark.parametrize('research_mode, requested_queue, expected_queue, notification_type, key_type', @@ -446,3 +462,41 @@ def test_persist_email_notification_stores_normalised_email( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised + + +@pytest.mark.parametrize('utc_time, day_in_key', [ + ('2016-01-01 23:00:00', '2016-01-01'), + ('2016-06-01 22:59:00', '2016-06-01'), + ('2016-06-01 23:00:00', '2016-06-02'), +]) +def test_persist_notification_increments_and_expires_redis_template_usage( + utc_time, + day_in_key, + sample_template, + sample_api_key, + mocker +): + mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') + mock_expire = mocker.patch('app.notifications.process_notifications.redis_store.expire') + mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) + mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) + + with freeze_time(utc_time): + persist_notification( + template_id=sample_template.id, + template_version=sample_template.version, + recipient='+447111111122', + service=sample_template.service, + personalisation={}, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + ) + mock_incr_hash_value.assert_called_once_with( + '{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + sample_template.id + ) + mock_expire.assert_called_once_with( + '{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) From 6e554188bd2e379d1fa2050a61c76bacc38fd8ce Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 3 Apr 2018 15:55:22 +0100 Subject: [PATCH 3/3] add command to backfill template usage The command takes a service id and a day, grabs the historical data for that day (potentially out of notification_history), and pops it in redis (for eight days, same as if it were written to manually). also, prefix template usage key with "service" to make clear that it's a service id, and not an individual template id. --- app/commands.py | 45 +++++++++++++++++-- app/utils.py | 2 +- .../test_process_notification.py | 10 ++--- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/app/commands.py b/app/commands.py index a0a00ca32..c76f563f3 100644 --- a/app/commands.py +++ b/app/commands.py @@ -8,8 +8,10 @@ import flask from click_datetime import Datetime as click_dt from flask import current_app from sqlalchemy.orm.exc import NoResultFound +from sqlalchemy import func +from notifications_utils.statsd_decorators import statsd -from app import db, DATETIME_FORMAT, encryption +from app import db, DATETIME_FORMAT, encryption, redis_store from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf @@ -28,8 +30,11 @@ from app.dao.services_dao import ( from app.dao.users_dao import (delete_model_user, delete_user_verify_codes) from app.models import PROVIDERS, User, SMS_TYPE, EMAIL_TYPE, Notification from app.performance_platform.processing_time import (send_processing_time_for_start_and_end) -from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc -from notifications_utils.statsd_decorators import statsd +from app.utils import ( + cache_key_for_service_template_usage_per_day, + get_london_midnight_in_utc, + get_midnight_for_day_before, +) @click.group(name='command', help='Additional commands') @@ -489,3 +494,37 @@ def migrate_data_to_ft_billing(start_date, end_date): total_updated += result.rowcount print('Total inserted/updated records = {}'.format(total_updated)) + + +@notify_command() +@click.option('-s', '--service_id', required=True, type=click.UUID) +@click.option('-d', '--day', required=True, type=click_dt(format='%Y-%m-%d')) +def populate_redis_template_usage(service_id, day): + """ + Recalculate and replace the stats in redis for a day. + To be used if redis data is lost for some reason. + """ + assert current_app.config['REDIS_ENABLED'] + usage = { + str(row.template_id): row.count + for row in db.session.query( + Notification.template_id, + func.count().label('count') + ).filter( + Notification.service_id == service_id, + ).group_by( + Notification.template_id + ) + } + current_app.logger.info('Populating usage dict for service {} day {}: {}'.format( + service_id, + day, + usage.items()) + ) + if usage: + key = cache_key_for_service_template_usage_per_day(service_id, day) + redis_store.set_hash_and_expire( + key, + usage, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) diff --git a/app/utils.py b/app/utils.py index a929e1a4f..1646639c6 100644 --- a/app/utils.py +++ b/app/utils.py @@ -80,7 +80,7 @@ def cache_key_for_service_template_counter(service_id, limit_days=7): def cache_key_for_service_template_usage_per_day(service_id, datetime): - return "{}-template-usage-{}".format(service_id, datetime.date().isoformat()) + return "service-{}-template-usage-{}".format(service_id, datetime.date().isoformat()) def get_public_notify_type_text(notify_type, plural=False): diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index dc8b3c2b0..cc3df6226 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -226,7 +226,7 @@ def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sa ) mock_incr.assert_not_called() mock_incr_hash_value.assert_called_once_with( - str(sample_template.service_id) + "-template-usage-2016-01-01", + "service-{}-template-usage-2016-01-01".format(sample_template.service_id), sample_template.id ) @@ -252,8 +252,8 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) assert mock_incr_hash_value.mock_calls == [ - call(str(sample_template.service_id) + "-template-counter-limit-7-days", sample_template.id), - call(str(sample_template.service_id) + "-template-usage-2016-01-01", sample_template.id), + call("{}-template-counter-limit-7-days".format(sample_template.service_id), sample_template.id), + call("service-{}-template-usage-2016-01-01".format(sample_template.service_id), sample_template.id), ] @@ -493,10 +493,10 @@ def test_persist_notification_increments_and_expires_redis_template_usage( key_type=sample_api_key.key_type, ) mock_incr_hash_value.assert_called_once_with( - '{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), sample_template.id ) mock_expire.assert_called_once_with( - '{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), + 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] )