From bfb8528ea907712da6c124e4ebc9b69725f5071e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 10:58:06 +0100 Subject: [PATCH 1/3] The /platform-admin takes a long time, probably because the marshmallow schema used joins to the service table to return all the service data and is inefficient. The query itself has not been improved much at all but by not using a marshmallow schema I hope to get the performance gain I am looking for. --- app/dao/services_dao.py | 29 +++++++++++++++++++++++++++++ app/service/rest.py | 21 +++++++++++++++++++-- tests/app/service/test_rest.py | 27 +++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5ae25c0e6..1082f4576 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -412,6 +412,35 @@ def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_fro return query.all() +@statsd(namespace='dao') +def fetch_aggregate_stats_by_date_range_for_all_services(start_date, end_date, include_from_test_key=True): + start_date = get_london_midnight_in_utc(start_date) + end_date = get_london_midnight_in_utc(end_date + timedelta(days=1)) + table = NotificationHistory + + if start_date >= datetime.utcnow() - timedelta(days=7): + table = Notification + + query = db.session.query( + table.notification_type, + table.status, + func.count(table.id).label('count') + ).filter( + table.created_at >= start_date, + table.created_at < end_date + ).group_by( + table.notification_type, + table.status + ).order_by( + table.notification_type + ) + + if not include_from_test_key: + query = query.filter(table.key_type != KEY_TYPE_TEST) + + return query.all() + + @transactional @version_class(Service) @version_class(ApiKey) diff --git a/app/service/rest.py b/app/service/rest.py index 3122484f6..4790e0113 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -46,8 +46,8 @@ from app.dao.services_dao import ( dao_suspend_service, dao_resume_service, dao_fetch_monthly_historical_stats_for_service, - dao_fetch_monthly_historical_stats_by_template_for_service -) + dao_fetch_monthly_historical_stats_by_template_for_service, + fetch_aggregate_stats_by_date_range_for_all_services) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, @@ -103,6 +103,23 @@ service_blueprint = Blueprint('service', __name__) register_errors(service_blueprint) +@service_blueprint.route('/platform-stats', methods=['GET']) +def get_platform_stats(): + include_from_test_key = request.args.get('include_from_test_key', 'True') != 'False' + + # If start and end date are not set, we are expecting today's stats. + today = str(datetime.utcnow().date()) + + start_date = datetime.strptime(request.args.get('start_date', today), '%Y-%m-%d').date() + end_date = datetime.strptime(request.args.get('end_date', today), '%Y-%m-%d').date() + data = fetch_aggregate_stats_by_date_range_for_all_services(start_date=start_date, + end_date=end_date, + include_from_test_key=include_from_test_key + ) + result = jsonify(data) + return result + + @service_blueprint.route('', methods=['GET']) def get_services(): only_active = request.args.get('only_active') == 'True' diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7db38221a..0ce87e341 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2771,3 +2771,30 @@ def test_get_service_sms_senders_for_service_returns_empty_list_when_service_doe ) assert response.status_code == 200 assert json.loads(response.get_data(as_text=True)) == [] + + +def test_get_platform_stats(client, notify_db_session): + service_1 = create_service(service_name='Service 1') + service_2 = create_service(service_name='Service 2') + sms_template = create_template(service=service_1) + email_template = create_template(service=service_2, template_type=EMAIL_TYPE) + letter_template = create_template(service=service_2, template_type=LETTER_TYPE) + create_notification(template=sms_template, status='sending') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=email_template, status='temporary-failure') + create_notification(template=email_template, status='delivered') + create_notification(template=letter_template, status='sending') + create_notification(template=letter_template, status='sending') + + response = client.get('/service/platform-stats', + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp == [['email', 'delivered', 1], + ['email', 'temporary-failure', 1], + ['sms', 'delivered', 3], + ['sms', 'sending', 1], + ['letter', 'sending', 2]] From f1f2e5cd90b9af871d339ba1be8d4bf7d9af3efe Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 15:06:11 +0100 Subject: [PATCH 2/3] Fix the results to be returned in the same format that the admin app expects. --- app/service/rest.py | 4 +++- tests/app/service/test_rest.py | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index 4790e0113..3d2dc2827 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -116,7 +116,9 @@ def get_platform_stats(): end_date=end_date, include_from_test_key=include_from_test_key ) - result = jsonify(data) + stats = statistics.format_statistics(data) + + result = jsonify(stats) return result diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 0ce87e341..72cfb2cd8 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2798,3 +2798,28 @@ def test_get_platform_stats(client, notify_db_session): ['sms', 'delivered', 3], ['sms', 'sending', 1], ['letter', 'sending', 2]] + + +def test_get_platform_stats_creates_zero_stats(client, notify_db_session): + service_1 = create_service(service_name='Service 1') + service_2 = create_service(service_name='Service 2') + sms_template = create_template(service=service_1) + email_template = create_template(service=service_2, template_type=EMAIL_TYPE) + letter_template = create_template(service=service_2, template_type=LETTER_TYPE) + create_notification(template=sms_template, status='sending') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=sms_template, status='delivered') + create_notification(template=email_template, status='temporary-failure') + create_notification(template=email_template, status='delivered') + + response = client.get('/service/platform-stats', + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + print(json_resp) + assert json_resp == {'email': {'failed': 1, 'requested': 2, 'delivered': 1}, + 'sms': {'failed': 0, 'requested': 4, 'delivered': 3}, + 'letter': {'failed': 0, 'requested': 0, 'delivered': 0} + } From 5ce44c07f5e74536250612d0aad65d280e8130e1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 16:07:10 +0100 Subject: [PATCH 3/3] Fix unit tests. --- tests/app/service/test_rest.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 72cfb2cd8..169fbe134 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2793,11 +2793,9 @@ def test_get_platform_stats(client, notify_db_session): ) assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp == [['email', 'delivered', 1], - ['email', 'temporary-failure', 1], - ['sms', 'delivered', 3], - ['sms', 'sending', 1], - ['letter', 'sending', 2]] + assert json_resp['email'] == {'delivered': 1, 'requested': 2, 'failed': 1} + assert json_resp['letter'] == {'delivered': 0, 'requested': 2, 'failed': 0} + assert json_resp['sms'] == {'delivered': 3, 'requested': 4, 'failed': 0} def test_get_platform_stats_creates_zero_stats(client, notify_db_session): @@ -2805,7 +2803,6 @@ def test_get_platform_stats_creates_zero_stats(client, notify_db_session): service_2 = create_service(service_name='Service 2') sms_template = create_template(service=service_1) email_template = create_template(service=service_2, template_type=EMAIL_TYPE) - letter_template = create_template(service=service_2, template_type=LETTER_TYPE) create_notification(template=sms_template, status='sending') create_notification(template=sms_template, status='delivered') create_notification(template=sms_template, status='delivered') @@ -2818,8 +2815,6 @@ def test_get_platform_stats_creates_zero_stats(client, notify_db_session): ) assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) - print(json_resp) - assert json_resp == {'email': {'failed': 1, 'requested': 2, 'delivered': 1}, - 'sms': {'failed': 0, 'requested': 4, 'delivered': 3}, - 'letter': {'failed': 0, 'requested': 0, 'delivered': 0} - } + assert json_resp['email'] == {'failed': 1, 'requested': 2, 'delivered': 1} + assert json_resp['letter'] == {'failed': 0, 'requested': 0, 'delivered': 0} + assert json_resp['sms'] == {'failed': 0, 'requested': 4, 'delivered': 3}