cds-snc / notification-planning-core

Project planning for GC Notify Core Team

0 stars 0 forks source link

Move SMS into their own dedicated queues #186

Open jimleroyer opened 10 months ago

jimleroyer commented 10 months ago

Description

As a Notify developer, I need SMS delivery tasks to be in their own queues so that I can treat them differently than email delivery tasks.

WHY are we building?

The sending rates set by AWS are different for email and sms, and hence we need to be able to tune them separately. We have decided that the best way to do this right now is to have the email and sms delivery tasks run on different groups of pods. This requires they also be placed in different celery queues.

WHAT are we building?

Move SMS delivery tasks to their own dedicated celery queues. Note that this task does not include moving the sms delivery tasks to their own dedicated pool of celery workers (that is the scope of #185)

VALUE created by our solution

We will be able to scale up both email and sms sending rates.

Acceptance Criteria

[x] deliver_sms() tasks go through their own celery queues.
[x] create wait time alarms for the new queues and add to the sms dashboard

QA Steps

[x] verify that smoke tests still pass (Steve)
[x] Verify that the new queues are being used with a cloudwatch query such as this one) (Jimmy)

sastels commented 10 months ago

Looking at a week of prod data from 8/2/2023 to 8/9/2023) we see the following queues used to run deliver_sms:

queue	total
priority-tasks	19497
send-sms-tasks	12784
research-mode-tasks	21
bulk-tasks	30

sastels commented 10 months ago

https://github.com/cds-snc/notification-api/pull/1958

sastels commented 10 months ago

finished changing the tests in the PR. Next will run the thing locally and check out that it still sends sms, and with the new queues.

sastels commented 10 months ago

fixed code for bulk sends and added missing tests

sastels commented 10 months ago

PRs ready for review: use new queues: https://github.com/cds-snc/notification-api/pull/1958 add alarms for new queues: https://github.com/cds-snc/notification-terraform/pull/877

jimleroyer commented 10 months ago

Steve: The PR for API #1958 should make the column storing the used queue closer to the truth (as this is not 100% reliable at the moment).

jimleroyer commented 10 months ago

One PR left to review for API #1958

sastels commented 10 months ago

Alarms merged into staging

sastels commented 10 months ago

new alarms released to prod
new queues merged into staging and tested.

sastels commented 10 months ago

new queues in prod

jimleroyer commented 9 months ago

Seems to be working as most volume passes through the new queues, but still older queues appear for the last week. I will tweak the query to add days on where the older/newer queues were used to have further details on what's being used now.

Private Zenhub Image

jimleroyer commented 9 months ago

Re-running the query for QA'ing the new priority queues, we see that the older queues are only present at day 6th and 7th of the 1 week period that it covers. Afterward, all new queues are used. So that all works good over there.

Private Zenhub Image