refactor template stats endpoint to read from new redis keys

New redis keys are partitioned per service per day. New process is as
follows:

* require a count of days to filter by. Currently admin always gives 7.
* for each day, check and see if there's anything in redis. There won't
  be if either a) redis is/was down or b) the service didn't send any
  notifications that day
  - if there isn't, go to the database and get a count out.
* combine all these stats together
* get the names/template types etc out of the DB at the end.
This commit is contained in:
Leo Hemsted
2018-04-12 10:46:22 +01:00
parent 5e702449cb
commit 9e8b6fd00d
7 changed files with 177 additions and 382 deletions

View File

@@ -50,7 +50,7 @@ from app.utils import convert_utc_to_bst, get_london_midnight_in_utc
@statsd(namespace="dao")
def dao_get_template_usage(service_id, limit_days=None, day=None):
def dao_get_template_usage(service_id, *, limit_days=None, day=None):
if bool(limit_days) == bool(day):
raise ValueError('Must filter on either limit_days or a specific day')
@@ -75,8 +75,9 @@ def dao_get_template_usage(service_id, limit_days=None, day=None):
).group_by(
Notification.template_id
).subquery()
query = db.session.query(
Template.id.label('template_id'),
Template.id,
Template.name,
Template.template_type,
Template.is_precompiled_letter,

View File

@@ -2,7 +2,6 @@ from datetime import datetime
import uuid
from sqlalchemy import asc, desc
from sqlalchemy.sql.expression import bindparam
from app import db
from app.models import (
@@ -124,25 +123,16 @@ def dao_get_template_versions(service_id, template_id):
).all()
def dao_get_templates_for_cache(cache):
if not cache or len(cache) == 0:
return []
# First create a subquery that is a union select of the cache values
# Then join templates to the subquery
cache_queries = [
db.session.query(bindparam("template_id" + str(i),
uuid.UUID(template_id.decode())).label('template_id'),
bindparam("count" + str(i), int(count.decode())).label('count'))
for i, (template_id, count) in enumerate(cache)]
cache_subq = cache_queries[0].union(*cache_queries[1:]).subquery()
query = db.session.query(Template.id.label('template_id'),
Template.template_type,
Template.name,
Template.is_precompiled_letter,
cache_subq.c.count.label('count')
).join(cache_subq,
Template.id == cache_subq.c.template_id
).order_by(Template.name)
def dao_get_multiple_template_details(template_ids):
query = db.session.query(
Template.id,
Template.template_type,
Template.name,
Template.is_precompiled_letter
).filter(
Template.id.in_(template_ids)
).order_by(
Template.name
)
return query.all()

View File

@@ -1,8 +1,8 @@
from flask import (
Blueprint,
jsonify,
request,
current_app)
request
)
from app import redis_store
from app.dao.notifications_dao import (
@@ -10,15 +10,16 @@ from app.dao.notifications_dao import (
dao_get_last_template_usage
)
from app.dao.templates_dao import (
dao_get_templates_for_cache,
dao_get_multiple_template_details,
dao_get_template_by_id_and_service_id
)
from app.schemas import notification_with_template_schema
from app.utils import cache_key_for_service_template_counter
from app.utils import cache_key_for_service_template_usage_per_day, last_n_days
from app.errors import register_errors, InvalidRequest
from collections import Counter
template_statistics = Blueprint('template-statistics',
template_statistics = Blueprint('template_statistics',
__name__,
url_prefix='/service/<service_id>/template-statistics')
@@ -27,31 +28,17 @@ register_errors(template_statistics)
@template_statistics.route('')
def get_template_statistics_for_service_by_day(service_id):
if request.args.get('limit_days'):
try:
limit_days = int(request.args['limit_days'])
except ValueError as e:
error = '{} is not an integer'.format(request.args['limit_days'])
message = {'limit_days': [error]}
raise InvalidRequest(message, status_code=400)
else:
limit_days = 7
try:
limit_days = int(request.args.get('limit_days'))
except ValueError:
error = '{} is not an integer'.format(request.args.get('limit_days'))
message = {'limit_days': [error]}
raise InvalidRequest(message, status_code=400)
if limit_days == 7:
stats = get_template_statistics_for_7_days(limit_days, service_id)
else:
stats = dao_get_template_usage(service_id, limit_days=limit_days)
if limit_days < 1 or limit_days > 7:
raise InvalidRequest({'limit_days': ['limit_days must be between 1 and 7']}, status_code=400)
def serialize(data):
return {
'count': data.count,
'template_id': str(data.template_id),
'template_name': data.name,
'template_type': data.template_type,
'is_precompiled_letter': data.is_precompiled_letter
}
return jsonify(data=[serialize(row) for row in stats])
return jsonify(data=get_template_statistics_for_last_n_days(service_id, limit_days))
@template_statistics.route('/<template_id>')
@@ -70,31 +57,30 @@ def get_template_statistics_for_template_id(service_id, template_id):
return jsonify(data=data)
def get_template_statistics_for_7_days(limit_days, service_id):
cache_key = cache_key_for_service_template_counter(service_id)
template_stats_by_id = redis_store.get_all_from_hash(cache_key)
if not template_stats_by_id:
stats = dao_get_template_usage(service_id, limit_days=limit_days)
cache_values = dict([(x.template_id, x.count) for x in stats])
if cache_values:
redis_store.set_hash_and_expire(cache_key,
cache_values,
current_app.config['EXPIRE_CACHE_TEN_MINUTES'])
else:
stats = dao_get_templates_for_cache(template_stats_by_id.items())
return stats
def get_template_statistics_for_last_n_days(service_id, limit_days):
template_stats_by_id = Counter()
# 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())
for day in last_n_days(limit_days):
# "{SERVICE_ID}-template-usage-{YYYY-MM-DD}"
key = cache_key_for_service_template_usage_per_day(service_id, day)
stats = redis_store.get_all_from_hash(key)
if not stats:
# key didn't exist (or redis was down) - lets populate from DB.
stats = {
row.id: row.count for row in dao_get_template_usage(service_id, day=day)
}
template_stats_by_id += Counter(stats)
# attach count from stats to name/type/etc from database
template_details = dao_get_multiple_template_details(template_stats_by_id.keys())
return [
{
'count': template_stats_by_id[template.id],
'template_id': str(template.id),
'template_name': template.name,
'template_type': template.template_type,
'is_precompiled_letter': template.is_precompiled_letter
}
for template in template_details
]

View File

@@ -80,6 +80,9 @@ def cache_key_for_service_template_counter(service_id, limit_days=7):
def cache_key_for_service_template_usage_per_day(service_id, datetime):
"""
You should probably pass a BST datetime into this function
"""
return "service-{}-template-usage-{}".format(service_id, datetime.date().isoformat())
@@ -96,4 +99,19 @@ def days_ago(number_of_days):
"""
Returns midnight a number of days ago. Takes care of daylight savings etc.
"""
return get_london_midnight_in_utc(datetime.utcnow() - timedelta(number_of_days))
return get_london_midnight_in_utc(datetime.utcnow() - timedelta(days=number_of_days))
def last_n_days(limit_days):
"""
Returns the last n dates, oldest first. Takes care of daylight savings (but returns a date, be careful how you manipulate it later! Don't
directly use the date for comparing to UTC datetimes!). Includes today.
"""
return [
datetime.combine(
(convert_utc_to_bst(datetime.utcnow()) - timedelta(days=x)),
datetime.min.time()
)
# reverse the countdown, -1 from first two args to ensure it stays 0-indexed
for x in range(limit_days - 1, -1, -1)
]

View File

@@ -10,14 +10,13 @@ from app.dao.templates_dao import (
dao_get_all_templates_for_service,
dao_update_template,
dao_get_template_versions,
dao_get_templates_for_cache,
dao_get_multiple_template_details,
dao_redact_template, dao_update_template_reply_to
)
from app.models import (
Template,
TemplateHistory,
TemplateRedacted,
PRECOMPILED_TEMPLATE_NAME
TemplateRedacted
)
from tests.app.conftest import sample_template as create_sample_template
@@ -481,77 +480,16 @@ def test_get_template_versions_is_empty_for_hidden_templates(notify_db, notify_d
assert len(versions) == 0
def test_get_templates_by_ids_successful(notify_db, notify_db_session):
template_1 = create_sample_template(
notify_db,
notify_db_session,
template_name='Sample Template 1',
template_type="sms",
content="Template content"
)
template_2 = create_sample_template(
notify_db,
notify_db_session,
template_name='Sample Template 2',
template_type="sms",
content="Template content"
)
create_sample_template(
notify_db,
notify_db_session,
template_name='Sample Template 3',
template_type="email",
content="Template content"
)
sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2'),
str.encode(str(template_2.id)): str.encode('3')}
cache = [[k, v] for k, v in sample_cache_dict.items()]
templates = dao_get_templates_for_cache(cache)
assert len(templates) == 2
assert [(template_1.id, template_1.template_type, template_1.name, False, 2),
(template_2.id, template_2.template_type, template_2.name, False, 3)] == templates
def test_get_multiple_template_details_returns_templates_for_list_of_ids(sample_service):
t1 = create_template(sample_service)
t2 = create_template(sample_service)
create_template(sample_service) # t3
res = dao_get_multiple_template_details([t1.id, t2.id])
def test_get_letter_templates_by_ids_successful(notify_db, notify_db_session):
template_1 = create_sample_template(
notify_db,
notify_db_session,
template_name=PRECOMPILED_TEMPLATE_NAME,
template_type="letter",
content="Template content",
hidden=True
)
template_2 = create_sample_template(
notify_db,
notify_db_session,
template_name='Sample Template 2',
template_type="letter",
content="Template content"
)
sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2'),
str.encode(str(template_2.id)): str.encode('3')}
cache = [[k, v] for k, v in sample_cache_dict.items()]
templates = dao_get_templates_for_cache(cache)
assert len(templates) == 2
assert [(template_1.id, template_1.template_type, template_1.name, True, 2),
(template_2.id, template_2.template_type, template_2.name, False, 3)] == templates
def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db_session):
template_1 = create_sample_template(
notify_db,
notify_db_session,
template_name='Sample Template 1',
template_type="sms",
content="Template content"
)
sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2')}
cache = [[k, v] for k, v in sample_cache_dict.items()]
templates = dao_get_templates_for_cache(cache)
assert len(templates) == 1
assert [(template_1.id, template_1.template_type, template_1.name, False, 2)] == templates
def test_get_templates_by_ids_returns_empty_list():
assert dao_get_templates_for_cache({}) == []
assert dao_get_templates_for_cache(None) == []
assert {x.id for x in res} == {t1.id, t2.id}
# make sure correct properties are on each row
assert res[0].id
assert res[0].template_type
assert res[0].name
assert not res[0].is_precompiled_letter

View File

@@ -1,281 +1,129 @@
from datetime import datetime, timedelta
import json
import uuid
import pytest
from freezegun import freeze_time
from app.dao.templates_dao import dao_update_template
from tests import create_authorization_header
from tests.app.conftest import (
sample_template as create_sample_template,
sample_notification,
sample_notification_history,
sample_email_template
)
from tests.app.conftest import sample_notification
def test_get_all_template_statistics_with_bad_arg_returns_400(client, sample_service):
auth_header = create_authorization_header()
# get_template_statistics_for_service_by_day
response = client.get(
'/service/{}/template-statistics'.format(sample_service.id),
headers=[('Content-Type', 'application/json'), auth_header],
query_string={'limit_days': 'blurk'}
@pytest.mark.parametrize('query_string', [
{},
{'limit_days': 0},
{'limit_days': 8},
{'limit_days': 3.5},
{'limit_days': 'blurk'},
])
def get_template_statistics_for_service_by_day_with_bad_arg_returns_400(admin_request, query_string):
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_service_by_day',
service_id=uuid.uuid4(),
**query_string,
_expected_status=400
)
assert response.status_code == 400
json_resp = json.loads(response.get_data(as_text=True))
assert json_resp['result'] == 'error'
assert json_resp['message'] == {'limit_days': ['blurk is not an integer']}
assert 'limit_days' in json_resp['message']
@freeze_time('2016-08-18')
def test_get_template_statistics_for_service(notify_db, notify_db_session, client, mocker):
email, sms = set_up_notifications(notify_db, notify_db_session)
mocked_redis = mocker.patch('app.redis_store.get_all_from_hash')
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics'.format(email.service_id),
headers=[('Content-Type', 'application/json'), auth_header]
def test_get_template_statistics_for_service_by_day_returns_template_info(admin_request, mocker, sample_notification):
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_service_by_day',
service_id=sample_notification.service_id,
limit_days=1
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))
assert len(json_resp['data']) == 2
assert json_resp['data'][0]['count'] == 3
assert json_resp['data'][0]['template_id'] == str(email.id)
assert json_resp['data'][0]['template_name'] == email.name
assert json_resp['data'][0]['template_type'] == email.template_type
assert json_resp['data'][1]['count'] == 3
assert json_resp['data'][1]['template_id'] == str(sms.id)
assert json_resp['data'][1]['template_name'] == sms.name
assert json_resp['data'][1]['template_type'] == sms.template_type
assert len(json_resp) == 1
mocked_redis.assert_not_called()
assert json_resp['data'][0]['count'] == 1
assert json_resp['data'][0]['template_id'] == str(sample_notification.template_id)
assert json_resp['data'][0]['template_name'] == 'Template Name'
assert json_resp['data'][0]['template_type'] == 'sms'
@freeze_time('2016-08-18')
def test_get_template_statistics_for_service_limited_1_day(notify_db, notify_db_session, client,
mocker):
email, sms = set_up_notifications(notify_db, notify_db_session)
mock_redis = mocker.patch('app.redis_store.get_all_from_hash')
def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_available(admin_request, mocker):
assert False
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics'.format(email.service_id),
headers=[('Content-Type', 'application/json'), auth_header],
query_string={'limit_days': 1}
def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis(admin_request, mocker):
assert False
def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days(admin_request, mocker):
assert False
def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_templates(
admin_request,
mocker,
sample_service
):
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_service_by_day',
service_id=sample_service.id,
limit_days=1
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))['data']
assert len(json_resp) == 2
assert json_resp[0]['count'] == 1
assert json_resp[0]['template_id'] == str(email.id)
assert json_resp[0]['template_name'] == email.name
assert json_resp[0]['template_type'] == email.template_type
assert json_resp[1]['count'] == 1
assert json_resp[1]['template_id'] == str(sms.id)
assert json_resp[1]['template_name'] == sms.name
assert json_resp[1]['template_type'] == sms.template_type
mock_redis.assert_not_called()
@pytest.mark.parametrize("cache_values", [False, True])
@freeze_time('2016-08-18')
def test_get_template_statistics_for_service_limit_7_days(notify_db, notify_db_session, client,
mocker,
cache_values):
email, sms = set_up_notifications(notify_db, notify_db_session)
mock_cache_values = {str.encode(str(sms.id)): str.encode('3'),
str.encode(str(email.id)): str.encode('3')} if cache_values else None
mocked_redis_get = mocker.patch('app.redis_store.get_all_from_hash', return_value=mock_cache_values)
mocked_redis_set = mocker.patch('app.redis_store.set_hash_and_expire')
auth_header = create_authorization_header()
response_for_a_week = client.get(
'/service/{}/template-statistics'.format(email.service_id),
headers=[('Content-Type', 'application/json'), auth_header],
query_string={'limit_days': 7}
)
assert response_for_a_week.status_code == 200
json_resp = json.loads(response_for_a_week.get_data(as_text=True))
assert len(json_resp['data']) == 2
assert json_resp['data'][0]['count'] == 3
assert json_resp['data'][0]['template_name'] == 'New Email Template Name'
assert json_resp['data'][1]['count'] == 3
assert json_resp['data'][1]['template_name'] == 'New SMS Template Name'
mocked_redis_get.assert_called_once_with("{}-template-counter-limit-7-days".format(email.service_id))
if cache_values:
mocked_redis_set.assert_not_called()
else:
mocked_redis_set.assert_called_once_with("{}-template-counter-limit-7-days".format(email.service_id),
{sms.id: 3, email.id: 3}, 600)
@freeze_time('2016-08-18')
def test_get_template_statistics_for_service_limit_30_days(notify_db, notify_db_session, client,
mocker):
email, sms = set_up_notifications(notify_db, notify_db_session)
mock_redis = mocker.patch('app.redis_store.get_all_from_hash')
auth_header = create_authorization_header()
response_for_a_month = client.get(
'/service/{}/template-statistics'.format(email.service_id),
headers=[('Content-Type', 'application/json'), auth_header],
query_string={'limit_days': 30}
)
assert response_for_a_month.status_code == 200
json_resp = json.loads(response_for_a_month.get_data(as_text=True))
assert len(json_resp['data']) == 2
assert json_resp['data'][0]['count'] == 3
assert json_resp['data'][0]['template_name'] == 'New Email Template Name'
assert json_resp['data'][1]['count'] == 3
assert json_resp['data'][1]['template_name'] == 'New SMS Template Name'
mock_redis.assert_not_called()
@freeze_time('2016-08-18')
def test_get_template_statistics_for_service_no_limit(notify_db, notify_db_session, client,
mocker):
email, sms = set_up_notifications(notify_db, notify_db_session)
mock_redis = mocker.patch('app.redis_store.get_all_from_hash')
auth_header = create_authorization_header()
response_for_all = client.get(
'/service/{}/template-statistics'.format(email.service_id),
headers=[('Content-Type', 'application/json'), auth_header]
)
assert response_for_all.status_code == 200
json_resp = json.loads(response_for_all.get_data(as_text=True))
assert len(json_resp['data']) == 2
assert json_resp['data'][0]['count'] == 3
assert json_resp['data'][0]['template_name'] == 'New Email Template Name'
assert json_resp['data'][1]['count'] == 3
assert json_resp['data'][1]['template_name'] == 'New SMS Template Name'
mock_redis.assert_not_called()
def set_up_notifications(notify_db, notify_db_session):
sms = create_sample_template(notify_db, notify_db_session)
email = sample_email_template(notify_db, notify_db_session)
today = datetime.now()
a_week_ago = datetime.now() - timedelta(days=7)
a_month_ago = datetime.now() - timedelta(days=30)
sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=sms)
sample_notification(notify_db, notify_db_session, created_at=a_month_ago, template=email)
email.name = 'Updated Email Template Name'
dao_update_template(email)
sms.name = 'Updated SMS Template Name'
dao_update_template(sms)
sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=sms)
sample_notification(notify_db, notify_db_session, created_at=a_week_ago, template=email)
email.name = 'New Email Template Name'
dao_update_template(email)
sms.name = 'New SMS Template Name'
dao_update_template(sms)
sample_notification(notify_db, notify_db_session, created_at=today, template=sms)
sample_notification(notify_db, notify_db_session, created_at=today, template=email)
return email, sms
@freeze_time('2016-08-18')
def test_returns_empty_list_if_no_templates_used(client, sample_service, mocker):
auth_header = create_authorization_header()
mock_redis = mocker.patch('app.redis_store.set_hash_and_expire')
response = client.get(
'/service/{}/template-statistics'.format(sample_service.id),
headers=[('Content-Type', 'application/json'), auth_header]
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))
assert len(json_resp['data']) == 0
mock_redis.assert_not_called()
# get_template_statistics_for_template
def test_get_template_statistics_by_id_returns_last_notification(
def test_get_template_statistics_for_template_returns_last_notification(
notify_db,
notify_db_session,
client):
admin_request):
sample_notification(notify_db, notify_db_session)
sample_notification(notify_db, notify_db_session)
notification_3 = sample_notification(notify_db, notify_db_session)
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics/{}'.format(notification_3.service_id, notification_3.template_id),
headers=[('Content-Type', 'application/json'), auth_header],
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_template_id',
service_id=notification_3.service_id,
template_id=notification_3.template_id
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))['data']
assert json_resp['id'] == str(notification_3.id)
assert json_resp['data']['id'] == str(notification_3.id)
def test_get_template_statistics_for_template_returns_empty_if_no_statistics(
client,
admin_request,
sample_template,
):
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics/{}'.format(sample_template.service_id, sample_template.id),
headers=[('Content-Type', 'application/json'), auth_header],
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_template_id',
service_id=sample_template.service_id,
template_id=sample_template.id
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))
assert not json_resp['data']
def test_get_template_statistics_raises_error_for_nonexistent_template(
client,
def test_get_template_statistics_for_template_raises_error_for_nonexistent_template(
admin_request,
sample_service,
fake_uuid
):
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics/{}'.format(sample_service.id, fake_uuid),
headers=[('Content-Type', 'application/json'), auth_header],
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_template_id',
service_id=sample_service.id,
template_id=fake_uuid,
_expected_status=404
)
assert response.status_code == 404
json_resp = json.loads(response.get_data(as_text=True))
assert json_resp['message'] == 'No result found'
assert json_resp['result'] == 'error'
def test_get_template_statistics_by_id_returns_empty_for_old_notification(
notify_db,
notify_db_session,
client,
sample_template
def test_get_template_statistics_for_template_returns_empty_for_old_notification(
admin_request,
sample_notification_history
):
sample_notification_history(notify_db, notify_db_session, sample_template)
auth_header = create_authorization_header()
response = client.get(
'/service/{}/template-statistics/{}'.format(sample_template.service.id, sample_template.id),
headers=[('Content-Type', 'application/json'), auth_header],
json_resp = admin_request.get(
'template_statistics.get_template_statistics_for_template_id',
service_id=sample_notification_history.service_id,
template_id=sample_notification_history.template_id
)
assert response.status_code == 200
json_resp = json.loads(response.get_data(as_text=True))['data']
assert not json_resp
assert not json_resp['data']

View File

@@ -8,7 +8,8 @@ from app.utils import (
get_midnight_for_day_before,
convert_utc_to_bst,
convert_bst_to_utc,
days_ago
days_ago,
last_n_days
)
@@ -66,3 +67,16 @@ def test_convert_bst_to_utc():
def test_days_ago(current_time, expected_datetime):
with freeze_time(current_time):
assert days_ago(1) == expected_datetime
def test_last_n_days():
with freeze_time('2018-03-27 12:00'):
res = last_n_days(5)
assert res == [
datetime(2018, 3, 23, 0, 0),
datetime(2018, 3, 24, 0, 0),
datetime(2018, 3, 25, 0, 0),
datetime(2018, 3, 26, 0, 0),
datetime(2018, 3, 27, 0, 0)
]