From 018a253b6f4524dfe4a6142c0036b9b6032d7d76 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 24 Jan 2022 13:58:50 +0000 Subject: [PATCH 1/7] Revert "Revert running status aggregation in parallel" This reverts commit 0f6dea0debc7aeb817e643bd95b5feeabfaad639. --- app/celery/reporting_tasks.py | 58 ++++++++++++------------ app/dao/fact_notification_status_dao.py | 7 ++- tests/app/celery/test_reporting_tasks.py | 24 +++++----- 3 files changed, 45 insertions(+), 44 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index b1fd308b8..9d404edaa 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta from flask import current_app from notifications_utils.timezones import convert_utc_to_bst -from app import notify_celery +from app import db, notify_celery from app.config import QueueNames from app.cronitor import cronitor from app.dao.fact_billing_dao import ( @@ -91,47 +91,46 @@ def create_nightly_notification_status(): yesterday = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) - for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: - days = 10 if notification_type == LETTER_TYPE else 4 + for (service_id,) in db.session.query(Service.id): + for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: + days = 10 if notification_type == LETTER_TYPE else 4 - for i in range(days): - process_day = yesterday - timedelta(days=i) + for i in range(days): + process_day = yesterday - timedelta(days=i) - create_nightly_notification_status_for_day.apply_async( - kwargs={ - 'process_day': process_day.isoformat(), - 'notification_type': notification_type, - }, - queue=QueueNames.REPORTING - ) - current_app.logger.info( - f"create-nightly-notification-status-for-day task created " - f"for {notification_type} and {process_day}" - ) + create_nightly_notification_status_for_service_and_day.apply_async( + kwargs={ + 'process_day': process_day.isoformat(), + 'notification_type': notification_type, + 'service_id': service_id, + }, + queue=QueueNames.REPORTING + ) + current_app.logger.info( + f"create-nightly-notification-status-for-day task created " + f"for {service_id}, {notification_type} and {process_day}" + ) -@notify_celery.task(name="create-nightly-notification-status-for-day") -def create_nightly_notification_status_for_day(process_day, notification_type): +@notify_celery.task(name="create-nightly-notification-status-for-service-and-day") +def create_nightly_notification_status_for_service_and_day(process_day, service_id, notification_type): process_day = datetime.strptime(process_day, "%Y-%m-%d").date() current_app.logger.info( f'create-nightly-notification-status-for-day task started ' - f'for {notification_type} for {process_day}' + f'for {service_id}, {notification_type} for {process_day}' ) start = datetime.utcnow() - new_status_rows = [] - - for service in Service.query.all(): - new_status_rows += fetch_status_data_for_service_and_day( - process_day=process_day, - notification_type=notification_type, - service_id=service.id, - ) + new_status_rows = fetch_status_data_for_service_and_day( + process_day=process_day, + notification_type=notification_type, + service_id=service_id, + ) end = datetime.utcnow() current_app.logger.info( f'create-nightly-notification-status-for-day task fetch ' - f'for {notification_type} for {process_day}: ' + f'for {service_id}, {notification_type} for {process_day}: ' f'data fetched in {(end - start).seconds} seconds' ) @@ -139,10 +138,11 @@ def create_nightly_notification_status_for_day(process_day, notification_type): new_status_rows=new_status_rows, process_day=process_day, notification_type=notification_type, + service_id=service_id ) current_app.logger.info( f'create-nightly-notification-status-for-day task finished ' - f'for {notification_type} for {process_day}: ' + f'for {service_id}, {notification_type} for {process_day}: ' f'{len(new_status_rows)} rows updated' ) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 318a4e397..9c4f10da7 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -45,7 +45,6 @@ def fetch_status_data_for_service_and_day(process_day, service_id, notification_ return db.session.query( table.template_id, - table.service_id, func.coalesce(table.job_id, '00000000-0000-0000-0000-000000000000').label('job_id'), table.key_type, table.status, @@ -58,7 +57,6 @@ def fetch_status_data_for_service_and_day(process_day, service_id, notification_ table.key_type.in_((KEY_TYPE_NORMAL, KEY_TYPE_TEAM)), ).group_by( table.template_id, - table.service_id, 'job_id', table.key_type, table.status @@ -66,11 +64,12 @@ def fetch_status_data_for_service_and_day(process_day, service_id, notification_ @autocommit -def update_fact_notification_status(new_status_rows, process_day, notification_type): +def update_fact_notification_status(new_status_rows, process_day, notification_type, service_id): table = FactNotificationStatus.__table__ FactNotificationStatus.query.filter( FactNotificationStatus.bst_date == process_day, FactNotificationStatus.notification_type == notification_type, + FactNotificationStatus.service_id == service_id, ).delete() for row in new_status_rows: @@ -78,7 +77,7 @@ def update_fact_notification_status(new_status_rows, process_day, notification_t insert(table).values( bst_date=process_day, template_id=row.template_id, - service_id=row.service_id, + service_id=service_id, job_id=row.job_id, notification_type=notification_type, key_type=row.key_type, diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index a8de19465..9e0345200 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -10,7 +10,7 @@ from app.celery.reporting_tasks import ( create_nightly_billing, create_nightly_billing_for_day, create_nightly_notification_status, - create_nightly_notification_status_for_day, + create_nightly_notification_status_for_service_and_day, ) from app.config import QueueNames from app.dao.fact_billing_dao import get_rate @@ -63,7 +63,7 @@ def test_create_nightly_billing_triggers_tasks_for_days(notify_api, mocker, day_ @freeze_time('2019-08-01') def test_create_nightly_notification_status_triggers_tasks(notify_api, sample_service, mocker): - mock_celery = mocker.patch('app.celery.reporting_tasks.create_nightly_notification_status_for_day') + mock_celery = mocker.patch('app.celery.reporting_tasks.create_nightly_notification_status_for_service_and_day') create_nightly_notification_status() assert mock_celery.apply_async.call_count == ( @@ -80,6 +80,7 @@ def test_create_nightly_notification_status_triggers_tasks(notify_api, sample_se kwargs={ 'process_day': process_date, 'notification_type': notification_type, + 'service_id': sample_service.id, }, queue=QueueNames.REPORTING ) @@ -89,6 +90,7 @@ def test_create_nightly_notification_status_triggers_tasks(notify_api, sample_se kwargs={ 'process_day': process_date, 'notification_type': LETTER_TYPE, + 'service_id': sample_service.id, }, queue=QueueNames.REPORTING ) @@ -507,7 +509,7 @@ def test_create_nightly_billing_for_day_update_when_record_exists( assert records[0].updated_at -def test_create_nightly_notification_status_for_day(notify_db_session): +def test_create_nightly_notification_status_for_service_and_day(notify_db_session): first_service = create_service(service_name='First Service') first_template = create_template(service=first_service) second_service = create_service(service_name='second Service') @@ -538,9 +540,9 @@ def test_create_nightly_notification_status_for_day(notify_db_session): assert len(FactNotificationStatus.query.all()) == 0 - create_nightly_notification_status_for_day(str(process_day), 'sms') - create_nightly_notification_status_for_day(str(process_day), 'email') - create_nightly_notification_status_for_day(str(process_day), 'letter') + create_nightly_notification_status_for_service_and_day(str(process_day), first_service.id, 'sms') + create_nightly_notification_status_for_service_and_day(str(process_day), second_service.id, 'email') + create_nightly_notification_status_for_service_and_day(str(process_day), third_service.id, 'letter') new_fact_data = FactNotificationStatus.query.order_by( FactNotificationStatus.notification_type @@ -575,13 +577,13 @@ def test_create_nightly_notification_status_for_day(notify_db_session): assert new_fact_data[2].key_type == KEY_TYPE_NORMAL -def test_create_nightly_notification_status_for_day_overwrites_old_data(notify_db_session): +def test_create_nightly_notification_status_for_service_and_day_overwrites_old_data(notify_db_session): first_service = create_service(service_name='First Service') first_template = create_template(service=first_service) create_notification(template=first_template, status='delivered') process_day = date.today() - create_nightly_notification_status_for_day(str(process_day), 'sms') + create_nightly_notification_status_for_service_and_day(str(process_day), first_service.id, 'sms') new_fact_data = FactNotificationStatus.query.order_by( FactNotificationStatus.bst_date, @@ -592,7 +594,7 @@ def test_create_nightly_notification_status_for_day_overwrites_old_data(notify_d assert new_fact_data[0].notification_count == 1 create_notification(template=first_template, status='delivered') - create_nightly_notification_status_for_day(str(process_day), 'sms') + create_nightly_notification_status_for_service_and_day(str(process_day), first_service.id, 'sms') updated_fact_data = FactNotificationStatus.query.order_by( FactNotificationStatus.bst_date, @@ -605,7 +607,7 @@ def test_create_nightly_notification_status_for_day_overwrites_old_data(notify_d # the job runs at 12:30am London time. 04/01 is in BST. @freeze_time('2019-04-01T23:30') -def test_create_nightly_notification_status_for_day_respects_bst(sample_template): +def test_create_nightly_notification_status_for_service_and_day_respects_bst(sample_template): create_notification(sample_template, status='delivered', created_at=datetime(2019, 4, 1, 23, 0)) # too new create_notification(sample_template, status='created', created_at=datetime(2019, 4, 1, 22, 59)) @@ -613,7 +615,7 @@ def test_create_nightly_notification_status_for_day_respects_bst(sample_template create_notification(sample_template, status='delivered', created_at=datetime(2019, 3, 31, 22, 59)) # too old - create_nightly_notification_status_for_day('2019-04-01', 'sms') + create_nightly_notification_status_for_service_and_day('2019-04-01', sample_template.service_id, 'sms') noti_status = FactNotificationStatus.query.order_by(FactNotificationStatus.bst_date).all() assert len(noti_status) == 1 From d6678b6a704de4562e0c025aa2f7f9a7ae4413de Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 24 Jan 2022 15:56:53 +0000 Subject: [PATCH 2/7] Remove unnecessary logs from status aggreagtion These can be inferred elsewhere: - Task creation is obvious from task execution. If we're concerned about a specific service, we can check the updated times on the DB records, since all records are recreated each time this runs. - Task starting is already logged. - Task completion is already logged. The number of rows updated can also be inferred from the DB. The log I've found useful is the one about fetching the data, and I've also added another to time how long it takes to insert the data, as both could be sources of poor performance. Arguably we should use metrics for this sort of thing, but logs are easier in practice for the metric systems we have. --- app/celery/reporting_tasks.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 9d404edaa..5119b76f9 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -106,19 +106,11 @@ def create_nightly_notification_status(): }, queue=QueueNames.REPORTING ) - current_app.logger.info( - f"create-nightly-notification-status-for-day task created " - f"for {service_id}, {notification_type} and {process_day}" - ) @notify_celery.task(name="create-nightly-notification-status-for-service-and-day") def create_nightly_notification_status_for_service_and_day(process_day, service_id, notification_type): process_day = datetime.strptime(process_day, "%Y-%m-%d").date() - current_app.logger.info( - f'create-nightly-notification-status-for-day task started ' - f'for {service_id}, {notification_type} for {process_day}' - ) start = datetime.utcnow() new_status_rows = fetch_status_data_for_service_and_day( @@ -134,6 +126,7 @@ def create_nightly_notification_status_for_service_and_day(process_day, service_ f'data fetched in {(end - start).seconds} seconds' ) + start = datetime.utcnow() update_fact_notification_status( new_status_rows=new_status_rows, process_day=process_day, @@ -141,8 +134,9 @@ def create_nightly_notification_status_for_service_and_day(process_day, service_ service_id=service_id ) + end = datetime.utcnow() current_app.logger.info( - f'create-nightly-notification-status-for-day task finished ' + f'create-nightly-notification-status-for-day task update ' f'for {service_id}, {notification_type} for {process_day}: ' - f'{len(new_status_rows)} rows updated' + f'data fetched in {(end - start).seconds} seconds' ) From c8db58d0e8d05a78659205053c4a722734ee39ce Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 24 Jan 2022 16:02:23 +0000 Subject: [PATCH 3/7] Reorder loops for creation status agg sub tasks This will help tailor the innermost loop on services. --- app/celery/reporting_tasks.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 5119b76f9..2f74bca10 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -91,12 +91,13 @@ def create_nightly_notification_status(): yesterday = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) - for (service_id,) in db.session.query(Service.id): - for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: - days = 10 if notification_type == LETTER_TYPE else 4 + for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: + days = 10 if notification_type == LETTER_TYPE else 4 - for i in range(days): - process_day = yesterday - timedelta(days=i) + for i in range(days): + process_day = yesterday - timedelta(days=i) + + for (service_id,) in db.session.query(Service.id): create_nightly_notification_status_for_service_and_day.apply_async( kwargs={ From 1213463b8e4b915f61992025d5da1362c071a899 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 Jan 2022 11:29:57 +0000 Subject: [PATCH 4/7] Only aggregate status when necessary for a service This takes a similar approach to the nightly deletion task so that we only create sub-tasks when there are actually notifications to aggregate for a given type and day [1]. We're making this change to stop the duplication errors we're getting at the moment and ensure the task can scale to more messages and more services. There are two parts to this: - Each subtask should now run within the 5 minute visibility timeout. However, they may still be duplicated if the parent task overruns [2]. - The parent task creates a mininal number of subtasks, and the query to determine this is very fast for a normal process day (milliseconds). Since all tasks will run quickly, there should be no more duplication. In order to test this more nuanced task, I rewrote the tests: - One test checks the subtask is called correctly. - One test checks we create all the right subtasks. [1]: https://github.com/alphagov/notifications-api/pull/3381 [2]: https://docs.google.com/document/d/1MaP6Nyy3nJKkuh_4lP1wuDm19X8LZITOLRd9n3Ax-xg/edit#heading=h.q3intzwqhfzl --- app/celery/reporting_tasks.py | 10 ++- app/dao/notifications_dao.py | 14 +++++ tests/app/celery/test_reporting_tasks.py | 77 +++++++++++++++--------- 3 files changed, 69 insertions(+), 32 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 2f74bca10..118bf967d 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -3,7 +3,7 @@ from datetime import datetime, timedelta from flask import current_app from notifications_utils.timezones import convert_utc_to_bst -from app import db, notify_celery +from app import notify_celery from app.config import QueueNames from app.cronitor import cronitor from app.dao.fact_billing_dao import ( @@ -14,7 +14,8 @@ from app.dao.fact_notification_status_dao import ( fetch_status_data_for_service_and_day, update_fact_notification_status, ) -from app.models import EMAIL_TYPE, LETTER_TYPE, SMS_TYPE, Service +from app.dao.notifications_dao import get_service_ids_with_notifications_on_date +from app.models import EMAIL_TYPE, LETTER_TYPE, SMS_TYPE @notify_celery.task(name="create-nightly-billing") @@ -97,8 +98,11 @@ def create_nightly_notification_status(): for i in range(days): process_day = yesterday - timedelta(days=i) - for (service_id,) in db.session.query(Service.id): + relevant_service_ids = get_service_ids_with_notifications_on_date( + notification_type, process_day + ) + for service_id in relevant_service_ids: create_nightly_notification_status_for_service_and_day.apply_async( kwargs={ 'process_day': process_day.isoformat(), diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 393bba051..4292f1c7a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -794,3 +794,17 @@ def get_service_ids_that_have_notifications_from_before_timestamp(notification_t Notification.created_at < timestamp ).distinct() } + + +def get_service_ids_with_notifications_on_date(notification_type, date): + return { + row.service_id + for row in db.session.query( + Notification.service_id + ).filter( + Notification.notification_type == notification_type, + # using >= + < is much more efficient than date(created_at) + Notification.created_at >= date, + Notification.created_at < date + timedelta(days=1) + ).distinct() + } diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 9e0345200..1e83161f7 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -1,4 +1,3 @@ -import itertools from datetime import date, datetime, time, timedelta from decimal import Decimal from uuid import UUID @@ -20,6 +19,7 @@ from app.models import ( KEY_TYPE_TEAM, KEY_TYPE_TEST, LETTER_TYPE, + NOTIFICATION_TYPES, SMS_TYPE, FactBilling, FactNotificationStatus, @@ -61,39 +61,58 @@ def test_create_nightly_billing_triggers_tasks_for_days(notify_api, mocker, day_ assert mock_celery.apply_async.call_args_list[i][1]['kwargs'] == {'process_day': expected_kwargs[i]} -@freeze_time('2019-08-01') -def test_create_nightly_notification_status_triggers_tasks(notify_api, sample_service, mocker): - mock_celery = mocker.patch('app.celery.reporting_tasks.create_nightly_notification_status_for_service_and_day') +@freeze_time('2019-08-01T00:30') +def test_create_nightly_notification_status_triggers_tasks( + notify_api, + sample_service, + sample_template, + mocker, +): + mock_celery = mocker.patch( + 'app.celery.reporting_tasks.create_nightly_notification_status_for_service_and_day' + ).apply_async + + create_notification(template=sample_template, created_at='2019-07-31') create_nightly_notification_status() - assert mock_celery.apply_async.call_count == ( - (4 * 3) # four days, three notification types - + - 6 # six more days of just letters + mock_celery.assert_called_with( + kwargs={ + 'service_id': sample_service.id, + 'process_day': '2019-07-31', + 'notification_type': SMS_TYPE + }, + queue=QueueNames.REPORTING ) - for process_date, notification_type in itertools.product( - ['2019-07-31', '2019-07-30', '2019-07-29', '2019-07-28'], - [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] - ): - mock_celery.apply_async.assert_any_call( - kwargs={ - 'process_day': process_date, - 'notification_type': notification_type, - 'service_id': sample_service.id, - }, - queue=QueueNames.REPORTING - ) - for process_date in ['2019-07-27', '2019-07-26', '2019-07-25', '2019-07-24', '2019-07-23', '2019-07-22']: - mock_celery.apply_async.assert_any_call( - kwargs={ - 'process_day': process_date, - 'notification_type': LETTER_TYPE, - 'service_id': sample_service.id, - }, - queue=QueueNames.REPORTING - ) +@freeze_time('2019-08-01T00:30') +@pytest.mark.parametrize('notification_date, expected_types_aggregated', [ + ('2019-08-01', set()), + ('2019-07-31', {EMAIL_TYPE, SMS_TYPE, LETTER_TYPE}), + ('2019-07-28', {EMAIL_TYPE, SMS_TYPE, LETTER_TYPE}), + ('2019-07-27', {LETTER_TYPE}), + ('2019-07-22', {LETTER_TYPE}), + ('2019-07-21', set()), +]) +def test_create_nightly_notification_status_triggers_relevant_tasks( + notify_api, + sample_service, + mocker, + notification_date, + expected_types_aggregated, +): + mock_celery = mocker.patch( + 'app.celery.reporting_tasks.create_nightly_notification_status_for_service_and_day' + ).apply_async + + for notification_type in NOTIFICATION_TYPES: + template = create_template(sample_service, template_type=notification_type) + create_notification(template=template, created_at=notification_date) + + create_nightly_notification_status() + + types = {call.kwargs['kwargs']['notification_type'] for call in mock_celery.mock_calls} + assert types == expected_types_aggregated @pytest.mark.parametrize('second_rate, records_num, billable_units, multiplier', From 7f4b140f97516874073d35cac9dee88a82fcced1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 24 Jan 2022 15:54:37 +0000 Subject: [PATCH 5/7] Rename function to make it consistent This is consistent with the new "on_date" function. It was going off the edge of my screen before in some parts of the code. --- app/celery/nightly_tasks.py | 4 ++-- app/dao/notifications_dao.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 04b3fc31a..54edbe889 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -22,7 +22,7 @@ from app.dao.jobs_dao import ( from app.dao.notifications_dao import ( dao_get_notifications_processing_time_stats, dao_timeout_notifications, - get_service_ids_that_have_notifications_from_before_timestamp, + get_service_ids_with_notifications_before, move_notifications_to_notification_history, ) from app.dao.service_data_retention_dao import ( @@ -107,7 +107,7 @@ def _delete_notifications_older_than_retention_by_type(notification_type): # get a list of all service ids that we'll need to delete for. Typically that might only be 5% of services. # This query takes a couple of mins to run. - service_ids_that_have_sent_notifications_recently = get_service_ids_that_have_notifications_from_before_timestamp( + service_ids_that_have_sent_notifications_recently = get_service_ids_with_notifications_before( notification_type, seven_days_ago ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4292f1c7a..df93fa4ee 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -784,7 +784,7 @@ def _duplicate_update_warning(notification, status): ) -def get_service_ids_that_have_notifications_from_before_timestamp(notification_type, timestamp): +def get_service_ids_with_notifications_before(notification_type, timestamp): return { row.service_id for row in db.session.query( From 6e8f12154857b7303fbb4ebb43a3b4f18f04052e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 9 Feb 2022 17:44:00 +0000 Subject: [PATCH 6/7] Standardise how we query midnight-to-midnight Partially addresses [1] (lots more detail to read in the comment). I've also added some tests for the status DAO function to confirm it behaves as expected across timezones. [1]: https://github.com/alphagov/notifications-api/pull/3437#discussion_r802634913 --- app/dao/fact_billing_dao.py | 8 +++---- app/dao/fact_notification_status_dao.py | 7 +++---- app/utils.py | 6 ++---- .../dao/test_fact_notification_status_dao.py | 21 +++++++++++++++++++ 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index 3fbc3953b..ff8783907 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -1,7 +1,7 @@ -from datetime import date, datetime, time, timedelta +from datetime import date, datetime, timedelta from flask import current_app -from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst +from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import Date, Integer, and_, desc, func from sqlalchemy.dialects.postgresql import insert from sqlalchemy.sql.expression import case, literal @@ -317,8 +317,8 @@ def delete_billing_data_for_service_for_day(process_day, service_id): def fetch_billing_data_for_day(process_day, service_id=None, check_permissions=False): - start_date = convert_bst_to_utc(datetime.combine(process_day, time.min)) - end_date = convert_bst_to_utc(datetime.combine(process_day + timedelta(days=1), time.min)) + start_date = get_london_midnight_in_utc(process_day) + end_date = get_london_midnight_in_utc(process_day + timedelta(days=1)) current_app.logger.info("Populate ft_billing for {} to {}".format(start_date, end_date)) transit_data = [] if not service_id: diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 9c4f10da7..d19ba748c 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -1,6 +1,5 @@ -from datetime import datetime, time, timedelta +from datetime import datetime, timedelta -from notifications_utils.timezones import convert_bst_to_utc from sqlalchemy import Date, case, func from sqlalchemy.dialects.postgresql import insert from sqlalchemy.sql.expression import extract, literal @@ -36,8 +35,8 @@ from app.utils import ( def fetch_status_data_for_service_and_day(process_day, service_id, notification_type): - start_date = convert_bst_to_utc(datetime.combine(process_day, time.min)) - end_date = convert_bst_to_utc(datetime.combine(process_day + timedelta(days=1), time.min)) + start_date = get_london_midnight_in_utc(process_day) + end_date = get_london_midnight_in_utc(process_day + timedelta(days=1)) # query notifications or notification_history for the day, depending on their data retention service = Service.query.get(service_id) diff --git a/app/utils.py b/app/utils.py index a2e8f8eb8..86cfad6c5 100644 --- a/app/utils.py +++ b/app/utils.py @@ -8,7 +8,7 @@ from notifications_utils.template import ( LetterPrintTemplate, SMSMessageTemplate, ) -from notifications_utils.timezones import convert_utc_to_bst +from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst from sqlalchemy import func DATETIME_FORMAT_NO_TIMEZONE = "%Y-%m-%d %H:%M:%S.%f" @@ -64,9 +64,7 @@ def get_london_midnight_in_utc(date): :param date: the day to calculate the London midnight in UTC for :return: the datetime of London midnight in UTC, for example 2016-06-17 = 2016-06-16 23:00:00 """ - return local_timezone.localize(datetime.combine(date, datetime.min.time())).astimezone( - pytz.UTC).replace( - tzinfo=None) + return convert_bst_to_utc(datetime.combine(date, datetime.min.time())) def get_midnight_for_day_before(date): diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 3007f1398..b86760c85 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -14,6 +14,7 @@ from app.dao.fact_notification_status_dao import ( fetch_notification_status_totals_for_all_services, fetch_notification_statuses_for_job, fetch_stats_for_all_services_by_date_range, + fetch_status_data_for_service_and_day, get_total_notifications_for_date_range, ) from app.models import ( @@ -607,3 +608,23 @@ def test_get_total_notifications_for_date_range(sample_service): assert len(results) == 1 assert results[0] == ("2021-03-01", 15, 20, 3) + + +@pytest.mark.parametrize('created_at_utc,process_day,expected_count', [ + # Clocks change on the 27th of March 2022, so the query needs to look at the + # time range 00:00 - 23:00 (UTC) thereafter. + ('2022-03-27T00:30', date(2022, 3, 27), 1), # 27/03 00:30 GMT + ('2022-03-27T22:30', date(2022, 3, 27), 1), # 27/03 23:30 BST + ('2022-03-27T23:30', date(2022, 3, 27), 0), # 28/03 00:30 BST + ('2022-03-26T23:30', date(2022, 3, 26), 1), # 26/03 23:30 GMT +]) +def test_fetch_status_data_for_service_and_day_respects_gmt_bst( + sample_template, + sample_service, + created_at_utc, + process_day, + expected_count, +): + create_notification(template=sample_template, created_at=created_at_utc) + rows = fetch_status_data_for_service_and_day(process_day, sample_service.id, SMS_TYPE) + assert len(rows) == expected_count From 966c4db8c6f96ee5fd3424f403338497a792b712 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 10 Feb 2022 10:37:32 +0000 Subject: [PATCH 7/7] Fix getting service IDs for status aggregation Addresses [1]. Previously the query would always use UTC midnight, even after we had switched to BST (+1h). We store timestamps as naive UTC in our DB - without a timezone - but we want the query to work in terms of GMT / BST so we adjust for that - BST midnight is 11PM in UTC. [1]: https://github.com/alphagov/notifications-api/pull/3437#discussion_r791998690 --- app/dao/notifications_dao.py | 13 ++++++++--- .../notification_dao/test_notification_dao.py | 23 ++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index df93fa4ee..e2af29eb6 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -46,7 +46,11 @@ from app.models import ( NotificationHistory, ProviderDetails, ) -from app.utils import escape_special_characters, midnight_n_days_ago +from app.utils import ( + escape_special_characters, + get_london_midnight_in_utc, + midnight_n_days_ago, +) def dao_get_last_date_template_was_used(template_id, service_id): @@ -797,6 +801,9 @@ def get_service_ids_with_notifications_before(notification_type, timestamp): def get_service_ids_with_notifications_on_date(notification_type, date): + start_date = get_london_midnight_in_utc(date) + end_date = get_london_midnight_in_utc(date + timedelta(days=1)) + return { row.service_id for row in db.session.query( @@ -804,7 +811,7 @@ def get_service_ids_with_notifications_on_date(notification_type, date): ).filter( Notification.notification_type == notification_type, # using >= + < is much more efficient than date(created_at) - Notification.created_at >= date, - Notification.created_at < date + timedelta(days=1) + Notification.created_at >= start_date, + Notification.created_at < end_date, ).distinct() } diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 501c5ec6e..03b75e345 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1,5 +1,5 @@ import uuid -from datetime import datetime, timedelta +from datetime import date, datetime, timedelta from functools import partial import pytest @@ -24,6 +24,7 @@ from app.dao.notifications_dao import ( get_notification_with_personalisation, get_notifications_for_job, get_notifications_for_service, + get_service_ids_with_notifications_on_date, is_delivery_slow_for_providers, notifications_not_yet_sent, update_notification_status_by_id, @@ -41,6 +42,7 @@ from app.models import ( NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_TEMPORARY_FAILURE, + SMS_TYPE, Job, Notification, NotificationHistory, @@ -1721,3 +1723,22 @@ def test_dao_get_letters_and_sheets_volume_by_postage(notify_db_session): for result in results: assert result._asdict() in expected_results + + +@pytest.mark.parametrize('created_at_utc,date_to_check,expected_count', [ + # Clocks change on the 27th of March 2022, so the query needs to look at the + # time range 00:00 - 23:00 (UTC) thereafter. + ('2022-03-27T00:30', date(2022, 3, 27), 1), # 27/03 00:30 GMT + ('2022-03-27T22:30', date(2022, 3, 27), 1), # 27/03 23:30 BST + ('2022-03-27T23:30', date(2022, 3, 27), 0), # 28/03 00:30 BST + ('2022-03-26T23:30', date(2022, 3, 26), 1), # 26/03 23:30 GMT +]) +def test_get_service_ids_with_notifications_on_date_respects_gmt_bst( + sample_template, + created_at_utc, + date_to_check, + expected_count +): + create_notification(template=sample_template, created_at=created_at_utc) + service_ids = get_service_ids_with_notifications_on_date(SMS_TYPE, date_to_check) + assert len(service_ids) == expected_count