From 2622866622cb331f79e208dcee389bca480012f1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 31 Aug 2017 12:52:06 +0100 Subject: [PATCH 1/4] log unhandled celery exceptions they were always caught locally by celery's base handler, however, we weren't logging them ourselves, which meant it wouldn't be put into the json logs that are sent to cloudwatch. --- app/celery/celery.py | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/app/celery/celery.py b/app/celery/celery.py index 183e50bd6..59d240d07 100644 --- a/app/celery/celery.py +++ b/app/celery/celery.py @@ -1,17 +1,28 @@ -from celery import Celery +from flask import current_app +from celery import Celery, Task + + +class NotifyTask(Task): + abstract = True + + def on_failure(self, exc, task_id, args, kwargs, einfo): + # ensure task will log exceptions to correct handlers + current_app.logger.exception('Celery task failed') + super().on_failure(exc, task_id, args, kwargs, einfo) + + def __call__(self, *args, **kwargs): + # ensure task has flask context to access config, logger, etc + with current_app.app_context(): + return super().__call__(*args, **kwargs) class NotifyCelery(Celery): def init_app(self, app): - super().__init__(app.import_name, broker=app.config['BROKER_URL']) + super().__init__( + app.import_name, + broker=app.config['BROKER_URL'], + task_cls=NotifyTask, + ) + self.conf.update(app.config) - TaskBase = self.Task - - class ContextTask(TaskBase): - abstract = True - - def __call__(self, *args, **kwargs): - with app.app_context(): - return TaskBase.__call__(self, *args, **kwargs) - self.Task = ContextTask From e5da0c6e8bf06b2514b3b559eb97588b012cd2f9 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 31 Aug 2017 13:03:25 +0100 Subject: [PATCH 2/4] update utils to 20.0.3 brings in change to logging to use WatchedFileHandler --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 3c277b754..09527005b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client==4.3.1 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@20.0.0#egg=notifications-utils==20.0.0 +git+https://github.com/alphagov/notifications-utils.git@20.0.3#egg=notifications-utils==20.0.3 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 2c758cbccd33720cd1e426a0b6ff5020fcbf96e3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 12 Sep 2017 16:13:07 +0100 Subject: [PATCH 3/4] use new send-jobs-to-dvla task instead of send-files-to-dvla the tasks (on the ftp app) do the same thing, but to improve clarity i've renamed it to jobs, because we'll be adding a notifications one soon --- app/celery/scheduled_tasks.py | 2 +- app/config.py | 5 +++-- app/letters/rest.py | 2 +- app/v2/notifications/post_notifications.py | 2 +- tests/app/celery/test_scheduled_tasks.py | 2 +- tests/app/letters/test_send_letter_jobs.py | 2 +- 6 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index bf10ceec7..e18cbe8e6 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -314,7 +314,7 @@ def populate_monthly_billing(): def run_letter_jobs(): job_ids = dao_get_letter_job_ids_by_status(JOB_STATUS_READY_TO_SEND) notify_celery.send_task( - name=TaskNames.DVLA_FILES, + name=TaskNames.DVLA_JOBS, args=(job_ids,), queue=QueueNames.PROCESS_FTP ) diff --git a/app/config.py b/app/config.py index c6c5291eb..daf552348 100644 --- a/app/config.py +++ b/app/config.py @@ -48,7 +48,8 @@ class QueueNames(object): class TaskNames(object): - DVLA_FILES = 'send-files-to-dvla' + DVLA_JOBS = 'send-jobs-to-dvla' + DVLA_NOTIFICATIONS = 'send-notifications-to-dvla' class Config(object): @@ -226,7 +227,7 @@ class Config(object): }, 'run-letter-jobs': { 'task': 'run-letter-jobs', - 'schedule': crontab(minute=30, hour=17), + 'schedule': crontab(hour=17, minute=30), 'options': {'queue': QueueNames.PERIODIC} } } diff --git a/app/letters/rest.py b/app/letters/rest.py index b9ce7bf35..611d5132d 100644 --- a/app/letters/rest.py +++ b/app/letters/rest.py @@ -16,7 +16,7 @@ register_errors(letter_job) @letter_job.route('/send-letter-jobs', methods=['POST']) def send_letter_jobs(): job_ids = validate(request.get_json(), letter_job_ids) - notify_celery.send_task(name=TaskNames.DVLA_FILES, args=(job_ids['job_ids'],), queue=QueueNames.PROCESS_FTP) + notify_celery.send_task(name=TaskNames.DVLA_JOBS, args=(job_ids['job_ids'],), queue=QueueNames.PROCESS_FTP) return jsonify(data={"response": "Task created to send files to DVLA"}), 201 diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c2544ffd4..47a47a711 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -151,7 +151,7 @@ def process_letter_notification(*, letter_data, api_key, template): raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) if api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: - raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) + raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6cfbcf57a..fca018597 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -690,6 +690,6 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): run_letter_jobs() - mock_celery.assert_called_once_with(name=TaskNames.DVLA_FILES, + mock_celery.assert_called_once_with(name=TaskNames.DVLA_JOBS, args=(job_ids,), queue=QueueNames.PROCESS_FTP) diff --git a/tests/app/letters/test_send_letter_jobs.py b/tests/app/letters/test_send_letter_jobs.py index d899387b6..792d55269 100644 --- a/tests/app/letters/test_send_letter_jobs.py +++ b/tests/app/letters/test_send_letter_jobs.py @@ -25,7 +25,7 @@ def test_send_letter_jobs(client, mocker, sample_letter_template): assert response.status_code == 201 assert json.loads(response.get_data())['data'] == {'response': "Task created to send files to DVLA"} - mock_celery.assert_called_once_with(name="send-files-to-dvla", + mock_celery.assert_called_once_with(name="send-jobs-to-dvla", args=(job_ids['job_ids'],), queue="process-ftp-tasks") From 71f459b2d829a2e2033384ec6d7f9e4db5391f62 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 15 Sep 2017 10:58:45 +0100 Subject: [PATCH 4/4] Bump utils to 21.2.0 This will stop logging the request, as it is logged by nginx-access logs --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 05ac008e9..69dde0971 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client==4.4.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.0.0#egg=notifications-utils==21.0.0 +git+https://github.com/alphagov/notifications-utils.git@21.2.0#egg=notifications-utils==21.2.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3