From 89f4f5173e0c225bfabc14856a8f1b3e67798063 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 23 Aug 2017 15:04:37 +0100 Subject: [PATCH 1/5] refactor performance platform code so that it doesn't appear generic when it's actually specific to sending the daily notification totals. To do this, split it out into a separate performance_platform directory, containing the business logic, and make the performance_platform_client incredibly thin - all it handles is adding ids to payloads, and sending stats. Also, some changes to the config (not all done yet) since there is one token per endpoint, not one for the whole platform as we'd previously coded --- app/celery/scheduled_tasks.py | 7 +- .../performance_platform_client.py | 87 ++++------- app/config.py | 2 +- app/performance_platform/__init__.py | 0 .../total_sent_notifications.py | 44 ++++++ tests/app/celery/test_scheduled_tasks.py | 4 +- .../app/clients/test_performance_platform.py | 138 +++++------------- .../test_total_sent_notifications.py | 77 ++++++++++ 8 files changed, 193 insertions(+), 166 deletions(-) create mode 100644 app/performance_platform/__init__.py create mode 100644 app/performance_platform/total_sent_notifications.py create mode 100644 tests/app/performance_platform/test_total_sent_notifications.py diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 5cc35f257..c7bda0f74 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -8,6 +8,7 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from app import notify_celery +from app.performance_platform import total_sent_notifications from app import performance_platform_client from app.dao.date_util import get_month_start_and_end_date_in_utc from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago @@ -175,7 +176,7 @@ def timeout_notifications(): @statsd(namespace="tasks") def send_daily_performance_platform_stats(): if performance_platform_client.active: - count_dict = performance_platform_client.get_total_sent_notifications_yesterday() + count_dict = total_sent_notifications.get_total_sent_notifications_yesterday() email_sent_count = count_dict.get('email').get('count') sms_sent_count = count_dict.get('sms').get('count') start_date = count_dict.get('start_date') @@ -185,14 +186,14 @@ def send_daily_performance_platform_stats(): .format(start_date, email_sent_count, sms_sent_count) ) - performance_platform_client.send_performance_stats( + total_sent_notifications.send_total_notifications_sent_for_day_stats( start_date, 'sms', sms_sent_count, 'day' ) - performance_platform_client.send_performance_stats( + total_sent_notifications.send_total_notifications_sent_for_day_stats( start_date, 'email', email_sent_count, diff --git a/app/clients/performance_platform/performance_platform_client.py b/app/clients/performance_platform/performance_platform_client.py index 045991181..b0f400a8c 100644 --- a/app/clients/performance_platform/performance_platform_client.py +++ b/app/clients/performance_platform/performance_platform_client.py @@ -1,11 +1,8 @@ import base64 import json -from datetime import datetime -import requests from flask import current_app - -from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc, convert_utc_to_bst +import requests class PerformancePlatformClient: @@ -14,72 +11,40 @@ class PerformancePlatformClient: def active(self): return self._active - @active.setter - def active(self, value): - self._active = value - def init_app(self, app): self._active = app.config.get('PERFORMANCE_PLATFORM_ENABLED') if self.active: - self.bearer_token = app.config.get('PERFORMANCE_PLATFORM_TOKEN') self.performance_platform_url = app.config.get('PERFORMANCE_PLATFORM_URL') + self.performance_platform_endpoints = app.config.get('PERFORMANCE_PLATFORM_ENDPOINTS') - def send_performance_stats(self, date, channel, count, period): + def send_stats_to_performance_platform(self, dataset, payload): if self.active: - payload = { - '_timestamp': convert_utc_to_bst(date).isoformat(), - 'service': 'govuk-notify', - 'channel': channel, - 'count': count, - 'dataType': 'notifications', - 'period': period + bearer_token = self.performance_platform_endpoints[dataset] + headers = { + 'Content-Type': "application/json", + 'Authorization': 'Bearer {}'.format(bearer_token) } - self._add_id_for_payload(payload) - self._send_stats_to_performance_platform(payload) - - def get_total_sent_notifications_yesterday(self): - today = datetime.utcnow() - start_date = get_midnight_for_day_before(today) - end_date = get_london_midnight_in_utc(today) - - from app.dao.notifications_dao import get_total_sent_notifications_in_date_range - email_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email') - sms_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'sms') - - return { - "start_date": start_date, - "email": { - "count": email_count - }, - "sms": { - "count": sms_count - } - } - - def _send_stats_to_performance_platform(self, payload): - headers = { - 'Content-Type': "application/json", - 'Authorization': 'Bearer {}'.format(self.bearer_token) - } - resp = requests.post( - self.performance_platform_url, - json=payload, - headers=headers - ) - - if resp.status_code == 200: - current_app.logger.info( - "Updated performance platform successfully with payload {}".format(json.dumps(payload)) - ) - else: - current_app.logger.error( - "Performance platform update request failed for payload with response details: {} '{}'".format( - json.dumps(payload), - resp.status_code, - resp.json()) + resp = requests.post( + self.performance_platform_url + dataset, + json=payload, + headers=headers ) - def _add_id_for_payload(self, payload): + if resp.status_code == 200: + current_app.logger.info( + "Updated performance platform successfully with payload {}".format(json.dumps(payload)) + ) + else: + current_app.logger.error( + "Performance platform update request failed for payload with response details: {} '{}'".format( + json.dumps(payload), + resp.status_code + ) + ) + resp.raise_for_status() + + @staticmethod + def add_id_to_payload(payload): payload_string = '{}{}{}{}{}'.format( payload['_timestamp'], payload['service'], diff --git a/app/config.py b/app/config.py index 958a0a532..d98e9fa1a 100644 --- a/app/config.py +++ b/app/config.py @@ -93,7 +93,7 @@ class Config(object): # Performance platform PERFORMANCE_PLATFORM_ENABLED = False - PERFORMANCE_PLATFORM_URL = 'https://www.performance.service.gov.uk/data/govuk-notify/notifications' + PERFORMANCE_PLATFORM_URL = 'https://www.performance.service.gov.uk/data/govuk-notify/' PERFORMANCE_PLATFORM_TOKEN = os.getenv('PERFORMANCE_PLATFORM_TOKEN') # Logging diff --git a/app/performance_platform/__init__.py b/app/performance_platform/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py new file mode 100644 index 000000000..774f300e6 --- /dev/null +++ b/app/performance_platform/total_sent_notifications.py @@ -0,0 +1,44 @@ +from datetime import datetime + +from app import performance_platform_client +from app.dao.notifications_dao import get_total_sent_notifications_in_date_range +from app.utils import ( + get_london_midnight_in_utc, + get_midnight_for_day_before, + convert_utc_to_bst, +) + + +def send_total_notifications_sent_for_day_stats(date, channel, count, period): + payload = { + '_timestamp': convert_utc_to_bst(date).isoformat(), + 'service': 'govuk-notify', + 'channel': channel, + 'count': count, + 'dataType': 'notifications', + 'period': period + } + performance_platform_client.add_id_to_payload(payload) + + performance_platform_client.send_stats_to_performance_platform( + dataset='notifications', + payload=payload + ) + +def get_total_sent_notifications_yesterday(): + today = datetime.utcnow() + start_date = get_midnight_for_day_before(today) + end_date = get_london_midnight_in_utc(today) + + email_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email') + sms_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'sms') + + return { + "start_date": start_date, + "email": { + "count": email_count + }, + "sms": { + "count": sms_count + } + } diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 78d46a54b..9a2454a8f 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -276,7 +276,7 @@ def test_send_daily_performance_stats_calls_does_not_send_if_inactive( sample_template, mocker ): - send_mock = mocker.patch('app.celery.scheduled_tasks.performance_platform_client.send_performance_stats') + send_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') with patch.object( PerformancePlatformClient, @@ -296,7 +296,7 @@ def test_send_daily_performance_stats_calls_with_correct_totals( sample_template, mocker ): - perf_mock = mocker.patch('app.celery.scheduled_tasks.performance_platform_client.send_performance_stats') + perf_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') notification_history = partial( create_notification_history, diff --git a/tests/app/clients/test_performance_platform.py b/tests/app/clients/test_performance_platform.py index 9913048f8..77bfad9d8 100644 --- a/tests/app/clients/test_performance_platform.py +++ b/tests/app/clients/test_performance_platform.py @@ -1,119 +1,59 @@ +import requests import requests_mock import pytest -from datetime import datetime -from freezegun import freeze_time -from functools import partial from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient -from app.utils import ( - get_london_midnight_in_utc, - get_midnight_for_day_before -) -from tests.app.conftest import sample_notification_history as create_notification_history @pytest.fixture(scope='function') -def client(mocker): - client = PerformancePlatformClient() +def perf_client(mocker): + perf_client = PerformancePlatformClient() current_app = mocker.Mock(config={ 'PERFORMANCE_PLATFORM_ENABLED': True, - 'PERFORMANCE_PLATFORM_URL': 'https://performance-platform-url/', - 'PERFORMANCE_PLATFORM_TOKEN': 'token' + 'PERFORMANCE_PLATFORM_ENDPOINTS': { + 'foo': 'my_token', + 'bar': 'other_token' + }, + 'PERFORMANCE_PLATFORM_URL': 'https://performance-platform-url/' }) - client.init_app(current_app) - return client + perf_client.init_app(current_app) + return perf_client -def test_should_not_call_if_not_enabled(notify_api, client, mocker): - mocker.patch.object(client, '_send_stats_to_performance_platform') - client.active = False - client.send_performance_stats( - date=datetime(2016, 10, 16, 0, 0, 0), - channel='sms', - count=142, - period='day' - ) - - client._send_stats_to_performance_platform.assert_not_called() - - -def test_should_call_if_enabled(notify_api, client, mocker): - mocker.patch.object(client, '_send_stats_to_performance_platform') - client.send_performance_stats( - date=datetime(2016, 10, 16, 0, 0, 0), - channel='sms', - count=142, - period='day' - ) - - assert client._send_stats_to_performance_platform.call_count == 1 - - -def test_send_platform_stats_creates_correct_call(notify_api, client): +def test_should_not_call_if_not_enabled(perf_client): with requests_mock.Mocker() as request_mock: - request_mock.post( - client.performance_platform_url, - json={}, - status_code=200 - ) - client.send_performance_stats( - date=datetime(2016, 10, 15, 23, 0, 0), - channel='sms', - count=142, - period='day' - ) + request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) + perf_client._active = False + perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) + + assert request_mock.called is False + + + +def test_should_call_if_enabled(perf_client): + with requests_mock.Mocker() as request_mock: + request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) + perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) assert request_mock.call_count == 1 - - assert request_mock.request_history[0].url == client.performance_platform_url - assert request_mock.request_history[0].method == 'POST' - - request_args = request_mock.request_history[0].json() - assert request_args['dataType'] == 'notifications' - assert request_args['service'] == 'govuk-notify' - assert request_args['period'] == 'day' - assert request_args['channel'] == 'sms' - assert request_args['_timestamp'] == '2016-10-16T00:00:00' - assert request_args['count'] == 142 - expected_base64_id = 'MjAxNi0xMC0xNlQwMDowMDowMGdvdnVrLW5vdGlmeXNtc25vdGlmaWNhdGlvbnNkYXk=' - assert request_args['_id'] == expected_base64_id + assert request_mock.last_request.method == 'POST' -@freeze_time("2016-01-11 12:30:00") -def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( - notify_db, - notify_db_session, - client, - sample_template -): - notification_history = partial( - create_notification_history, - notify_db, - notify_db_session, - sample_template, - status='delivered' - ) +@pytest.mark.parametrize('dataset, token', [ + ('foo', 'my_token'), + ('bar', 'other_token') +]) +def test_should_use_correct_token(perf_client, dataset, token): + with requests_mock.Mocker() as request_mock: + request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) + request_mock.post('https://performance-platform-url/bar', json={}, status_code=200) + perf_client.send_stats_to_performance_platform(dataset=dataset, payload={}) - notification_history(notification_type='email') - notification_history(notification_type='sms') + assert request_mock.call_count == 1 + assert request_mock.last_request.headers.get('authorization') == 'Bearer {}'.format(token) - # Create some notifications for the day before - yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) - with freeze_time(yesterday): - notification_history(notification_type='sms') - notification_history(notification_type='sms') - notification_history(notification_type='email') - notification_history(notification_type='email') - notification_history(notification_type='email') - total_count_dict = client.get_total_sent_notifications_yesterday() - - assert total_count_dict == { - "start_date": get_midnight_for_day_before(datetime.utcnow()), - "email": { - "count": 3 - }, - "sms": { - "count": 2 - } - } +def test_should_raise_for_status(perf_client): + with pytest.raises(requests.HTTPError), requests_mock.Mocker() as request_mock: + request_mock.post('https://performance-platform-url/foo', json={}, status_code=403) + perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py new file mode 100644 index 000000000..04226b263 --- /dev/null +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -0,0 +1,77 @@ +from datetime import datetime +from functools import partial + +from freezegun import freeze_time + +from app.utils import get_midnight_for_day_before +from app.performance_platform.total_sent_notifications import( + send_total_notifications_sent_for_day_stats, + get_total_sent_notifications_yesterday +) + +from tests.app.conftest import ( + sample_notification_history as create_notification_history +) + + +def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call(mocker, client): + send_stats = mocker.patch('app.performance_platform.total_sent_notifications.performance_platform_client.send_stats_to_performance_platform') # noqa + + send_total_notifications_sent_for_day_stats( + date=datetime(2016, 10, 15, 23, 0, 0), + channel='sms', + count=142, + period='day' + ) + + assert send_stats.call_count == 1 + assert send_stats.call_args[1]['dataset'] == 'notifications' + + request_args = send_stats.call_args[1]['payload'] + assert request_args['dataType'] == 'notifications' + assert request_args['service'] == 'govuk-notify' + assert request_args['period'] == 'day' + assert request_args['channel'] == 'sms' + assert request_args['_timestamp'] == '2016-10-16T00:00:00' + assert request_args['count'] == 142 + expected_base64_id = 'MjAxNi0xMC0xNlQwMDowMDowMGdvdnVrLW5vdGlmeXNtc25vdGlmaWNhdGlvbnNkYXk=' + assert request_args['_id'] == expected_base64_id + + +@freeze_time("2016-01-11 12:30:00") +def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( + notify_db, + notify_db_session, + sample_template +): + notification_history = partial( + create_notification_history, + notify_db, + notify_db_session, + sample_template, + status='delivered' + ) + + notification_history(notification_type='email') + notification_history(notification_type='sms') + + # Create some notifications for the day before + yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) + with freeze_time(yesterday): + notification_history(notification_type='sms') + notification_history(notification_type='sms') + notification_history(notification_type='email') + notification_history(notification_type='email') + notification_history(notification_type='email') + + total_count_dict = get_total_sent_notifications_yesterday() + + assert total_count_dict == { + "start_date": get_midnight_for_day_before(datetime.utcnow()), + "email": { + "count": 3 + }, + "sms": { + "count": 2 + } + } From bd2682b5219a888951d19738c9e30b049320ad93 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 23 Aug 2017 18:11:44 +0100 Subject: [PATCH 2/5] add new performance-platform section to cf config it's a new cf-service we've got to create, that contains endpoints and the bearer tokens for them. --- app/cloudfoundry_config.py | 7 ++++- app/config.py | 4 +++ tests/app/celery/test_scheduled_tasks.py | 7 +---- tests/app/test_cloudfoundry_config.py | 37 +++++++++++++++--------- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index c890a8333..33cd4f753 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -34,6 +34,8 @@ def set_config_env_vars(vcap_services): extract_firetext_config(s) elif s['name'] == 'redis': extract_redis_config(s) + elif s['name'] == 'performance-platform': + extract_performance_platform_config(s) def extract_notify_config(notify_config): @@ -42,10 +44,13 @@ def extract_notify_config(notify_config): os.environ['ADMIN_CLIENT_SECRET'] = notify_config['credentials']['admin_client_secret'] os.environ['SECRET_KEY'] = notify_config['credentials']['secret_key'] os.environ['DANGEROUS_SALT'] = notify_config['credentials']['dangerous_salt'] - os.environ['PERFORMANCE_PLATFORM_TOKEN'] = notify_config['credentials'].get('performance_platform_token', '') os.environ['SMS_INBOUND_WHITELIST'] = json.dumps(notify_config['credentials']['allow_ip_inbound_sms']) +def extract_performance_platform_config(performance_platform_config): + os.environ['PERFORMANCE_PLATFORM_ENDPOINTS'] = json.dumps(performance_platform_config['credentials']) + + def extract_notify_aws_config(aws_config): os.environ['NOTIFICATION_QUEUE_PREFIX'] = aws_config['credentials']['sqs_queue_prefix'] os.environ['AWS_ACCESS_KEY_ID'] = aws_config['credentials']['aws_access_key_id'] diff --git a/app/config.py b/app/config.py index d98e9fa1a..c6c5291eb 100644 --- a/app/config.py +++ b/app/config.py @@ -273,6 +273,10 @@ class Config(object): SMS_INBOUND_WHITELIST = json.loads(os.environ.get('SMS_INBOUND_WHITELIST', '[]')) + # Format is as follows: + # {"dataset_1": "token_1", ...} + PERFORMANCE_PLATFORM_ENDPOINTS = json.loads(os.environ.get('PERFORMANCE_PLATFORM_ENDPOINTS', '{}')) + ###################### # Config overrides ### diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 9a2454a8f..d05423e00 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -270,12 +270,7 @@ def test_will_remove_csv_files_for_jobs_older_than_seven_days( ] -def test_send_daily_performance_stats_calls_does_not_send_if_inactive( - notify_db, - notify_db_session, - sample_template, - mocker -): +def test_send_daily_performance_stats_calls_does_not_send_if_inactive(mocker): send_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') with patch.object( diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index a393fc8bb..c299976d9 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -16,7 +16,6 @@ def notify_config(): 'admin_client_secret': 'admin client secret', 'secret_key': 'secret key', 'dangerous_salt': 'dangerous salt', - 'performance_platform_token': 'performance platform token', 'allow_ip_inbound_sms': ['111.111.111.111', '100.100.100.100'] } } @@ -87,6 +86,16 @@ def redis_config(): } } +@pytest.fixture +def performance_platform_config(): + return { + 'name': 'performance-platform', + 'credentials': { + 'foo': 'my_token', + 'bar': 'other_token' + } + } + @pytest.fixture def cloudfoundry_config( @@ -96,7 +105,8 @@ def cloudfoundry_config( hosted_graphite_config, mmg_config, firetext_config, - redis_config + redis_config, + performance_platform_config ): return { 'postgres': postgres_config, @@ -106,7 +116,8 @@ def cloudfoundry_config( hosted_graphite_config, mmg_config, firetext_config, - redis_config + redis_config, + performance_platform_config ] } @@ -148,16 +159,6 @@ def test_notify_config(): assert os.environ['ADMIN_CLIENT_SECRET'] == 'admin client secret' assert os.environ['SECRET_KEY'] == 'secret key' assert os.environ['DANGEROUS_SALT'] == 'dangerous salt' - assert os.environ['PERFORMANCE_PLATFORM_TOKEN'] == 'performance platform token' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_notify_config_if_perf_platform_not_set(cloudfoundry_config): - del cloudfoundry_config['user-provided'][0]['credentials']['performance_platform_token'] - - set_config_env_vars(cloudfoundry_config) - - assert os.environ['PERFORMANCE_PLATFORM_TOKEN'] == '' @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') @@ -205,3 +206,13 @@ def test_sms_inbound_config(): extract_cloudfoundry_config() assert os.environ['SMS_INBOUND_WHITELIST'] == json.dumps(['111.111.111.111', '100.100.100.100']) + + +@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') +def test_performance_platform_config(): + extract_cloudfoundry_config() + + assert os.environ['PERFORMANCE_PLATFORM_ENDPOINTS'] == json.dumps({ + 'foo': 'my_token', + 'bar': 'other_token' + }) From 1f93fc889ca33a729e3526f44e5684ef7fdb4290 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 23 Aug 2017 18:21:54 +0100 Subject: [PATCH 3/5] add new cf performance-platform service to manifest --- manifest-api-base.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/manifest-api-base.yml b/manifest-api-base.yml index 0a79d96bd..9eb4ceb70 100644 --- a/manifest-api-base.yml +++ b/manifest-api-base.yml @@ -1,4 +1,4 @@ ---- +#--- buildpack: python_buildpack command: scripts/run_app_paas.sh gunicorn -w 5 -b 0.0.0.0:$PORT wsgi @@ -10,6 +10,7 @@ services: - firetext - hosted-graphite - redis + - performance-platform env: NOTIFY_APP_NAME: public-api CW_APP_NAME: api From 412c87cfc8aeece522096a7541b65139383cd2db Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 24 Aug 2017 10:52:47 +0100 Subject: [PATCH 4/5] pycodestyle --- app/performance_platform/total_sent_notifications.py | 1 + tests/app/celery/test_scheduled_tasks.py | 4 ++-- tests/app/clients/test_performance_platform.py | 1 - .../app/performance_platform/test_total_sent_notifications.py | 2 +- tests/app/test_cloudfoundry_config.py | 1 + 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 774f300e6..9f455f113 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -25,6 +25,7 @@ def send_total_notifications_sent_for_day_stats(date, channel, count, period): payload=payload ) + def get_total_sent_notifications_yesterday(): today = datetime.utcnow() start_date = get_midnight_for_day_before(today) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index d05423e00..6064f1c79 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -271,7 +271,7 @@ def test_will_remove_csv_files_for_jobs_older_than_seven_days( def test_send_daily_performance_stats_calls_does_not_send_if_inactive(mocker): - send_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') + send_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa with patch.object( PerformancePlatformClient, @@ -291,7 +291,7 @@ def test_send_daily_performance_stats_calls_with_correct_totals( sample_template, mocker ): - perf_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') + perf_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa notification_history = partial( create_notification_history, diff --git a/tests/app/clients/test_performance_platform.py b/tests/app/clients/test_performance_platform.py index 77bfad9d8..aba87b9e7 100644 --- a/tests/app/clients/test_performance_platform.py +++ b/tests/app/clients/test_performance_platform.py @@ -29,7 +29,6 @@ def test_should_not_call_if_not_enabled(perf_client): assert request_mock.called is False - def test_should_call_if_enabled(perf_client): with requests_mock.Mocker() as request_mock: request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index 04226b263..12125d9c0 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -4,7 +4,7 @@ from functools import partial from freezegun import freeze_time from app.utils import get_midnight_for_day_before -from app.performance_platform.total_sent_notifications import( +from app.performance_platform.total_sent_notifications import ( send_total_notifications_sent_for_day_stats, get_total_sent_notifications_yesterday ) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index c299976d9..72b2f2865 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -86,6 +86,7 @@ def redis_config(): } } + @pytest.fixture def performance_platform_config(): return { From e85b621cbc81f794428ef35912cec5e2b87029cc Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 24 Aug 2017 17:08:39 +0100 Subject: [PATCH 5/5] make perf platform client handle more stuff sensibly specifically, all of the performance platform specific data layout now happens in performance_platform_client.py - stuff like setting the _timestamp, period etc, and the perf platform-specific nomenclature is all handled there. --- app/celery/scheduled_tasks.py | 20 +++++----- .../performance_platform_client.py | 38 ++++++++++++++++--- .../total_sent_notifications.py | 24 +++++------- manifest-api-base.yml | 2 +- manifest-delivery-base.yml | 1 + tests/app/celery/test_scheduled_tasks.py | 6 +-- .../app/clients/test_performance_platform.py | 12 +++--- .../test_total_sent_notifications.py | 8 ++-- 8 files changed, 64 insertions(+), 47 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index c7bda0f74..68295ff7d 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -87,7 +87,7 @@ def delete_verify_codes(): current_app.logger.info( "Delete job started {} finished {} deleted {} verify codes".format(start, datetime.utcnow(), deleted) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete verify codes") raise @@ -106,7 +106,7 @@ def delete_sms_notifications_older_than_seven_days(): deleted ) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete sms notifications") raise @@ -125,7 +125,7 @@ def delete_email_notifications_older_than_seven_days(): deleted ) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete sms notifications") raise @@ -144,7 +144,7 @@ def delete_letter_notifications_older_than_seven_days(): deleted ) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete sms notifications") raise @@ -158,7 +158,7 @@ def delete_invitations(): current_app.logger.info( "Delete job started {} finished {} deleted {} invitations".format(start, datetime.utcnow(), deleted) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete invitations") raise @@ -189,15 +189,13 @@ def send_daily_performance_platform_stats(): total_sent_notifications.send_total_notifications_sent_for_day_stats( start_date, 'sms', - sms_sent_count, - 'day' + sms_sent_count ) total_sent_notifications.send_total_notifications_sent_for_day_stats( start_date, 'email', - email_sent_count, - 'day' + email_sent_count ) @@ -254,7 +252,7 @@ def delete_inbound_sms_older_than_seven_days(): deleted ) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete inbound sms notifications") raise @@ -289,7 +287,7 @@ def delete_dvla_response_files_older_than_seven_days(): len(older_than_seven_days) ) ) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to delete dvla response files") raise diff --git a/app/clients/performance_platform/performance_platform_client.py b/app/clients/performance_platform/performance_platform_client.py index b0f400a8c..d04b22659 100644 --- a/app/clients/performance_platform/performance_platform_client.py +++ b/app/clients/performance_platform/performance_platform_client.py @@ -4,6 +4,8 @@ import json from flask import current_app import requests +from app.utils import convert_utc_to_bst + class PerformancePlatformClient: @@ -17,15 +19,15 @@ class PerformancePlatformClient: self.performance_platform_url = app.config.get('PERFORMANCE_PLATFORM_URL') self.performance_platform_endpoints = app.config.get('PERFORMANCE_PLATFORM_ENDPOINTS') - def send_stats_to_performance_platform(self, dataset, payload): + def send_stats_to_performance_platform(self, payload): if self.active: - bearer_token = self.performance_platform_endpoints[dataset] + bearer_token = self.performance_platform_endpoints[payload['dataType']] headers = { 'Content-Type': "application/json", 'Authorization': 'Bearer {}'.format(bearer_token) } resp = requests.post( - self.performance_platform_url + dataset, + self.performance_platform_url + payload['dataType'], json=payload, headers=headers ) @@ -44,13 +46,37 @@ class PerformancePlatformClient: resp.raise_for_status() @staticmethod - def add_id_to_payload(payload): + def format_payload(*, dataset, date, group_name, group_value, count, period='day'): + """ + :param dataset - the name of the overall graph, as referred to in the endpoint. + :param date - the date we're sending stats for + :param group_name - the name of the individual groups of data, eg "channel" or "status" + :param group_value - the value of the group, eg "sms" or "email" for group_name=channel + :param count - the actual numeric value to send + :param period - the period that this data covers - "day", "week", "month", "quarter". + """ + payload = { + '_timestamp': convert_utc_to_bst(date).isoformat(), + 'service': 'govuk-notify', + 'dataType': dataset, + 'period': period, + 'count': count, + group_name: group_value, + } + payload['_id'] = PerformancePlatformClient.generate_payload_id(payload, group_name) + return payload + + @staticmethod + def generate_payload_id(payload, group_name): + """ + group_name is the name of the group - eg "channel" or "status" + """ payload_string = '{}{}{}{}{}'.format( payload['_timestamp'], payload['service'], - payload['channel'], + payload[group_name], payload['dataType'], payload['period'] ) _id = base64.b64encode(payload_string.encode('utf-8')) - payload.update({'_id': _id.decode('utf-8')}) + return _id.decode('utf-8') diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 9f455f113..4ad57171e 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -4,27 +4,21 @@ from app import performance_platform_client from app.dao.notifications_dao import get_total_sent_notifications_in_date_range from app.utils import ( get_london_midnight_in_utc, - get_midnight_for_day_before, - convert_utc_to_bst, + get_midnight_for_day_before ) -def send_total_notifications_sent_for_day_stats(date, channel, count, period): - payload = { - '_timestamp': convert_utc_to_bst(date).isoformat(), - 'service': 'govuk-notify', - 'channel': channel, - 'count': count, - 'dataType': 'notifications', - 'period': period - } - performance_platform_client.add_id_to_payload(payload) - - performance_platform_client.send_stats_to_performance_platform( +def send_total_notifications_sent_for_day_stats(date, notification_type, count): + payload = performance_platform_client.format_payload( dataset='notifications', - payload=payload + date=date, + group_name='channel', + group_value=notification_type, + count=count ) + performance_platform_client.send_stats_to_performance_platform(payload) + def get_total_sent_notifications_yesterday(): today = datetime.utcnow() diff --git a/manifest-api-base.yml b/manifest-api-base.yml index 9eb4ceb70..19c198d21 100644 --- a/manifest-api-base.yml +++ b/manifest-api-base.yml @@ -1,4 +1,4 @@ -#--- +--- buildpack: python_buildpack command: scripts/run_app_paas.sh gunicorn -w 5 -b 0.0.0.0:$PORT wsgi diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 78053a729..6ceeaf201 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -11,6 +11,7 @@ services: - firetext - hosted-graphite - redis + - performance-platform instances: 1 memory: 1G diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6064f1c79..2a8f2160e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -270,7 +270,7 @@ def test_will_remove_csv_files_for_jobs_older_than_seven_days( ] -def test_send_daily_performance_stats_calls_does_not_send_if_inactive(mocker): +def test_send_daily_performance_stats_calls_does_not_send_if_inactive(client, mocker): send_mock = mocker.patch('app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa with patch.object( @@ -322,8 +322,8 @@ def test_send_daily_performance_stats_calls_with_correct_totals( send_daily_performance_platform_stats() perf_mock.assert_has_calls([ - call(get_london_midnight_in_utc(yesterday), 'sms', 2, 'day'), - call(get_london_midnight_in_utc(yesterday), 'email', 3, 'day') + call(get_london_midnight_in_utc(yesterday), 'sms', 2), + call(get_london_midnight_in_utc(yesterday), 'email', 3) ]) diff --git a/tests/app/clients/test_performance_platform.py b/tests/app/clients/test_performance_platform.py index aba87b9e7..c55a004cd 100644 --- a/tests/app/clients/test_performance_platform.py +++ b/tests/app/clients/test_performance_platform.py @@ -6,7 +6,7 @@ from app.clients.performance_platform.performance_platform_client import Perform @pytest.fixture(scope='function') -def perf_client(mocker): +def perf_client(client, mocker): perf_client = PerformancePlatformClient() current_app = mocker.Mock(config={ 'PERFORMANCE_PLATFORM_ENABLED': True, @@ -24,15 +24,15 @@ def test_should_not_call_if_not_enabled(perf_client): with requests_mock.Mocker() as request_mock: request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) perf_client._active = False - perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) + perf_client.send_stats_to_performance_platform({'dataType': 'foo'}) assert request_mock.called is False -def test_should_call_if_enabled(perf_client): +def test_should_call_datatype_endpoint_if_enabled(perf_client): with requests_mock.Mocker() as request_mock: request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) - perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) + perf_client.send_stats_to_performance_platform({'dataType': 'foo'}) assert request_mock.call_count == 1 assert request_mock.last_request.method == 'POST' @@ -46,7 +46,7 @@ def test_should_use_correct_token(perf_client, dataset, token): with requests_mock.Mocker() as request_mock: request_mock.post('https://performance-platform-url/foo', json={}, status_code=200) request_mock.post('https://performance-platform-url/bar', json={}, status_code=200) - perf_client.send_stats_to_performance_platform(dataset=dataset, payload={}) + perf_client.send_stats_to_performance_platform({'dataType': dataset}) assert request_mock.call_count == 1 assert request_mock.last_request.headers.get('authorization') == 'Bearer {}'.format(token) @@ -55,4 +55,4 @@ def test_should_use_correct_token(perf_client, dataset, token): def test_should_raise_for_status(perf_client): with pytest.raises(requests.HTTPError), requests_mock.Mocker() as request_mock: request_mock.post('https://performance-platform-url/foo', json={}, status_code=403) - perf_client.send_stats_to_performance_platform(dataset='foo', payload={}) + perf_client.send_stats_to_performance_platform({'dataType': 'foo'}) diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index 12125d9c0..e4fe597d0 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -19,15 +19,13 @@ def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call( send_total_notifications_sent_for_day_stats( date=datetime(2016, 10, 15, 23, 0, 0), - channel='sms', - count=142, - period='day' + notification_type='sms', + count=142 ) assert send_stats.call_count == 1 - assert send_stats.call_args[1]['dataset'] == 'notifications' - request_args = send_stats.call_args[1]['payload'] + request_args = send_stats.call_args[0][0] assert request_args['dataType'] == 'notifications' assert request_args['service'] == 'govuk-notify' assert request_args['period'] == 'day'