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

feat: Call webhooks async and add backoff to webhooks #2932

Merged

Conversation

tushar5526
Copy link
Contributor

@tushar5526 tushar5526 commented Nov 6, 2023

Related to #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 task processor to exponentially backoff webhooks and make webhook calling process async. Earlier it was a sync for loop.

How did you test this code?

Tested it locally, by running the task processor and running faulty and health webhooks. Also, updated test to support the new changes.

Copy link

vercel bot commented Nov 6, 2023

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

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the api Issue related to the REST API label Nov 6, 2023
Copy link
Contributor

github-actions bot commented Nov 6, 2023

Uffizzi Ephemeral Environment deployment-41316

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/2932

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@tushar5526 tushar5526 changed the title Fix/1654/webhooks backoff features feat: Call webhooks async and add backoff to webhooks Nov 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1592919) 95.88% compared to head (9c4e1ec) 95.82%.

Files Patch % Lines
api/webhooks/webhooks.py 94.23% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2932      +/-   ##
==========================================
- Coverage   95.88%   95.82%   -0.06%     
==========================================
  Files        1044     1044              
  Lines       30663    30723      +60     
==========================================
+ Hits        29401    29441      +40     
- Misses       1262     1282      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

api/webhooks/webhooks.py Show resolved Hide resolved
api/webhooks/webhooks.py Outdated Show resolved Hide resolved
api/webhooks/webhooks.py Outdated Show resolved Hide resolved
api/webhooks/webhooks.py Outdated Show resolved Hide resolved
@tushar5526
Copy link
Contributor Author

tushar5526 commented Nov 7, 2023

@khvn26 @matthewelwell
In order to avoid any herding in large projects with a lot of webhooks configured, should I also add in jitter?

Earlier this was synchronous, but now webhooks are scheduled at almost the same time with the same constant backoffs.

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

This looks good to me now! Thanks so much for all your hard work on this @tushar5526.

api/webhooks/webhooks.py Outdated Show resolved Hide resolved
@tushar5526
Copy link
Contributor Author

This looks good to me now! Thanks so much for all your hard work on this @tushar5526.

Learned lots of stuff :D, thanks for the high-quality reviews @matthewelwell @khvn26 !

@tushar5526
Copy link
Contributor Author

Resolved the conflicts, good to go!

@tushar5526
Copy link
Contributor Author

Just checking if there is anything else in this - that needs a second look ?

@dabeeeenster
Copy link
Contributor

cc @khvn26 !

@matthewelwell
Copy link
Contributor

@tushar5526 sorry for the delay in getting this merged, with the holidays and trying to manage the size of releases it's been waiting on the priorities a little bit. I'm merging it now. Thanks again for the hard work on this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants