Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overriding retry policy for SMS high priority #2008

Merged
merged 33 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d5ce6a2
Overriding retry policy for SMS high priority
jimleroyer Oct 24, 2023
1099d85
Added comments + format
jimleroyer Oct 24, 2023
990f339
Fixed ordering
jimleroyer Oct 24, 2023
89731a3
mypy: Added typing to the untyped dict
jimleroyer Oct 25, 2023
00457b7
mypy again
jimleroyer Oct 25, 2023
4804bbb
Fix existing test with new changes
jimleroyer Oct 25, 2023
2d95aa0
Fix existing test with new changes
jimleroyer Oct 25, 2023
6cdd740
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Oct 26, 2023
2dc05b8
Refactoring: rename method
jimleroyer Oct 26, 2023
77dd00f
Refactoring: granular params
jimleroyer Oct 26, 2023
d80087a
Added test around params building for delivery tasks
jimleroyer Oct 26, 2023
eaebf11
Changed test for correct value of retry period for sms high
jimleroyer Oct 26, 2023
cba4269
Introducing feature flag for celery retry policies
jimleroyer Oct 27, 2023
0a5d388
Changed FF name for custom task celery params
jimleroyer Oct 27, 2023
5a144f8
Adding extension to center editor on kb cursor
jimleroyer Oct 27, 2023
74834f7
Forgot the test config to activate env var
jimleroyer Oct 30, 2023
9cc04c2
Use constants in tests
jimleroyer Oct 30, 2023
b51f63f
Reversing env var config enabling logic (but same config)
jimleroyer Oct 30, 2023
dc81e0d
Arf got trick by Python conditional logic
jimleroyer Oct 30, 2023
d36e65f
Print that env var because it aint working
jimleroyer Oct 30, 2023
2fd7be1
Format
jimleroyer Oct 30, 2023
7737bfc
Removing print debug statement
jimleroyer Oct 30, 2023
43b4c47
Merge branch 'main' into feat/decrease-retry-for-sms-high
jimleroyer Nov 1, 2023
77b436f
Apply low retry period for the sms high priority lane on celery retry…
jimleroyer Nov 6, 2023
662112e
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 6, 2023
ff09bed
Merge remote-tracking branch 'origin/feat/decrease-retry-for-sms-high…
jimleroyer Nov 6, 2023
687b790
Refactoring retry periods for sms high priority
jimleroyer Nov 6, 2023
ad01cfd
Fix test
jimleroyer Nov 6, 2023
c82785f
Fix sort
jimleroyer Nov 6, 2023
887d4f3
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 6, 2023
57161f1
Added timonwong.shellcheck shell checker to devcontainer
jimleroyer Nov 6, 2023
d52c173
Removed manually raised exception only meant for testing
jimleroyer Nov 6, 2023
d655af3
Merge remote-tracking branch 'origin/main' into feat/decrease-retry-f…
jimleroyer Nov 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"GitHub.copilot",
"GitHub.copilot-labs",
"googlecloudtools.cloudcode",
"kaiwood.center-editor-window",
"matangover.mypy",
"ms-azuretools.vscode-docker",
"ms-ossdata.vscode-postgresql",
Expand All @@ -35,6 +36,7 @@
"mtxr.sqltools",
"mtxr.sqltools-driver-pg",
"pmbenjamin.vscode-snyk",
"timonwong.shellcheck",
"usernamehw.errorlens",
"visualstudioexptteam.vscodeintellicode",
"wenfangdu.jump",
Expand Down
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"-l",
"DEBUG",
"-Q",
"database-tasks,-priority-database-tasks.fifo,-normal-database-tasks,-bulk-database-tasks,job-tasks,notify-internal-tasks,periodic-tasks,priority-tasks,bulk-tasks,reporting-tasks,research-mode-tasks,retry-tasks,send-sms-tasks,send-email-tasks,service-callbacks,delivery-receipts",
"database-tasks,-priority-database-tasks.fifo,-normal-database-tasks,-bulk-database-tasks,job-tasks,notify-internal-tasks,periodic-tasks,priority-tasks,normal-tasks,bulk-tasks,reporting-tasks,research-mode-tasks,retry-tasks,send-sms-tasks,send-sms-high,send-sms-medium,send-sms-low,send-throttled-sms-tasks,send-email-high,send-email-medium,send-email-low,send-email-tasks,service-callbacks,delivery-receipts",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

]
},
{
Expand Down
3 changes: 2 additions & 1 deletion app/celery/provider_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
NotificationTechnicalFailureException,
)
from app.models import NOTIFICATION_TECHNICAL_FAILURE
from app.notifications import build_retry_task_params
from app.notifications.callbacks import _check_and_queue_callback_task


Expand Down Expand Up @@ -127,7 +128,7 @@ def _deliver_sms(self, notification_id):
# Once the previous retry failed, log the exception and this time,
# retry with the default delay.
current_app.logger.exception("SMS notification delivery for id: {} failed".format(notification_id))
self.retry(queue=QueueNames.RETRY)
self.retry(**build_retry_task_params(notification.notification_type, notification.template.process_type))
except self.MaxRetriesExceededError:
message = (
"RETRY FAILED: Max retries reached. The task send_sms_to_provider failed for notification {}. "
Expand Down
12 changes: 7 additions & 5 deletions app/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,6 @@ class Config(object):
CSV_BULK_REDIRECT_THRESHOLD = os.getenv("CSV_BULK_REDIRECT_THRESHOLD", 200)

# Endpoint of Cloudwatch agent running as a side car in EKS listening for embedded metrics
FF_CLOUDWATCH_METRICS_ENABLED = env.bool("FF_CLOUDWATCH_METRICS_ENABLED", False)
CLOUDWATCH_AGENT_EMF_PORT = 25888
CLOUDWATCH_AGENT_ENDPOINT = os.getenv("CLOUDWATCH_AGENT_ENDPOINT", f"tcp://{STATSD_HOST}:{CLOUDWATCH_AGENT_EMF_PORT}")

Expand All @@ -583,14 +582,15 @@ class Config(object):
BR_WARNING_PERCENTAGE = 0.05
BR_CRITICAL_PERCENTAGE = 0.1

FF_SALESFORCE_CONTACT = env.bool("FF_SALESFORCE_CONTACT", False)

# Feature flags for bounce rate
# Timestamp in epoch milliseconds to seed the bounce rate. We will seed data for (24, the below config) included.
FF_BOUNCE_RATE_SEED_EPOCH_MS = os.getenv("FF_BOUNCE_RATE_SEED_EPOCH_MS", False)

# Feature flag to enable custom retry policies such as lowering retry period for certain priority lanes.
FF_CELERY_CUSTOM_TASK_PARAMS = env.bool("FF_CELERY_CUSTOM_TASK_PARAMS", True)
FF_CLOUDWATCH_METRICS_ENABLED = env.bool("FF_CLOUDWATCH_METRICS_ENABLED", False)
# Feature flags for email_daily_limit
FF_EMAIL_DAILY_LIMIT = env.bool("FF_EMAIL_DAILY_LIMIT", False)
FF_SALESFORCE_CONTACT = env.bool("FF_SALESFORCE_CONTACT", False)

@classmethod
def get_sensitive_config(cls) -> list[str]:
Expand Down Expand Up @@ -687,11 +687,12 @@ class Test(Development):
API_HOST_NAME = "http://localhost:6011"

TEMPLATE_PREVIEW_API_HOST = "http://localhost:9999"
FF_EMAIL_DAILY_LIMIT = False
CRM_GITHUB_PERSONAL_ACCESS_TOKEN = "test-token"
CRM_ORG_LIST_URL = "https://test-url.com"
FAILED_LOGIN_LIMIT = 0

FF_EMAIL_DAILY_LIMIT = False


class Production(Config):
NOTIFY_EMAIL_DOMAIN = os.getenv("NOTIFY_EMAIL_DOMAIN", "notification.canada.ca")
Expand All @@ -708,6 +709,7 @@ class Production(Config):
API_RATE_LIMIT_ENABLED = True
CHECK_PROXY_HEADER = False
CRONITOR_ENABLED = False
FF_CELERY_CUSTOM_TASK_PARAMS = False


class Staging(Production):
Expand Down
3 changes: 3 additions & 0 deletions app/delivery/send_to_providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def send_sms_to_provider(notification):
inactive_service_failure(notification=notification)
return

# If the notification was not sent already, the status should be created.
if notification.status == "created":
provider = provider_to_use(
SMS_TYPE,
Expand Down Expand Up @@ -215,6 +216,8 @@ def send_email_to_provider(notification: Notification):
if not service.active:
inactive_service_failure(notification=notification)
return

# If the notification was not sent already, the status should be created.
if notification.status == "created":
provider = provider_to_use(EMAIL_TYPE, notification.id)

Expand Down
36 changes: 36 additions & 0 deletions app/notifications/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from typing import Any, Dict

from flask import current_app

from app.config import QueueNames
from app.models import BULK, NORMAL, PRIORITY, SMS_TYPE

# Default retry periods for sending notifications.
RETRY_DEFAULT = 300
RETRY_HIGH = 25

RETRY_PERIODS = {
BULK: RETRY_DEFAULT,
NORMAL: RETRY_DEFAULT,
PRIORITY: RETRY_HIGH,
}


def build_retry_task_params(notification_type: str, notification_process_type: str) -> Dict[str, Any]:
"""
Build task params for the sending parameter retry tasks.

If the notification is a high priority SMS, set the retry policy to retry every 25 seconds
else fall back to the default retry policy of retrying every 5 minutes.
"""
params: dict[str, Any] = {"queue": QueueNames.RETRY}
if current_app.config["FF_CELERY_CUSTOM_TASK_PARAMS"] is False:
return params

# Overring the retry policy is only supported for SMS for now;
# email support coming later.
if notification_type == SMS_TYPE:
params["countdown"] = RETRY_PERIODS[notification_process_type]
else:
params["countdown"] = RETRY_DEFAULT
return params
5 changes: 4 additions & 1 deletion app/notifications/process_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,10 @@ def db_save_and_send_notification(notification: Notification):

deliver_task = choose_deliver_task(notification)
try:
deliver_task.apply_async([str(notification.id)], queue=notification.queue_name)
deliver_task.apply_async(
[str(notification.id)],
queue=notification.queue_name,
)
except Exception:
dao_delete_notifications_by_id(notification.id)
raise
Expand Down
1 change: 1 addition & 0 deletions pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ env =
ASSET_DOMAIN=assets.notification.canada.ca
NOTIFY_EMAIL_DOMAIN=notification.canada.ca
AWS_EMF_ENVIRONMENT=local
D:FF_CELERY_CUSTOM_TASK_PARAMS=True
D:FF_SALESFORCE_CONTACT=True
D:FF_CLOUDWATCH_METRICS_ENABLED=True
D:REDIS_URL=redis://localhost:6380
Expand Down
25 changes: 25 additions & 0 deletions tests/app/notifications/test_process_notification.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import datetime
import uuid
from typing import Any, Dict
from unittest.mock import call

import pytest
Expand All @@ -14,12 +15,18 @@
from app.config import QueueNames
from app.dao.service_sms_sender_dao import dao_update_service_sms_sender
from app.models import (
BULK,
EMAIL_TYPE,
LETTER_TYPE,
NORMAL,
PRIORITY,
SMS_TYPE,
Notification,
NotificationHistory,
ScheduledNotification,
Template,
)
from app.notifications import RETRY_PERIODS, build_retry_task_params
from app.notifications.process_notifications import (
choose_queue,
create_content_for_notification,
Expand Down Expand Up @@ -1085,6 +1092,24 @@ def test_db_save_and_send_notification_throws_exception_deletes_notification(
assert Notification.query.count() == 0
assert NotificationHistory.query.count() == 0

@pytest.mark.parametrize(
("notification_type, process_type, expected_retry_period"),
[
(EMAIL_TYPE, BULK, RETRY_PERIODS[BULK]),
(EMAIL_TYPE, NORMAL, RETRY_PERIODS[NORMAL]),
(EMAIL_TYPE, PRIORITY, RETRY_PERIODS[NORMAL]),
(SMS_TYPE, BULK, RETRY_PERIODS[BULK]),
(SMS_TYPE, NORMAL, RETRY_PERIODS[NORMAL]),
(SMS_TYPE, PRIORITY, RETRY_PERIODS[PRIORITY]),
],
)
def test_retry_task_parameters(self, notify_api, notification_type, process_type, expected_retry_period):
with notify_api.app_context():
params: Dict[str, Any] = build_retry_task_params(notification_type, process_type)

assert params["queue"] == QueueNames.RETRY
assert params["countdown"] == expected_retry_period

def test_db_save_and_send_notification_throws_exception_when_missing_template(self, sample_api_key, mocker):
mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
assert Notification.query.count() == 0
Expand Down
Loading