jaeger wip

app/__init__.py

@@ -1,23 +1,21 @@
-import time
 import os
 import random
 import string
 import uuid
 
-from celery import current_task
-from flask import _request_ctx_stack, request, g, jsonify, make_response, current_app, has_request_context
+from flask import _request_ctx_stack, request, g, jsonify, make_response
 from flask_sqlalchemy import SQLAlchemy as _SQLAlchemy
 from flask_marshmallow import Marshmallow
 from flask_migrate import Migrate
+from flask_opentracing import FlaskTracer
 from gds_metrics import GDSMetrics
-from gds_metrics.metrics import Gauge, Histogram
+from jaeger_client import Config as JaegerConfig
 from time import monotonic
 from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient
 from notifications_utils.clients.statsd.statsd_client import StatsdClient
 from notifications_utils.clients.redis.redis_client import RedisClient
 from notifications_utils.clients.encryption.encryption_client import Encryption
 from notifications_utils import logging, request_helper
-from sqlalchemy import event
 from werkzeug.exceptions import HTTPException as WerkzeugHTTPException
 from werkzeug.local import LocalProxy
 
@@ -67,11 +65,7 @@ clients = Clients()
 
 api_user = LocalProxy(lambda: _request_ctx_stack.top.api_user)
 authenticated_service = LocalProxy(lambda: _request_ctx_stack.top.authenticated_service)
-
-CONCURRENT_REQUESTS = Gauge(
-    'concurrent_web_request_count',
-    'How many concurrent requests are currently being served',
-)
+flask_tracer = FlaskTracer(initialize_tracer, False)
 
 
 def create_app(application):
@@ -113,13 +107,13 @@ def create_app(application):
     register_blueprint(application)
     register_v2_blueprints(application)
 
+    flask_tracer = FlaskTracer(initialize_tracer, True, application)
+
+
     # avoid circular imports by importing this file later
     from app.commands import setup_commands
     setup_commands(application)
 
-    # set up sqlalchemy events
-    setup_sqlalchemy_events(application)
-
     return application
 
 
@@ -267,22 +261,17 @@ def register_v2_blueprints(application):
 
 
 def init_app(app):
-
     @app.before_request
     def record_user_agent():
         statsd_client.incr("user-agent.{}".format(process_user_agent(request.headers.get('User-Agent', None))))
 
     @app.before_request
     def record_request_details():
-        CONCURRENT_REQUESTS.inc()
-
         g.start = monotonic()
         g.endpoint = request.endpoint
 
     @app.after_request
     def after_request(response):
-        CONCURRENT_REQUESTS.dec()
-
         response.headers.add('Access-Control-Allow-Origin', '*')
         response.headers.add('Access-Control-Allow-Headers', 'Content-Type,Authorization')
         response.headers.add('Access-Control-Allow-Methods', 'GET,PUT,POST,DELETE')
@@ -329,82 +318,11 @@ def process_user_agent(user_agent_string):
     else:
         return "unknown"
 
-
-def setup_sqlalchemy_events(app):
-
-    TOTAL_DB_CONNECTIONS = Gauge(
-        'db_connection_total_connected',
-        'How many db connections are currently held (potentially idle) by the server',
+def initialize_tracer():
+    config = JaegerConfig(
+        config={
+            "sampler": {"type": "const", "param": 1}
+        },
+        service_name="notify-api"
     )
-
-    TOTAL_CHECKED_OUT_DB_CONNECTIONS = Gauge(
-        'db_connection_total_checked_out',
-        'How many db connections are currently checked out by web requests',
-    )
-
-    DB_CONNECTION_OPEN_DURATION_SECONDS = Histogram(
-        'db_connection_open_duration_seconds',
-        'How long db connections are held open for in seconds',
-        ['method', 'host', 'path']
-    )
-
-    # need this or db.engine isn't accessible
-    with app.app_context():
-        @event.listens_for(db.engine, 'connect')
-        def connect(dbapi_connection, connection_record):
-            # connection first opened with db
-            TOTAL_DB_CONNECTIONS.inc()
-
-        @event.listens_for(db.engine, 'close')
-        def close(dbapi_connection, connection_record):
-            # connection closed (probably only happens with overflow connections)
-            TOTAL_DB_CONNECTIONS.dec()
-
-        @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()
-
-            # 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
-
-            # 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',
-                }
-
-        @event.listens_for(db.engine, 'checkin')
-        def checkin(dbapi_connection, connection_record):
-            # 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']
-
-            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)
+    return config.initialize_tracer()

app/authentication/auth.py

@@ -6,18 +6,12 @@ from notifications_python_client.errors import (
 from notifications_utils import request_helper
 from sqlalchemy.exc import DataError
 from sqlalchemy.orm.exc import NoResultFound
-from gds_metrics import Histogram
 
 from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys
 
 
 GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header'  # noqa
 
-AUTH_DB_CONNECTION_DURATION_SECONDS = Histogram(
-    'auth_db_connection_duration_seconds',
-    'Time taken to get DB connection and fetch service from database',
-)
-
 
 class AuthError(Exception):
     def __init__(self, message, code, service_id=None, api_key_id=None):
@@ -93,8 +87,7 @@ def requires_auth():
     issuer = __get_token_issuer(auth_token)  # ie the `iss` claim which should be a service ID
 
     try:
-        with AUTH_DB_CONNECTION_DURATION_SECONDS.time():
-            service = dao_fetch_service_by_id_with_api_keys(issuer)
+        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:

app/celery/celery.py

@@ -1,6 +1,5 @@
 import time
 
-from gds_metrics.metrics import Histogram
 from celery import Celery, Task
 from celery.signals import worker_process_shutdown
 from flask import g, request
@@ -20,12 +19,6 @@ def log_on_worker_shutdown(sender, signal, pid, exitcode, **kwargs):
 
 
 def make_task(app):
-    SQS_APPLY_ASYNC_DURATION_SECONDS = Histogram(
-        'sqs_apply_async_duration_seconds',
-        'Time taken to put task on queue',
-        ['task_name']
-    )
-
     class NotifyTask(Task):
         abstract = True
         start = None
@@ -59,8 +52,7 @@ def make_task(app):
             if has_request_context() and hasattr(request, 'request_id'):
                 kwargs['request_id'] = request.request_id
 
-            with SQS_APPLY_ASYNC_DURATION_SECONDS.labels(self.name).time():
-                return super().apply_async(args, kwargs, task_id, producer, link, link_error, **options)
+            return super().apply_async(args, kwargs, task_id, producer, link, link_error, **options)
 
     return NotifyTask
 

app/dao/organisation_dao.py

@@ -1,6 +1,6 @@
 from sqlalchemy.sql.expression import func
 
-from app import db
+from app import db, flask_tracer
 from app.dao.dao_utils import VersionOptions, transactional, version_class
 from app.models import (
     Organisation,
@@ -17,6 +17,7 @@ def dao_get_organisations():
     ).all()
 
 
+@flask_tracer.trace()
 def dao_count_organisations_with_live_services():
     return db.session.query(Organisation.id).join(Organisation.services).filter(
         Service.active.is_(True),

app/delivery/send_to_providers.py

@@ -72,8 +72,8 @@ def send_sms_to_provider(notification):
                 notification.billable_units = template.fragment_count
                 update_notification_to_sending(notification, provider)
 
-        delta_seconds = (datetime.utcnow() - notification.created_at).total_seconds()
-        statsd_client.timing("sms.total-time", delta_seconds)
+        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
+        statsd_client.timing("sms.total-time", delta_milliseconds)
 
 
 def send_email_to_provider(notification):
@@ -118,8 +118,8 @@ def send_email_to_provider(notification):
             notification.reference = reference
             update_notification_to_sending(notification, provider)
 
-        delta_seconds = (datetime.utcnow() - notification.created_at).total_seconds()
-        statsd_client.timing("email.total-time", delta_seconds)
+        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
+        statsd_client.timing("email.total-time", delta_milliseconds)
 
 
 def update_notification_to_sending(notification, provider):

app/notifications/process_letter_notifications.py

@@ -5,9 +5,7 @@ from app.models import LETTER_TYPE
 from app.notifications.process_notifications import persist_notification
 
 
-def create_letter_notification(
-    letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None
-):
+def create_letter_notification(letter_data, template, api_key, status, reply_to_text=None, billable_units=None):
     notification = persist_notification(
         template_id=template.id,
         template_version=template.version,
@@ -26,7 +24,6 @@ def create_letter_notification(
         status=status,
         reply_to_text=reply_to_text,
         billable_units=billable_units,
-        postage=letter_data.get('postage'),
-        updated_at=updated_at
+        postage=letter_data.get('postage')
     )
     return notification

app/notifications/process_notifications.py

@@ -34,15 +34,6 @@ from app.dao.notifications_dao import (
 from app.v2.errors import BadRequestError
 
 
-from gds_metrics import Histogram
-
-
-REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
-    'redis_get_and_incr_daily_limit_duration_seconds',
-    'Time taken to get and possibly incremement the daily limit cache key',
-)
-
-
 def create_content_for_notification(template, personalisation):
     template_object = template._as_utils_template_with_personalisation(personalisation)
     check_placeholders(template_object)
@@ -79,8 +70,7 @@ def persist_notification(
     billable_units=None,
     postage=None,
     template_postage=None,
-    document_download_count=None,
-    updated_at=None
+    document_download_count=None
 ):
     notification_created_at = created_at or datetime.utcnow()
     if not notification_id:
@@ -106,7 +96,6 @@ def persist_notification(
         reply_to_text=reply_to_text,
         billable_units=billable_units,
         document_download_count=document_download_count,
-        updated_at=updated_at
     )
 
     if notification_type == SMS_TYPE:
@@ -122,17 +111,12 @@ def persist_notification(
         notification.postage = postage or template_postage
         notification.normalised_to = ''.join(notification.to.split()).lower()
 
-    # Get service attributes before the commit
-    service_in_trial_mode = service.restricted
-    service_id = service.id
-
     # if simulated create a Notification model to return but do not persist the Notification to the dB
     if not simulated:
         dao_create_notification(notification)
-        # Only keep track of the daily limit for trial mode services.
-        if service_in_trial_mode and key_type != KEY_TYPE_TEST:
-            if redis_store.get(redis.daily_limit_cache_key(service_id)):
-                redis_store.incr(redis.daily_limit_cache_key(service_id))
+        if key_type != KEY_TYPE_TEST:
+            if redis_store.get(redis.daily_limit_cache_key(service.id)):
+                redis_store.incr(redis.daily_limit_cache_key(service.id))
 
         current_app.logger.info(
             "{} {} created at {}".format(notification_type, notification_id, notification_created_at)
@@ -140,43 +124,35 @@ def persist_notification(
     return notification
 
 
-def send_notification_to_queue_detached(
-    key_type, notification_type, notification_id, research_mode, queue=None
-):
-    if research_mode or key_type == KEY_TYPE_TEST:
+def send_notification_to_queue(notification, research_mode, queue=None):
+    if research_mode or notification.key_type == KEY_TYPE_TEST:
         queue = QueueNames.RESEARCH_MODE
 
-    if notification_type == SMS_TYPE:
+    if notification.notification_type == SMS_TYPE:
         if not queue:
             queue = QueueNames.SEND_SMS
         deliver_task = provider_tasks.deliver_sms
-    if notification_type == EMAIL_TYPE:
+    if notification.notification_type == EMAIL_TYPE:
         if not queue:
             queue = QueueNames.SEND_EMAIL
         deliver_task = provider_tasks.deliver_email
-    if notification_type == LETTER_TYPE:
+    if notification.notification_type == LETTER_TYPE:
         if not queue:
             queue = QueueNames.CREATE_LETTERS_PDF
         deliver_task = get_pdf_for_templated_letter
 
     try:
-        deliver_task.apply_async([str(notification_id)], queue=queue)
+        deliver_task.apply_async([str(notification.id)], queue=queue)
     except Exception:
-        dao_delete_notifications_by_id(notification_id)
+        dao_delete_notifications_by_id(notification.id)
         raise
 
     current_app.logger.debug(
-        "{} {} sent to the {} queue for delivery".format(notification_type,
-                                                         notification_id,
+        "{} {} sent to the {} queue for delivery".format(notification.notification_type,
+                                                         notification.id,
                                                          queue))
 
 
-def send_notification_to_queue(notification, research_mode, queue=None):
-    send_notification_to_queue_detached(
-        notification.key_type, notification.notification_type, notification.id, research_mode, queue
-    )
-
-
 def simulated_recipient(to_address, notification_type):
     if notification_type == SMS_TYPE:
         formatted_simulated_numbers = [

app/notifications/validators.py

@@ -22,24 +22,15 @@ from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id
 
-from gds_metrics.metrics import Histogram
-
-
-REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS = Histogram(
-    'redis_exceeded_rate_limit_duration_seconds',
-    'Time taken to check rate limit',
-)
-
 
 def check_service_over_api_rate_limit(service, api_key):
     if current_app.config['API_RATE_LIMIT_ENABLED'] and current_app.config['REDIS_ENABLED']:
         cache_key = rate_limit_cache_key(service.id, api_key.key_type)
         rate_limit = service.rate_limit
         interval = 60
-        with REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS.time():
-            if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval):
-                current_app.logger.info("service {} has been rate limited for throughput".format(service.id))
-                raise RateLimitError(rate_limit, interval, api_key.key_type)
+        if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval):
+            current_app.logger.info("service {} has been rate limited for throughput".format(service.id))
+            raise RateLimitError(rate_limit, interval, api_key.key_type)
 
 
 def check_service_over_daily_message_limit(key_type, service):

app/schemas.py

@@ -235,18 +235,6 @@ class ServiceSchema(BaseSchema):
             'letter_contacts',
             'complaints',
             'data_retention',
-            'all_template_folders',
-            'annual_billing',
-            'contact_list',
-            'crown',
-            'inbound_number',
-            'inbound_sms',
-            'letter_logo_filename',
-            'rate_limit',
-            'returned_letters',
-            'users',
-            'version',
-            'whitelist',
         )
         strict = True
 
@@ -301,18 +289,7 @@ class DetailedServiceSchema(BaseSchema):
             'sms_sender',
             'permissions',
             'inbound_number',
-            'inbound_sms',
-            'all_template_folders',
-            'annual_billing',
-            'contact_list',
-            'created_by',
-            'crown',
-            'letter_logo_filename',
-            'rate_limit',
-            'returned_letters',
-            'users',
-            'version',
-            'whitelist',
+            'inbound_sms'
         )
 
 

app/v2/notifications/create_response.py

@@ -1,70 +1,45 @@
 
 
-def create_post_sms_response_from_notification(
-    notification_id, client_reference, template_id, template_version, service_id,
-    content, from_number, url_root, scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         'from_number': from_number,
         'body': content
     }
-    return resp
+    return noti
 
 
-def create_post_email_response_from_notification(
-    notification_id,
-    client_reference,
-    template_id,
-    template_version,
-    service_id,
-    content,
-    subject,
-    email_from,
-    url_root,
-    scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         "from_email": email_from,
         "body": content,
         "subject": subject
     }
-    return resp
+    return noti
 
 
-def create_post_letter_response_from_notification(
-    notification_id, client_reference, template_id, template_version, service_id,
-    content, subject, url_root, scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         "body": content,
         "subject": subject
     }
-    return resp
+    return noti
 
 
-def __create_notification_response(
-    notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-):
+def __create_notification_response(notification, url_root, scheduled_for):
     return {
-        "id": notification_id,
-        "reference": client_reference,
-        "uri": "{}v2/notifications/{}".format(url_root, str(notification_id)),
+        "id": notification.id,
+        "reference": notification.client_reference,
+        "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)),
         'template': {
-            "id": template_id,
-            "version": template_version,
+            "id": notification.template_id,
+            "version": notification.template_version,
             "uri": "{}services/{}/templates/{}".format(
                 url_root,
-                str(service_id),
-                str(template_id)
+                str(notification.service_id),
+                str(notification.template_id)
             )
         },
         "scheduled_for": scheduled_for if scheduled_for else None

app/v2/notifications/post_notifications.py

@@ -7,7 +7,6 @@ from boto.exception import SQSError
 from flask import request, jsonify, current_app, abort
 from notifications_utils.postal_address import PostalAddress
 from notifications_utils.recipients import try_validate_and_format_phone_number
-from gds_metrics import Histogram
 
 from app import (
     api_user,
@@ -15,13 +14,14 @@ from app import (
     notify_celery,
     document_download_client,
     encryption,
-    DATETIME_FORMAT,
-    db)
+    DATETIME_FORMAT
+)
 from app.celery.letters_pdf_tasks import get_pdf_for_templated_letter, sanitise_letter
 from app.celery.research_mode_tasks import create_fake_letter_response_file
 from app.celery.tasks import save_api_email
 from app.clients.document_download import DocumentDownloadError
 from app.config import QueueNames, TaskNames
+from app.dao.notifications_dao import update_notification_status_by_reference
 from app.dao.templates_dao import get_precompiled_letter_template
 from app.letters.utils import upload_letter_pdf
 from app.models import (
@@ -44,8 +44,9 @@ from app.notifications.process_letter_notifications import (
 from app.notifications.process_notifications import (
     persist_notification,
     persist_scheduled_notification,
-    simulated_recipient,
-    send_notification_to_queue_detached)
+    send_notification_to_queue,
+    simulated_recipient
+)
 from app.notifications.validators import (
     check_if_service_can_send_files_by_email,
     check_rate_limiting,
@@ -60,8 +61,10 @@ from app.schema_validation import validate
 from app.v2.errors import BadRequestError, ValidationError
 from app.v2.notifications import v2_notification_blueprint
 from app.v2.notifications.create_response import (
-    create_post_sms_response_from_notification, create_post_email_response_from_notification,
-    create_post_letter_response_from_notification)
+    create_post_sms_response_from_notification,
+    create_post_email_response_from_notification,
+    create_post_letter_response_from_notification
+)
 from app.v2.notifications.notification_schemas import (
     post_sms_request,
     post_email_request,
@@ -71,12 +74,6 @@ from app.v2.notifications.notification_schemas import (
 from app.v2.utils import get_valid_json
 
 
-POST_NOTIFICATION_JSON_PARSE_DURATION_SECONDS = Histogram(
-    'post_notification_json_parse_duration_seconds',
-    'Time taken to parse and validate post request json',
-)
-
-
 @v2_notification_blueprint.route('/{}'.format(LETTER_TYPE), methods=['POST'])
 def post_precompiled_letter_notification():
     request_json = get_valid_json()
@@ -104,27 +101,31 @@ def post_precompiled_letter_notification():
         letter_data=form,
         api_key=api_user,
         template=template,
-        template_with_content=None,  # not required for precompiled
         reply_to_text=reply_to,
         precompiled=True
     )
 
-    return jsonify(notification), 201
+    resp = {
+        'id': notification.id,
+        'reference': notification.client_reference,
+        'postage': notification.postage
+    }
+
+    return jsonify(resp), 201
 
 
 @v2_notification_blueprint.route('/<notification_type>', methods=['POST'])
 def post_notification(notification_type):
-    with POST_NOTIFICATION_JSON_PARSE_DURATION_SECONDS.time():
-        request_json = get_valid_json()
+    request_json = get_valid_json()
 
-        if notification_type == EMAIL_TYPE:
-            form = validate(request_json, post_email_request)
-        elif notification_type == SMS_TYPE:
-            form = validate(request_json, post_sms_request)
-        elif notification_type == LETTER_TYPE:
-            form = validate(request_json, post_letter_request)
-        else:
-            abort(404)
+    if notification_type == EMAIL_TYPE:
+        form = validate(request_json, post_email_request)
+    elif notification_type == SMS_TYPE:
+        form = validate(request_json, post_sms_request)
+    elif notification_type == LETTER_TYPE:
+        form = validate(request_json, post_letter_request)
+    else:
+        abort(404)
 
     check_service_has_permission(notification_type, authenticated_service.permissions)
 
@@ -148,7 +149,6 @@ def post_notification(notification_type):
             letter_data=form,
             api_key=api_user,
             template=template,
-            template_with_content=template_with_content,
             reply_to_text=reply_to
         )
     else:
@@ -156,19 +156,41 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template_with_content,
-            template_process_type=template.process_type,
+            template=template,
             service=authenticated_service,
             reply_to_text=reply_to
         )
 
-    return jsonify(notification), 201
+        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,
+        )
+    elif notification_type == EMAIL_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_email_response_from_notification,
+            subject=template_with_content.subject,
+            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
+        )
+    elif notification_type == LETTER_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_letter_response_from_notification,
+            subject=template_with_content.subject,
+        )
+
+    resp = create_resp_partial(
+        notification=notification,
+        url_root=request.url_root,
+        scheduled_for=scheduled_for,
+        content=template_with_content.content_with_placeholders_filled_in,
+    )
+    return jsonify(resp), 201
 
 
-def process_sms_or_email_notification(
-    *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None
-):
-    notification_id = uuid.uuid4()
+def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None):
+    notification_id = None
     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,
@@ -184,46 +206,27 @@ def process_sms_or_email_notification(
         service,
         simulated=simulated
     )
-    if document_download_count:
-        # We changed personalisation which means we need to update the content
-        template.values = personalisation
-    api_key_id = api_key.id
-    key_type = api_key.key_type
-    service_in_research_mode = service.research_mode
-    template_version = template._template['version']
-    resp = create_response_for_post_notification(
-        notification_id=notification_id,
-        client_reference=form.get('reference', None),
-        template_id=template.id,
-        template_version=template._template['version'],
-        service_id=service.id,
-        notification_type=notification_type,
-        reply_to=reply_to_text,
-        scheduled_for=form.get("scheduled_for", None),
-        template_with_content=template)
 
-    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and key_type == KEY_TYPE_NORMAL \
+    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \
        and notification_type == EMAIL_TYPE:
         # Put GOV.UK Email notifications onto a queue
         # To take the pressure off the db for API requests put the notification for our high volume service onto a queue
         # the task will then save the notification, then call send_notification_to_queue.
         # We know that this team does not use the GET request, but relies on callbacks to get the status updates.
         try:
-            db.session.commit()
-            save_email_to_queue(
+            notification_id = uuid.uuid4()
+            notification = save_email_to_queue(
                 form=form,
                 notification_id=str(notification_id),
                 notification_type=notification_type,
-                api_key_id=api_key_id,
-                key_type=key_type,
-                template_id=template.id,
-                template_version=template_version,
+                api_key=api_key,
+                template=template,
                 service_id=service.id,
                 personalisation=personalisation,
                 document_download_count=document_download_count,
                 reply_to_text=reply_to_text
             )
-            return resp
+            return notification
         except SQSError:
             # if SQS cannot put the task on the queue, it's probably because the notification body was too long and it
             # went over SQS's 256kb message limit. If so, we
@@ -231,16 +234,16 @@ def process_sms_or_email_notification(
                 f'Notification {notification_id} failed to save to high volume queue. Using normal flow instead'
             )
 
-    persist_notification(
+    notification = persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template_version,
+        template_version=template.version,
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
         notification_type=notification_type,
-        api_key_id=api_key_id,
-        key_type=key_type,
+        api_key_id=api_key.id,
+        key_type=api_key.key_type,
         client_reference=form.get('reference', None),
         simulated=simulated,
         reply_to_text=reply_to_text,
@@ -249,21 +252,19 @@ def process_sms_or_email_notification(
 
     scheduled_for = form.get("scheduled_for", None)
     if scheduled_for:
-        persist_scheduled_notification(notification_id, form["scheduled_for"])
+        persist_scheduled_notification(notification.id, form["scheduled_for"])
     else:
         if not simulated:
-            queue_name = QueueNames.PRIORITY if template_process_type == PRIORITY else None
-            send_notification_to_queue_detached(
-                key_type=key_type,
-                notification_type=notification_type,
-                notification_id=notification_id,
-                research_mode=service_in_research_mode,  # research_mode is deprecated
+            queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
+            send_notification_to_queue(
+                notification=notification,
+                research_mode=service.research_mode,
                 queue=queue_name
             )
         else:
-            current_app.logger.debug("POST simulated notification for id: {}".format(notification_id))
+            current_app.logger.debug("POST simulated notification for id: {}".format(notification.id))
 
-    return resp
+    return notification
 
 
 def save_email_to_queue(
@@ -271,26 +272,23 @@ def save_email_to_queue(
     notification_id,
     form,
     notification_type,
-    api_key_id,
-    key_type,
-    template_id,
-    template_version,
+    api_key,
+    template,
     service_id,
     personalisation,
     document_download_count,
     reply_to_text=None
 ):
-    db.session.commit()
     data = {
         "id": notification_id,
-        "template_id": str(template_id),
-        "template_version": template_version,
+        "template_id": str(template.id),
+        "template_version": template.version,
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
         "notification_type": notification_type,
-        "api_key_id": str(api_key_id),
-        "key_type": key_type,
+        "api_key_id": str(api_key.id),
+        "key_type": api_key.key_type,
         "client_reference": form.get('reference', None),
         "reply_to_text": reply_to_text,
         "document_download_count": document_download_count,
@@ -335,9 +333,7 @@ def process_document_uploads(personalisation_data, service, simulated=False):
     return personalisation_data, len(file_keys)
 
 
-def process_letter_notification(
-    *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False
-):
+def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False):
     if api_key.key_type == KEY_TYPE_TEAM:
         raise BadRequestError(message='Cannot send letters with a team api key', status_code=403)
 
@@ -350,68 +346,21 @@ def process_letter_notification(
                                                         template=template,
                                                         reply_to_text=reply_to_text)
 
-    validate_address(api_key, letter_data)
-
-    test_key = api_key.key_type == KEY_TYPE_TEST
-
-    status = NOTIFICATION_CREATED
-    updated_at = None
-    if test_key:
-        # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
-        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-            status = NOTIFICATION_SENDING
-        # mark test letter as delivered and do not create a fake response later
-        else:
-            status = NOTIFICATION_DELIVERED
-            updated_at = datetime.utcnow()
-
-    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
-
-    notification = create_letter_notification(letter_data=letter_data,
-                                              template=template,
-                                              api_key=api_key,
-                                              status=status,
-                                              reply_to_text=reply_to_text,
-                                              updated_at=updated_at
-                                              )
-
-    get_pdf_for_templated_letter.apply_async(
-        [str(notification.id)],
-        queue=queue
-    )
-
-    if test_key and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-        create_fake_letter_response_file.apply_async(
-            (notification.reference,),
-            queue=queue
-        )
-    resp = create_response_for_post_notification(
-        notification_id=notification.id,
-        client_reference=notification.client_reference,
-        template_id=notification.template_id,
-        template_version=notification.template_version,
-        notification_type=notification.notification_type,
-        reply_to=reply_to_text,
-        scheduled_for=letter_data.get('scheduled_for', None),
-        service_id=notification.service_id,
-        template_with_content=template_with_content
-    )
-    return resp
-
-
-def validate_address(api_key, letter_data):
     address = PostalAddress.from_personalisation(
         letter_data['personalisation'],
         allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS),
     )
+
     if not address.has_enough_lines:
         raise ValidationError(
             message=f'Address must be at least {PostalAddress.MIN_LINES} lines'
         )
+
     if address.has_too_many_lines:
         raise ValidationError(
             message=f'Address must be no more than {PostalAddress.MAX_LINES} lines'
         )
+
     if not address.has_valid_last_line:
         if address.allow_international_letters:
             raise ValidationError(
@@ -421,6 +370,34 @@ def validate_address(api_key, letter_data):
             message='Must be a real UK postcode'
         )
 
+    test_key = api_key.key_type == KEY_TYPE_TEST
+
+    # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
+    status = NOTIFICATION_CREATED if not test_key else NOTIFICATION_SENDING
+    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
+
+    notification = create_letter_notification(letter_data=letter_data,
+                                              template=template,
+                                              api_key=api_key,
+                                              status=status,
+                                              reply_to_text=reply_to_text)
+
+    get_pdf_for_templated_letter.apply_async(
+        [str(notification.id)],
+        queue=queue
+    )
+
+    if test_key:
+        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+            create_fake_letter_response_file.apply_async(
+                (notification.reference,),
+                queue=queue
+            )
+        else:
+            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
+
+    return notification
+
 
 def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
     try:
@@ -435,12 +412,6 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
                                               status=status,
                                               reply_to_text=reply_to_text)
 
-    resp = {
-        'id': notification.id,
-        'reference': notification.client_reference,
-        'postage': notification.postage
-    }
-
     filename = upload_letter_pdf(notification, letter_content, precompiled=True)
 
     current_app.logger.info('Calling task scan-file for {}'.format(filename))
@@ -459,7 +430,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
             queue=QueueNames.LETTERS
         )
 
-    return resp
+    return notification
 
 
 def get_reply_to_text(notification_type, form, template):
@@ -484,31 +455,3 @@ def get_reply_to_text(notification_type, form, template):
         reply_to = template.get_reply_to_text()
 
     return reply_to
-
-
-def create_response_for_post_notification(notification_id, client_reference, template_id, template_version, service_id,
-                                          notification_type, reply_to, scheduled_for,
-                                          template_with_content):
-    if notification_type == SMS_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_sms_response_from_notification,
-            from_number=reply_to,
-        )
-    elif notification_type == EMAIL_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_email_response_from_notification,
-            subject=template_with_content.subject,
-            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
-        )
-    elif notification_type == LETTER_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_letter_response_from_notification,
-            subject=template_with_content.subject,
-        )
-    resp = create_resp_partial(
-        notification_id, client_reference, template_id, template_version, service_id,
-        url_root=request.url_root,
-        scheduled_for=scheduled_for,
-        content=template_with_content.content_with_placeholders_filled_in,
-    )
-    return resp

requirements-app.txt

@@ -20,14 +20,14 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors
 psycopg2-binary==2.8.5
 PyJWT==1.7.1
 SQLAlchemy==1.3.17
+Flask-Opentracing==1.1.0
+jaeger-client==4.3.0
 
 notifications-python-client==5.5.1
 
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0
+git+https://github.com/alphagov/notifications-utils.git@39.4.3#egg=notifications-utils==39.4.3
 
-# gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
-prometheus-client==0.7.1
 gds-metrics==0.2.0

requirements.txt

@@ -22,16 +22,16 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors
 psycopg2-binary==2.8.5
 PyJWT==1.7.1
 SQLAlchemy==1.3.17
+Flask-Opentracing==1.1.0
+jaeger-client==4.3.0
 
 notifications-python-client==5.5.1
 
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0
+git+https://github.com/alphagov/notifications-utils.git@39.4.3#egg=notifications-utils==39.4.3
 
-# gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
-prometheus-client==0.7.1
 gds-metrics==0.2.0
 
 ## The following requirements were added by pip freeze:
@@ -39,15 +39,14 @@ alembic==1.4.2
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.18.82
+awscli==1.18.76
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.4
 blinker==1.4
 boto==2.49.0
 boto3==1.10.38
-botocore==1.17.5
-cachetools==4.1.0
+botocore==1.16.26
 certifi==2020.4.5.2
 chardet==3.0.4
 click==7.1.2
@@ -67,8 +66,10 @@ Mako==1.1.3
 MarkupSafe==1.1.1
 mistune==0.8.4
 monotonic==1.5
+opentracing==2.3.0
 orderedset==2.0.1
 phonenumbers==8.11.2
+prometheus-client==0.2.0
 pyasn1==0.4.8
 pycparser==2.20
 PyPDF2==1.26.0
@@ -79,12 +80,15 @@ python-json-logger==0.1.11
 pytz==2020.1
 PyYAML==5.3.1
 redis==3.5.3
-requests==2.24.0
+requests==2.23.0
 rsa==3.4.2
 s3transfer==0.3.3
 six==1.15.0
 smartypants==2.0.1
 statsd==3.3.0
+threadloop==1.0.2
+thrift==0.13.0
+tornado==6.0.4
 urllib3==1.25.9
 webencodings==0.5.1
 Werkzeug==1.0.1

tests/app/notifications/test_process_notification.py

@@ -23,7 +23,7 @@ from app.notifications.process_notifications import (
 )
 from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address
 from app.v2.errors import BadRequestError
-from tests.app.db import create_service, create_template, create_api_key
+from tests.app.db import create_service, create_template
 
 
 def test_create_content_for_notification_passes(sample_email_template):
@@ -51,7 +51,8 @@ def test_create_content_for_notification_allows_additional_personalisation(sampl
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job):
+def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job, mocker):
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get')
 
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
@@ -90,6 +91,8 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
     assert notification_from_db.created_by_id == notification.created_by_id
     assert notification_from_db.reply_to_text == sample_template.service.get_default_sms_sender()
 
+    mocked_redis.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count")
+
 
 def test_persist_notification_throws_exception_when_missing_template(sample_api_key):
     assert Notification.query.count() == 0
@@ -154,9 +157,10 @@ def test_persist_notification_does_not_increment_cache_if_test_key(
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_with_optionals(sample_job, sample_api_key):
+def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker):
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get')
     n_id = uuid.uuid4()
     created_at = datetime.datetime(2016, 11, 11, 16, 8, 18)
     persist_notification(
@@ -182,7 +186,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
     persisted_notification.job_id == sample_job.id
     assert persisted_notification.job_row_number == 10
     assert persisted_notification.created_at == created_at
-
+    mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count")
     assert persisted_notification.client_reference == "ref from client"
     assert persisted_notification.reference is None
     assert persisted_notification.international is False
@@ -193,77 +197,44 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(notify_db_session, mocker):
-    service = create_service(restricted=True)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
+def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker):
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None)
 
     persist_notification(
-        template_id=template.id,
-        template_version=template.version,
+        template_id=sample_template.id,
+        template_version=sample_template.version,
         recipient='+447111111111',
-        service=template.service,
+        service=sample_template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
         reference="ref"
     )
     mock_incr.assert_not_called()
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_increments_cache_if_key_exists_and_for_trial_service(
-    notify_db_session, mocker
-):
-    service = create_service(restricted=True)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
+def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker):
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
-                 return_value={template.id, 1})
+                 return_value={sample_template.id, 1})
 
     persist_notification(
-        template_id=template.id,
-        template_version=template.version,
+        template_id=sample_template.id,
+        template_version=sample_template.version,
         recipient='+447111111122',
-        service=template.service,
+        service=sample_template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
         reference="ref2")
 
-    mock_incr.assert_called_once_with(str(service.id) + "-2016-01-01-count", )
-
-
-def test_persist_notification_does_not_increments_cache_live_service(
-    notify_db_session, mocker
-):
-    service = create_service(restricted=False)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
-    mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
-    mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
-    mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
-                 return_value={template.id, 1})
-
-    persist_notification(
-        template_id=template.id,
-        template_version=template.version,
-        recipient='+447111111122',
-        service=template.service,
-        personalisation={},
-        notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
-        reference="ref2")
-
-    assert not mock_incr.called
+    mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", )
 
 
 @pytest.mark.parametrize((

tests/app/service/test_rest.py

@@ -235,6 +235,7 @@ def test_get_service_by_id(admin_request, sample_service):
     assert json_resp['data']['email_branding'] is None
     assert 'branding' not in json_resp['data']
     assert json_resp['data']['prefix_sms'] is True
+    assert json_resp['data']['letter_logo_filename'] is None
 
 
 @pytest.mark.parametrize('detailed', [True, False])
@@ -346,6 +347,7 @@ def test_create_service(
     assert json_resp['data']['name'] == 'created service'
     assert json_resp['data']['email_from'] == 'created.service'
     assert not json_resp['data']['research_mode']
+    assert json_resp['data']['rate_limit'] == 3000
     assert json_resp['data']['letter_branding'] is None
     assert json_resp['data']['count_as_live'] is expected_count_as_live
 
@@ -1223,6 +1225,7 @@ def test_add_existing_user_to_another_service_with_all_permissions(
             )
             assert resp.status_code == 200
             json_resp = resp.json
+            assert str(user_to_add.id) in json_resp['data']['users']
 
             # check user has all permissions
             auth_header = create_authorization_header()

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -245,7 +245,6 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_d
     fake_create_letter_task.assert_called_once_with([str(notification.id)], queue='research-mode-tasks')
     assert not fake_create_dvla_response_task.called
     assert notification.status == NOTIFICATION_DELIVERED
-    assert notification.updated_at is not None
 
 
 @pytest.mark.parametrize('env', [

tests/app/v2/notifications/test_post_notifications.py

@@ -49,7 +49,6 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol
         path='/v2/notifications/sms',
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), auth_header])
-
     assert response.status_code == 201
     resp_json = json.loads(response.get_data(as_text=True))
     assert validate(resp_json, post_sms_response) == resp_json
@@ -425,7 +424,7 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
 ):
     sample = create_template(service=sample_service, template_type=notification_type)
     persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification')
-    deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue_detached')
+    deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue')
     mocker.patch(
         'app.v2.notifications.post_notifications.check_rate_limiting',
         side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE"))
@@ -1006,14 +1005,11 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl
         "template_id": template.id,
         "personalisation": {"message": "Dear citizen, have a nice day"}
     }
-
-    print("******** Start")
     response = client.post(
         path='/v2/notifications/email',
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)]
     )
-    print("********** End")
 
     json_resp = response.get_json()
 

tests/app/v2/test_errors.py

@@ -4,7 +4,7 @@ from sqlalchemy.exc import DataError
 
 
 @pytest.fixture(scope='function')
-def app_for_test():
+def app_for_test(mocker):
     import flask
     from flask import Blueprint
     from app.authentication.auth import AuthError