feat: Add backoff to webhooks, and call multiple webhooks async #2559

[tushar5526]

Closes #1654

Thanks for submitting a PR! Please check the boxes below:

• I have run pre-commit to check linting
• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have used a Conventional Commit title for this Pull Request

Changes

Use a task processor to schedule each backoff as a task instead of retrying in a single task.
Also, I tried re-using the backoff library to have support for different backoff and jitter options supported in backoff.

How did you test this code?

Ran a task processor and the master server, set up some faulty webhooks and called them.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 16, 2023 9:32am

[vercel]

@tushar5526 is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

[tushar5526]

Some tests will fail, I will fix them after getting this approach reviewed. @gagantrivedi @matthewelwell you can check the latest commit to see the major changes of scheduling retries as a task in task processor.

[github-actions]

Uffizzi Preview deployment-36101 was deleted.

[matthewelwell]

@tushar5526 thanks for this. It seems quite complicated to me, however. I think we can ignore the need to support multiple backoff algorithms and just pick on that we think fits (my preference would be some kind of exponential algorithm I think).

Perhaps I am over simplifying it, but I was imagining that we could achieve this functionality by just wrapping the task which calls the webhook in a try / except (requests.Exception, SomeCustomExceptionForNon200) and scheduling a new task for the expected time in the except clause.

I've not really been able to follow the code in this PR to understand the approach you've taken. Could you add a more detailed description of the approach to the summary of this PR?

[tushar5526]

Perhaps I am over simplifying it, but I was imagining that we could achieve this functionality by just wrapping the task which calls the webhook in a try / except (requests.Exception, SomeCustomExceptionForNon200) and scheduling a new task for the expected time in the except clause.

Agreed, I tried a similar approach in this PR. Let me describe it briefly.

This PR majorly does the following. Provide both async and sync variants for _call_webhook function. We need the sync version of _call_webhook because there are some functions (def call_integration_webhook) that need the return value of _call_webhook function.

_call_webhook_async additionally takes in the following optional parameters.

1. max_tries: int = 3
2. send _error_email: bool = False
3. backoff_type: one of the backoff types supported by backoff library
4. jitter_type: one of the jitter type supported by backoff library

Now some kind of recursive logic is needed to keep scheduling the tasks for retries according to the specified backoffs and jitter techniques.

For this, the _call_webhook_async function has a helper function defined in it, which maintains the number of retries and scheduling of new tasks.

There is one function to calculate the next delay (backoff + jitter time).

Overall 2 main functions from user's point of view that can be used to call webhooks.

_call_webhook_sync - to call a webhook synchronously and get the response value as well.

_call_webhook_async - to call webhook async with optional parameters to control the number of retries, backoff and jitter strategies and whether to send an error email on fail.

Example of how can we call the former.

I think we can ignore the need to support multiple backoff algorithms and just pick on that we think fits (my preference would be some kind of exponential algorithm I think).

I generalised the code to use any back-off strategy as I was unsure of which one would work better in this case, an agreed with sticking to any single approach to reduce complexity.

PS: In my understanding, we can't get the return values of scheduled tasks in the task processor, so each scheduled task for retrying a webhook should contain the state of how many retries have happened so far.

Also, it might help to review the PR in the unified view as the split view is indeed confusing. You can use this link.

[tushar5526]

@matthewelwell should I scrape this PR and make a new one with a simpler implementation?

[matthewelwell]

@matthewelwell should I scrape this PR and make a new one with a simpler implementation?

@tushar5526 If there is a simpler way to implement it then yes, please - sorry, I haven't really had a chance to go over this PR again since your latest explanation.

[tushar5526]

Sure! I will share a simpler implementation then.

[tushar5526]

@matthewelwell here is a simpler implementation - #2932. Closing this one.