From 3d9f173e35431f663b815f24c08d4d9dd835cd71 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Apr 2020 11:28:43 +0100 Subject: [PATCH] Remove, cache or hardcode anything that could be cached when sending an email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probably shouldn’t deploy this to production 😅 This shows exactly what I removed, bodged and hardcoded to test what the performance implications of caching a bunch of stuff might look like. Test command: ```bash python -m timeit -n 1 -s "import notifications_python_client; c = notifications_python_client.notifications.NotificationsAPIClient('🤫', base_url='http://localhost:6011')" "c.send_email_notification(email_address='sender@something.com', template_id='be433bfc-fe31-464b-9f2c-5be11abf2176')" ``` Before: ``` raw times: 12.1 11.9 11.8 100 loops, best of 3: 118 msec per loop ``` After ``` raw times: 11.2 10.7 10.1 100 loops, best of 3: 101 msec per loop ``` Not a big improvement… So I was curious what it was doing in those ~100ms Let’s go back to master and comment out persisting the notification to the database: ``` raw times: 12.3 10.5 10.5 100 loops, best of 3: 105 msec per loop ``` (saves about 13ms) If we comment out sending to the queue: ``` raw times: 3.43 3.24 4.88 100 loops, best of 3: 32.4 msec per loop ``` (saves about 85ms) This means most of our request time is spent waiting for SQS. If we test our fake caching while sending to the queue is commented out we get a clearer picture of the potential improvements: ``` raw times: 2.13 1.84 2.18 100 loops, best of 3: 18.4 msec per loop ``` This is a saving of 14ms, from a baseline of 32.4ms, so 56%. A typical call to fetch a service from Redis from the admin app takes about 0.6ms, for context. It’s also worth thinking about whether we’re holding a database connection longer than we need to if we still have it while talking to SQS. --- app/authentication/auth.py | 59 ++++------------------ app/notifications/validators.py | 11 +++- app/v2/notifications/post_notifications.py | 41 ++++----------- 3 files changed, 31 insertions(+), 80 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 02ae025bf..20656820e 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,3 +1,4 @@ +from functools import lru_cache from flask import request, _request_ctx_stack, current_app, g from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import ( @@ -80,62 +81,24 @@ def requires_admin_auth(): raise AuthError('Unauthorized: admin authentication token required', 401) +@lru_cache(maxsize=None) +def get_service(issuer): + return dao_fetch_service_by_id_with_api_keys(issuer) + + def requires_auth(): request_helper.check_proxy_header_before_request() auth_token = get_auth_token(request) issuer = __get_token_issuer(auth_token) # ie the `iss` claim which should be a service ID - try: - service = dao_fetch_service_by_id_with_api_keys(issuer) - except DataError: - raise AuthError("Invalid token: service id is not the right data type", 403) - except NoResultFound: - raise AuthError("Invalid token: service not found", 403) + service = get_service(issuer) - if not service.api_keys: - raise AuthError("Invalid token: service has no API keys", 403, service_id=service.id) + g.service_id = issuer + _request_ctx_stack.top.authenticated_service = service + _request_ctx_stack.top.api_user = None - if not service.active: - raise AuthError("Invalid token: service is archived", 403, service_id=service.id) - - for api_key in service.api_keys: - try: - decode_jwt_token(auth_token, api_key.secret) - except TokenExpiredError: - err_msg = "Error: Your system clock must be accurate to within 30 seconds" - raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) - except TokenAlgorithmError: - err_msg = "Invalid token: algorithm used is not HS256" - raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) - except TokenDecodeError: - # we attempted to validate the token but it failed meaning it was not signed using this api key. - # Let's try the next one - # TODO: Change this so it doesn't also catch `TokenIssuerError` or `TokenIssuedAtError` exceptions (which - # are children of `TokenDecodeError`) as these should cause an auth error immediately rather than - # continue on to check the next API key - continue - except TokenError: - # General error when trying to decode and validate the token - raise AuthError(GENERAL_TOKEN_ERROR_MESSAGE, 403, service_id=service.id, api_key_id=api_key.id) - - if api_key.expiry_date: - raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) - - g.service_id = api_key.service_id - _request_ctx_stack.top.authenticated_service = service - _request_ctx_stack.top.api_user = api_key - - current_app.logger.info('API authorised for service {} with api key {}, using issuer {} for URL: {}'.format( - service.id, - api_key.id, - request.headers.get('User-Agent'), - request.base_url - )) - return - else: - # service has API keys, but none matching the one the user provided - raise AuthError("Invalid token: API key not found", 403, service_id=service.id) + return def __get_token_issuer(auth_token): diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 3b9cf6c62..a61ab71d7 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,3 +1,4 @@ +from functools import lru_cache from sqlalchemy.orm.exc import NoResultFound from flask import current_app from notifications_utils import SMS_CHAR_COUNT_LIMIT @@ -138,9 +139,17 @@ def check_notification_content_is_not_empty(template_with_content): raise BadRequestError(message=message) +@lru_cache(maxsize=None) +def get_template(template_id, service_id): + return templates_dao.dao_get_template_by_id_and_service_id( + template_id=template_id, + service_id=service_id + ) + + def validate_template(template_id, personalisation, service, notification_type): try: - template = templates_dao.dao_get_template_by_id_and_service_id( + template = get_template( template_id=template_id, service_id=service.id ) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 9299b08ab..b23f3a968 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -121,20 +121,12 @@ def post_notification(notification_type): if notification_type == EMAIL_TYPE: form = validate(request_json, post_email_request) elif notification_type == SMS_TYPE: - form = validate(request_json, post_sms_request) + abort(400) elif notification_type == LETTER_TYPE: - form = validate(request_json, post_letter_request) + abort(400) else: abort(404) - check_service_has_permission(notification_type, authenticated_service.permissions) - - scheduled_for = form.get("scheduled_for", None) - - check_service_can_schedule_notification(authenticated_service.permissions, scheduled_for) - - check_rate_limiting(authenticated_service, api_user) - template, template_with_content = validate_template( form['template_id'], form.get('personalisation', {}), @@ -142,15 +134,10 @@ def post_notification(notification_type): notification_type, ) - reply_to = get_reply_to_text(notification_type, form, template) + reply_to = 'test@example.com' if notification_type == LETTER_TYPE: - notification = process_letter_notification( - letter_data=form, - api_key=api_user, - template=template, - reply_to_text=reply_to - ) + abort(400) else: notification = process_sms_or_email_notification( form=form, @@ -164,11 +151,7 @@ def post_notification(notification_type): template_with_content.values = notification.personalisation if notification_type == SMS_TYPE: - create_resp_partial = functools.partial( - create_post_sms_response_from_notification, - from_number=reply_to, - content=str(template_with_content), - ) + abort(400) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( create_post_email_response_from_notification, @@ -177,16 +160,12 @@ def post_notification(notification_type): content=WithSubjectTemplate.__str__(template_with_content), ) elif notification_type == LETTER_TYPE: - create_resp_partial = functools.partial( - create_post_letter_response_from_notification, - subject=template_with_content.subject, - content=WithSubjectTemplate.__str__(template_with_content), - ) + abort(400) resp = create_resp_partial( notification=notification, url_root=request.url_root, - scheduled_for=scheduled_for + scheduled_for=None ) return jsonify(resp), 201 @@ -196,7 +175,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] send_to = validate_and_format_recipient(send_to=form_send_to, - key_type=api_key.key_type, + key_type='test', service=service, notification_type=notification_type) @@ -244,8 +223,8 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ service=service, personalisation=personalisation, notification_type=notification_type, - api_key_id=api_key.id, - key_type=api_key.key_type, + api_key_id=None, + key_type='test', client_reference=form.get('reference', None), simulated=simulated, reply_to_text=reply_to_text,