Include status in stats about delivery times

Previously these metrics weren't very useful because they could be
skewed by long timings for failed notifications, which can take up
to 72 hours to deliver. I'm intentionally not trying to have a dual
running period (with the old and new names) because:

- We don't use the current stats for anything (checking Grafana).

- The current stats get turned into a "bucket" metric in Prometheus
[1][2], which isn't very useful because it can only tell us the mean
time to deliver, but we're actually interested in percentiles.

Switching to a new naming is an opportunity to fix the raw data and
the way it's aggregated, using the same kind of "summary" metric that
we now use for stats about our Celery tasks [3].

[1]: c330a8ac8a/paas/statsd/statsd-mapping.yml (L82)
[2]: https://prometheus.io/docs/practices/histograms/#quantiles
[3]: https://github.com/alphagov/notifications-aws/pull/890

app/broadcast_message/translators.py

@@ -7,7 +7,6 @@ def cap_xml_to_dict(cap_xml):
     return {
         "msgType": cap.alert.msgType.text,
         "reference": cap.alert.identifier.text,
-        "event": cap.alert.info.event.text,
         "category": cap.alert.info.category.text,
         "expires": cap.alert.info.expires.text,
         "content": cap.alert.info.description.text,

app/celery/process_ses_receipts_tasks.py

@@ -70,7 +70,11 @@ def process_ses_results(self, response):
         statsd_client.incr('callback.ses.{}'.format(notification_status))
 
         if notification.sent_at:
-            statsd_client.timing_with_dates('callback.ses.elapsed-time', datetime.utcnow(), notification.sent_at)
+            statsd_client.timing_with_dates(
+                f'callback.ses.{notification_status}.elapsed-time',
+                datetime.utcnow(),
+                notification.sent_at
+            )
 
         _check_and_queue_callback_task(notification)
 

app/celery/process_sms_client_response_tasks.py

@@ -75,7 +75,7 @@ def _process_for_status(notification_status, client_name, provider_reference, de
 
     if notification.sent_at:
         statsd_client.timing_with_dates(
-            'callback.{}.elapsed-time'.format(client_name.lower()),
+            f'callback.{client_name.lower()}.{notification_status}.elapsed-time',
             datetime.utcnow(),
             notification.sent_at
         )

app/models.py

@@ -2319,7 +2319,6 @@ class BroadcastMessage(db.Model):
     api_key = db.relationship('ApiKey')
 
     reference = db.Column(db.String(255), nullable=True)
-    event = db.Column(db.String(255), nullable=True)
 
     stubbed = db.Column(db.Boolean, nullable=False)
 
@@ -2339,7 +2338,6 @@ class BroadcastMessage(db.Model):
         return {
             'id': str(self.id),
             'reference': self.reference,
-            'event': self.event,
 
             'service_id': str(self.service_id),
 

app/v2/broadcast/broadcast_schemas.py

@@ -4,7 +4,6 @@ post_broadcast_schema = {
     "required": [
         "msgType",
         "reference",
-        "event",
         "category",
         "content",
         "areas",
@@ -17,12 +16,6 @@ post_broadcast_schema = {
                 "null",
             ],
         },
-        "event": {
-            "type": [
-                "string",
-                "null",
-            ],
-        },
         "category": {
             "type": "string",
             "enum": [

app/v2/broadcast/post_broadcast.py

@@ -51,7 +51,6 @@ def create_broadcast():
         service_id=authenticated_service.id,
         content=broadcast_json['content'],
         reference=broadcast_json['reference'],
-        event=broadcast_json['event'],
         areas={
             'names': [
                 area['name'] for area in broadcast_json['areas']

migrations/versions/0362_broadcast_msg_event.py

@@ -1,21 +0,0 @@
-"""
-
-Revision ID: 0362_broadcast_msg_event
-Revises: 0361_new_user_bcast_permissions
-Create Date: 2020-12-04 15:06:22.544803
-
-"""
-from alembic import op
-import sqlalchemy as sa
-from sqlalchemy.dialects import postgresql
-
-revision = '0362_broadcast_msg_event'
-down_revision = '0361_new_user_bcast_permissions'
-
-
-def upgrade():
-    op.add_column('broadcast_message', sa.Column('event', sa.String(length=255), nullable=True))
-
-
-def downgrade():
-    op.drop_column('broadcast_message', 'event')

tests/app/broadcast_message/test_rest.py

@@ -91,26 +91,6 @@ def test_get_broadcast_message_without_template(
     assert response['personalisation'] is None
 
 
-def test_get_broadcast_message_with_event(
-    admin_request,
-    sample_broadcast_service
-):
-    bm = create_broadcast_message(
-        service=sample_broadcast_service,
-        content='emergency broadcast content',
-        event='001 example event',
-    )
-
-    response = admin_request.get(
-        'broadcast_message.get_broadcast_message',
-        service_id=sample_broadcast_service.id,
-        broadcast_message_id=bm.id,
-        _expected_status=200
-    )
-
-    assert response['event'] == '001 example event'
-
-
 def test_get_broadcast_message_404s_if_message_doesnt_exist(admin_request, sample_broadcast_service):
     err = admin_request.get(
         'broadcast_message.get_broadcast_message',
@@ -275,7 +255,6 @@ def test_create_broadcast_message_can_be_created_from_content(admin_request, sam
     assert response['content'] == 'Some content\n€ŷŵ~\n\'\'""---'
     assert response['reference'] == 'abc123'
     assert response['template_id'] is None
-    assert response['event'] is None
 
 
 def test_create_broadcast_message_400s_if_content_and_template_provided(

tests/app/celery/test_process_ses_receipts_tasks.py

@@ -86,7 +86,7 @@ def test_ses_callback_should_update_notification_status(
         assert process_ses_results(ses_notification_callback(reference='ref'))
         assert get_notification_by_id(notification.id).status == 'delivered'
         statsd_client.timing_with_dates.assert_any_call(
-            "callback.ses.elapsed-time", datetime.utcnow(), notification.sent_at
+            "callback.ses.delivered.elapsed-time", datetime.utcnow(), notification.sent_at
         )
         statsd_client.incr.assert_any_call("callback.ses.delivered")
         updated_notification = Notification.query.get(notification.id)

tests/app/db.py

@@ -1119,8 +1119,7 @@ def create_broadcast_message(
     starts_at=None,
     finishes_at=None,
     areas=None,
-    stubbed=False,
-    event=None,
+    stubbed=False
 ):
     if template:
         service = template.service
@@ -1149,8 +1148,7 @@ def create_broadcast_message(
         created_by_id=created_by.id if created_by else service.created_by_id,
         areas=areas or {'ids': [], 'simple_polygons': []},
         content=content,
-        stubbed=stubbed,
-        event=event,
+        stubbed=stubbed
     )
     db.session.add(broadcast_message)
     db.session.commit()

tests/app/notifications/test_process_client_response.py

@@ -144,7 +144,7 @@ def test_process_sms_client_response_records_statsd_metrics(sample_notification,
 
     statsd_client.incr.assert_any_call("callback.firetext.delivered")
     statsd_client.timing_with_dates.assert_any_call(
-        "callback.firetext.elapsed-time", datetime.utcnow(), sample_notification.sent_at
+        "callback.firetext.delivered.elapsed-time", datetime.utcnow(), sample_notification.sent_at
     )
 
 

tests/app/v2/broadcast/test_post_broadcast.py

@@ -96,7 +96,6 @@ def test_valid_post_cap_xml_broadcast_returns_201(
         'closely monitoring the situation throughout the night. '
     )
     assert response_json['reference'] == '50385fcb0ab7aa447bbd46d848ce8466E'
-    assert response_json['event'] == '053/055 Issue Severe Flood Warning EA'
     assert response_json['created_at']  # datetime generated by the DB so can’t freeze it
     assert response_json['created_by_id'] is None
     assert response_json['finishes_at'] is None