From 7dc34fc3a46cdeae87b840b8e617f8712755ecd8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 13 Apr 2018 14:21:10 +0100 Subject: [PATCH] set 0'd dict in redis if no data exists in redis to get the data for a day can be reasonably slow (a few hundred milliseconds), and if someone's viewing a service with no activity we don't want to do that query seven times every two seconds. So if there is no data in redis, when we get the data out of the database, we should put it in redis so we can just grab it from there next time. This'll happen in two cases: * redis data is deleted * the service sent no messages that day additionally, make sure that we convert nicely from redis' return values (ascii strings) to unicode keys and integer counts. --- app/dao/notifications_dao.py | 6 +- app/template_statistics/rest.py | 23 +++++- .../test_notification_dao_template_usage.py | 13 ++-- tests/app/template_statistics/test_rest.py | 72 ++++++++++++------- 4 files changed, 81 insertions(+), 33 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 953b678c2..b80f9d26b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -81,10 +81,12 @@ def dao_get_template_usage(service_id, *, limit_days=None, day=None): Template.name, Template.template_type, Template.is_precompiled_letter, - notifications_aggregate_query.c.count - ).join( + func.coalesce(notifications_aggregate_query.c.count, 0).label('count') + ).outerjoin( notifications_aggregate_query, notifications_aggregate_query.c.template_id == Template.id + ).filter( + Template.service_id == service_id ).order_by(Template.name) return query.all() diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 7e9297cff..f992fe90d 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -1,7 +1,8 @@ from flask import ( Blueprint, jsonify, - request + request, + current_app ) from app import redis_store @@ -61,15 +62,28 @@ def get_template_statistics_for_last_n_days(service_id, limit_days): template_stats_by_id = Counter() for day in last_n_days(limit_days): + print('\n') # "{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: + if stats: + stats = { + k.decode('utf-8'): int(v) for k, v in stats.items() + } + else: # key didn't exist (or redis was down) - lets populate from DB. stats = { str(row.id): row.count for row in dao_get_template_usage(service_id, day=day) } - + # if there is data in db, but not in redis - lets put it in redis so we don't have to do + # this calc again next time. If there isn't any data, we can't put it in redis. + # Zero length hashes aren't a thing in redis. (There'll only be no data if the service has no templates) + if stats: + redis_store.set_hash_and_expire( + key, + stats, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) template_stats_by_id += Counter(stats) # attach count from stats to name/type/etc from database @@ -83,4 +97,7 @@ def get_template_statistics_for_last_n_days(service_id, limit_days): 'is_precompiled_letter': template.is_precompiled_letter } for template in template_details + # we don't want to return templates with no count to the front-end, + # but they're returned from the DB and might be put in redis like that (if there was no data that day) + if template_stats_by_id[str(template.id)] != 0 ] diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py index 8f44f4996..e66d50955 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py +++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py @@ -114,23 +114,28 @@ def test_template_usage_should_filter_by_service(notify_db_session): template_1 = create_template(service_1) template_2 = create_template(service_2) # noqa - template_3 = create_template(service_3) + template_3a = create_template(service_3) + template_3b = create_template(service_3) # noqa # two for service_1, one for service_3 create_notification(template_1) create_notification(template_1) - create_notification(template_3) + create_notification(template_3a) res1 = dao_get_template_usage(service_1.id, limit_days=1) res2 = dao_get_template_usage(service_2.id, limit_days=1) res3 = dao_get_template_usage(service_3.id, limit_days=1) assert len(res1) == 1 - assert len(res2) == 0 - assert len(res3) == 1 assert res1[0].count == 2 + + assert len(res2) == 1 + assert res2[0].count == 0 + + assert len(res3) == 2 assert res3[0].count == 1 + assert res3[1].count == 0 def test_template_usage_should_by_able_to_get_zero_count_from_notifications_history_if_no_rows(sample_service): diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index cfedd8e9e..0f1a652a9 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -3,6 +3,7 @@ from datetime import datetime from unittest.mock import Mock, call, ANY import pytest +from flask import current_app from freezegun import freeze_time from tests.app.db import ( @@ -11,8 +12,24 @@ from tests.app.db import ( ) +def set_up_get_all_from_hash(mock_redis, side_effect): + """ + redis returns binary strings for both keys and values - so given a list of side effects (return values), + make sure + """ + assert type(side_effect) == list + side_effects = [] + for ret_val in side_effect: + if ret_val is None: + side_effects.append(None) + else: + side_effects += [{str(k).encode('utf-8'): str(v).encode('utf-8') for k, v in ret_val.items()}] + + mock_redis.get_all_from_hash.side_effect = side_effects + # get_template_statistics_for_service_by_day + @pytest.mark.parametrize('query_string', [ {}, {'limit_days': 0}, @@ -54,9 +71,9 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab sample_template ): mock_redis = mocker.patch('app.template_statistics.rest.redis_store') - mock_redis.get_all_from_hash.return_value = { - str(sample_template.id): 3 - } + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 3} + ]) json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', @@ -68,7 +85,7 @@ def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_availab assert json_resp['data'][0]['count'] == 3 assert json_resp['data'][0]['template_id'] == str(sample_template.id) mock_redis.get_all_from_hash.assert_called_once_with( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-01') + 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01') ) @@ -81,10 +98,10 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(sample_template.id): 2}, + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 2}, None - ] + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -102,18 +119,20 @@ def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( assert json_resp['data'][0]['count'] == 5 assert json_resp['data'][0]['template_id'] == str(sample_template.id) # first redis call - assert mock_redis.mock_calls == [ - call.get_all_from_hash( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-01') - ), - call.get_all_from_hash( - "service-{}-template-usage-{}".format(sample_template.service_id, '2018-01-02') - ) + assert mock_redis.get_all_from_hash.mock_calls == [ + call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01')), + call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02')) ] # dao only called for 2nd, since redis returned values for first call mock_dao.assert_called_once_with( str(sample_template.service_id), day=datetime(2018, 1, 2) ) + mock_redis.set_hash_and_expire.assert_called_once_with( + 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02'), + # sets the data that the dao returned + {str(sample_template.id): 3}, + current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] + ) def test_get_template_statistics_for_service_by_day_combines_templates_correctly( @@ -127,11 +146,11 @@ def test_get_template_statistics_for_service_by_day_combines_templates_correctly mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(t1.id): 2}, + set_up_get_all_from_hash(mock_redis, [ + {t1.id: 2}, None, - {str(t1.id): 1, str(t2.id): 4}, - ] + {t1.id: 1, t2.id: 4}, + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -165,15 +184,15 @@ def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - mock_redis.get_all_from_hash.side_effect = [ - {str(sample_template.id): 1}, + set_up_get_all_from_hash(mock_redis, [ + {sample_template.id: 1}, None, - {str(sample_template.id): 1}, - {str(sample_template.id): 1}, - {str(sample_template.id): 1}, + {sample_template.id: 1}, + {sample_template.id: 1}, + {sample_template.id: 1}, None, None, - ] + ]) mock_dao = mocker.patch( 'app.template_statistics.rest.dao_get_template_usage', return_value=[ @@ -213,6 +232,8 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem mocker, sample_service ): + mock_redis = mocker.patch('app.template_statistics.rest.redis_store') + json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_service.id, @@ -220,6 +241,9 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem ) assert len(json_resp['data']) == 0 + assert mock_redis.get_all_from_hash.call_count == 7 + # make sure we don't try and set any empty hashes in redis + assert mock_redis.set_hash_and_expire.call_count == 0 # get_template_statistics_for_template