mirror of
https://github.com/GSA/notifications-api.git
synced 2026-07-03 07:57:06 -04:00
Remove, cache or hardcode anything that could be cached when sending an email
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.
This commit is contained in:
@@ -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):
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user