Merge pull request #1818 from alphagov/redis-templates

add new redis template usage per day key

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']
+        )

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

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

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_TEN_MINUTES'])
     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:
+    #     # "<SERVICE_ID>-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())

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 "service-{}-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

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

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(
+        "service-{}-template-usage-2016-01-01".format(sample_template.service_id),
+        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("{}-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),
+    ]
 
 
 @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(
+        'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key),
+        sample_template.id
+    )
+    mock_expire.assert_called_once_with(
+        'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key),
+        current_app.config['EXPIRE_CACHE_EIGHT_DAYS']
+    )