[NoQA] Increase the octokit gh throttling times #22210
Conversation
melvin-bot
bot
requested review from
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| @@ -67,6 +67,7 @@ function getDeployMessage(deployer, deployVerb, prTitle) { | |||
| */ | |||
| function commentPR(PR, message) { | |||
| return GithubUtils.createComment(context.repo.repo, PR, message) | |||
| .then(ActionUtils.sleep(1000)) | |||
There was a problem hiding this comment.
This is the main place we are commenting on many issues/ PRs from here so I think tis hould be fine added only here to start with as maybe the rate limiting wont be such problem in future anymore as we should not have such big deploys as we had when this issue occurred.
Working with JS Promises is not my strength so just wanna highlight this to make sure its correctly implemented
There was a problem hiding this comment.
I think we have to provide () => ActionUtils.sleep(1000), I created a small mock of the code and it was not working and the mentioned change made it work - https://jsfiddle.net/abdulrahuman5196/Loupsxwc/ . Let me know if there is something wrong with the mock.
There was a problem hiding this comment.
Thanks! that makes sense, update the code 🙇
|
Reviewing the PR now |
| @@ -67,6 +67,7 @@ function getDeployMessage(deployer, deployVerb, prTitle) { | |||
| */ | |||
| function commentPR(PR, message) { | |||
| return GithubUtils.createComment(context.repo.repo, PR, message) | |||
| .then(ActionUtils.sleep(1000)) | |||
There was a problem hiding this comment.
I think we have to provide () => ActionUtils.sleep(1000), I created a small mock of the code and it was not working and the mentioned change made it work - https://jsfiddle.net/abdulrahuman5196/Loupsxwc/ . Let me know if there is something wrong with the mock.
|
@mountiny With the last change, this seems to be working in Fiddle with the mock code. Is there any different way i can test this out? not sure on this since it essentially adds comments to PR |
There was a problem hiding this comment.
Code looks good but throwing this out there if we wanted to switch to an async method hah
async function commentPR(PR, message) {
try {
await GithubUtils.createComment(context.repo.repo, PR, message);
await new Promise(r => setTimeout(r, 1000));
console.log(`Comment created on #${PR} successfully 🎉`);
} catch (err) {
console.log(`Unable to write comment on #${PR} 😞`);
core.setFailed(err.message);
}
}
There was a problem hiding this comment.
This looks pretty simple, but I personally do not think we should merge this as-is, because we already have a throttling plugin for all GitHub requests. Documentation is light but available here and here. This plugin has separate limits for reads, writes, and search, accounts for rate limiting headers in the GitHub API response, and I think will end up being more robust than this.
- I think the first thing I would try would be to update this to retry more than once, let's say 5 times
- Also add
onSecondaryRateLimitthat logs a warning and returns true. This should pause for 60 seconds to address rate limiting. - If we still have issues, we can pass a higher value for
options.retryAfterBaseValueto the plugin. It defaults to 1000ms, we could bump that to 1500 or 2000ms. - We could also pass a higher value for
options.fallbackSecondaryRateRetryAfterto wait for more than 60 seconds when we reach a secondary rate limit
Once you are secondary rate limited the The one second sleep is the advised fix in the GitHub documentation here:
|
|
Not sure if there is a good way tot test this now either @abdulrahuman5196 thanks for testing with the fiddle! @roryabraham thanks for the links! I have assumed what Andrew mentioned, once we hit this rate limit we cant easily just wait to retry later, so I think that 1 second wait is better at this point. Let me know what you think, thanks! |
|
The plugin already does 1, 2, or 3 second sleep between every request, depending on the type of request link |
|
From the octokit docs:
|
|
Seems like comments should be rate limited then: https://github.com/octokit/plugin-throttling.js/blob/main/src/generated/triggers-notification-paths.ts#L9C2-L9C2 I like the idea of just increasing Long term however, we are still using a Personal Access Token based bot, correct? I think we might need to look at moving over to an GitHub App bot e.g. |
Got a source? Regardless, we could still do the other two things I suggested as well to increase the amount of throttling we do in general:
Ultimately the throttling we get from the plugin is more sophisticated than a basic 1-second throttle between requests + accounts for the retry-after headers we get from the API, including extra throttling that can occur during GitHub incidents. Increasing the |
Definitely down to open an issue for this, seems like a good solution to allow us to scale up 👍🏼 |
No, just experience from my testing in the past. We should also rename the function to fix this:
|
mountiny
changed the title
|
@roryabraham @AndrewGable Updated with the suggestions, thanks for patience I agree using github app bot would be best here to also align with our other github flows. |
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
|
Going to merge this since we agreed on the approach in the convo above. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.39-11 🚀
|
Details
In case when there was many PRs in the deploy, we have experience rate limitting as we dont wait between the issue comment requests. This PR is adding 1s sleep between the create comment requests to avoid this happening in future.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/293688
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
N/A
Offline tests
N/A
QA Steps
Ensure the deploy works correctly and all the PRs deployed got a message posted on them.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
No need for screenshots because the changes are only in CI/CD
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android