From da4efbbb82f0658414831f3fe4bdc99b6b190004 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Fri, 28 Aug 2020 11:23:51 +0100
Subject: [PATCH] Catch and log any exception thrown in the checkin event
 method.

We don't want an exception while recording metrics to affect a user action. A KeyError exception was thrown today, that meant that a user say a 500, the action being performed was to download a document from the document-download-frontend app. By catching the error we prevent the user from seeing a 500 when a recording the connection metric fails.

Also catch the exception in the checkout event.
---
 app/__init__.py | 82 ++++++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 38 deletions(-)

diff --git a/app/__init__.py b/app/__init__.py
index 79c57e9ef..efa1a6697 100644
--- a/app/__init__.py
+++ b/app/__init__.py
@@ -352,49 +352,55 @@ def setup_sqlalchemy_events(app):
 
         @event.listens_for(db.engine, 'checkout')
         def checkout(dbapi_connection, connection_record, connection_proxy):
-            # connection given to a web worker
-            TOTAL_CHECKED_OUT_DB_CONNECTIONS.inc()
+            try:
+                # connection given to a web worker
+                TOTAL_CHECKED_OUT_DB_CONNECTIONS.inc()
 
-            # this will overwrite any previous checkout_at timestamp
-            connection_record.info['checkout_at'] = time.monotonic()
+                # this will overwrite any previous checkout_at timestamp
+                connection_record.info['checkout_at'] = time.monotonic()
 
-            # checkin runs after the request is already torn down, therefore we add the request_data onto the
-            # connection_record as otherwise it won't have that information when checkin actually runs.
-            # Note: this is not a problem for checkouts as the checkout always happens within a web request or task
+                # checkin runs after the request is already torn down, therefore we add the request_data onto the
+                # connection_record as otherwise it won't have that information when checkin actually runs.
+                # Note: this is not a problem for checkouts as the checkout always happens within a web request or task
 
-            # web requests
-            if has_request_context():
-                connection_record.info['request_data'] = {
-                    'method': request.method,
-                    'host': request.host,
-                    'url_rule': request.url_rule.rule if request.url_rule else 'No endpoint'
-                }
-            # celery apps
-            elif current_task:
-                connection_record.info['request_data'] = {
-                    'method': 'celery',
-                    'host': current_app.config['NOTIFY_APP_NAME'],  # worker name
-                    'url_rule': current_task.name,  # task name
-                }
-            # anything else. migrations possibly.
-            else:
-                current_app.logger.warning('Checked out sqlalchemy connection from outside of request/task')
-                connection_record.info['request_data'] = {
-                    'method': 'unknown',
-                    'host': 'unknown',
-                    'url_rule': 'unknown',
-                }
+                # web requests
+                if has_request_context():
+                    connection_record.info['request_data'] = {
+                        'method': request.method,
+                        'host': request.host,
+                        'url_rule': request.url_rule.rule if request.url_rule else 'No endpoint'
+                    }
+                # celery apps
+                elif current_task:
+                    connection_record.info['request_data'] = {
+                        'method': 'celery',
+                        'host': current_app.config['NOTIFY_APP_NAME'],  # worker name
+                        'url_rule': current_task.name,  # task name
+                    }
+                # anything else. migrations possibly.
+                else:
+                    current_app.logger.warning('Checked out sqlalchemy connection from outside of request/task')
+                    connection_record.info['request_data'] = {
+                        'method': 'unknown',
+                        'host': 'unknown',
+                        'url_rule': 'unknown',
+                    }
+            except Exception:
+                current_app.logger.exception("Exception caught for checkout event.")
 
         @event.listens_for(db.engine, 'checkin')
         def checkin(dbapi_connection, connection_record):
-            # connection returned by a web worker
-            TOTAL_CHECKED_OUT_DB_CONNECTIONS.dec()
+            try:
+                # connection returned by a web worker
+                TOTAL_CHECKED_OUT_DB_CONNECTIONS.dec()
 
-            # duration that connection was held by a single web request
-            duration = time.monotonic() - connection_record.info['checkout_at']
+                # duration that connection was held by a single web request
+                duration = time.monotonic() - connection_record.info['checkout_at']
 
-            DB_CONNECTION_OPEN_DURATION_SECONDS.labels(
-                connection_record.info['request_data']['method'],
-                connection_record.info['request_data']['host'],
-                connection_record.info['request_data']['url_rule']
-            ).observe(duration)
+                DB_CONNECTION_OPEN_DURATION_SECONDS.labels(
+                    connection_record.info['request_data']['method'],
+                    connection_record.info['request_data']['host'],
+                    connection_record.info['request_data']['url_rule']
+                ).observe(duration)
+            except Exception:
+                current_app.logger.exception("Exception caught for checkin event.")