cds-snc / notification-planning

Project planning for GC Notify Team
5 stars 0 forks source link

Add queue_name to notification #440

Closed sastels closed 2 years ago

sastels commented 2 years ago

Description

As a developer of Notify, I need to be able to see our performance for priority messages vs normal or low priority to determine if we are meeting our SLO.

Our notifications are sent using different queues in an attempt to, for example, not have large bulk distributions slow down 2FA messages. We recently added a column queue_name to our Notifications and NotificationsHistory table, but we did not add code to set the value of the field. We should add code to ensure that in all cases (single or bulk, sent by api or gui) we set the value appropriately. This might be as simple as adding a line to save the value in send_notification_to_queue()

Acceptance Criteria** (Definition of done)

QA

yaelberger-commits commented 2 years ago

Added to Product Backlog

yaelberger-commits commented 2 years ago

check with Jimmy and Steve if it needs further refinement

sastels commented 2 years ago

I think it's fairly clear - we just want to ensure we capture the queue that's used to send the notification.

iokpala commented 2 years ago

@sastels @jimleroyer Are we referring to the Notification[1] and NotificationHistory[2] models respectively?

This card makes mention of the evaluating the performance.. Are we referring to Locusts, celery admin or Notify admin?

Any indication of where these filling would occur?

[1] https://github.com/cds-snc/notification-api/blob/main/app/models.py#L1597 [2] https://github.com/cds-snc/notification-api/blob/main/app/models.py#L1886

sastels commented 2 years ago

We can set the queue_name in the notification object at any time up to when we save it to the database, and I think then it'll get set in both the Notifications table as well as the NotificationHistory table. For example, here in save_emails we set decide what queue we are going to use to send the email (priority, normal, bulk) https://github.com/cds-snc/notification-api/blob/main/app/celery/tasks.py#L479 If we moved this line up to after 461 then we could set queue_name before we do the persist_notifications.

yaelberger-commits commented 2 years ago

Needs refinement

jzbahrai commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @sastels @andrewleith

yaelberger-commits commented 2 years ago

Steve doing another test, then bringing into review

sastels commented 2 years ago

Had to scrap original approach and redo. Seems to be working now. Working on unit tests.

jimleroyer commented 2 years ago

Released in staging, planned to release in production this morning. Steve investigated mismatched expected queue and found 2 results.