manually retry for requests.exceptions.Timeout exceptions when sending outgoing webhooks
#3632
Conversation
exceptions when sending outgoing webhooks
joeyorlando
added
the
pr:no public docs
| autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else 3 | ||
| autoretry_for=(Exception,), | ||
| # This allows to exclude some exceptions that match autoretry_for but for which you don’t want a retry. | ||
| # https://docs.celeryq.dev/en/stable/userguide/tasks.html#Task.dont_autoretry_for | ||
| dont_autoretry_for=(requests.exceptions.Timeout,), | ||
| retry_backoff=True, | ||
| max_retries=1 if settings.DEBUG else 3, |
There was a problem hiding this comment.
main change (rest is just adding some type hints)
There was a problem hiding this comment.
An alternative here would be to not use dont_autoretry_for and instead manually retry the task if isinstance(exception, requests.exceptions.Timeout) (up to a max of 3 after which point we simply return). This would allow to still retry for these tasks but without having to raise an exception (and avoiding triggering our AmixrRetriedFailedTasks alert)
There was a problem hiding this comment.
I think this would be preferred to have some limited retry to cover customer network hiccups
| @@ -52,15 +66,14 @@ def send_webhook_event(trigger_type, alert_group_id, organization_id=None, user_ | |||
| ).exclude(is_webhook_enabled=False) | |||
|
|
|||
| for webhook in webhooks_qs: | |||
| print(webhook.name) | |||
There was a problem hiding this comment.
presumably we don't need this?
| autoretry_for=(Exception,), | ||
| # This allows to exclude some exceptions that match autoretry_for but for which you don’t want a retry. | ||
| # https://docs.celeryq.dev/en/stable/userguide/tasks.html#Task.dont_autoretry_for | ||
| dont_autoretry_for=(requests.exceptions.Timeout,), |
There was a problem hiding this comment.
Catching this error will catch both requests.exceptions.ConnectTimeout and requests.exceptions.ReadTimeout errors (source code):
requests.exceptions.ConnectTimeout: "The request timed out while trying to connect to the remote server"requests.exceptions.ReadTimeout: "The server did not send any data in the allotted amount of time"
both of these seem out of our control
joeyorlando
changed the title
requests.exceptions.Timeout exceptions when sending outgoing webhooksrequests.exceptions.Timeout exceptions when sending outgoing webhooks
….com:grafana/oncall into jorlando/address-outgoing-webhook-exceptions
…ing outgoing webhooks (#3632) # Which issue(s) this PR fixes Fixes grafana/oncall-private#2439 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
Which issue(s) this PR fixes
Fixes https://github.com/grafana/oncall-private/issues/2439
Checklist
pr:no public docsPR label added if not required)CHANGELOG.mdupdated (orpr:no changelogPR label added if not required)