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(runners): Add delay to prevent ssm rate limits using setTimeout #2823

Merged
merged 7 commits into from
Jan 5, 2023

Conversation

devsh4
Copy link
Contributor

@devsh4 devsh4 commented Dec 29, 2022

Adding a fix for bug #1841 which prevents hitting AWS SSM rate limits by adding a delay between subsequent putParameter calls.

Note: 25ms delay is based on the AWS service quotas as they only allow 40 requests per second to the parameter store by default.

[Tests]:

  • Pre commit checks ✅
  • Ran build locally ✅
  • Ran linter ✅
  • Ran jest tests ✅

Related PRs/discussion for reference:

  1. (never got merged): Add delay to prevent ssm rate limits using setTimeout #1843
  2. (went stale due to inactivity & failing tests): fix: Add delay to prevent ssm rate limits if instance count is greater than max throughput #2305

@devsh4 devsh4 changed the title ISSUE-1841: Add delay to prevent ssm rate limits using setTimeout fix: [ISSUE-1841] Add delay to prevent ssm rate limits using setTimeout Dec 29, 2022
@devsh4
Copy link
Contributor Author

devsh4 commented Dec 29, 2022

@npalm @GuptaNavdeep1983 Please review this PR so we can merge this fix and unblock everyone out there from upgrading the module 🙏 This is my third attempt at merging this.

In the earlier two attempts, one PR went stale & the test coverage went below threshold. Therefore this PR also adds the required tests.

@GuptaNavdeep1983
Copy link
Contributor

GuptaNavdeep1983 commented Dec 30, 2022

@npalm @GuptaNavdeep1983 Please review this PR so we can merge this fix and unblock everyone out there from upgrading the module 🙏 This is my third attempt at merging this.

In the earlier two attempts, one PR went stale & the test coverage went below threshold. Therefore this PR also adds the required tests.

@devsh4, Thanks for the PR. The PR looks good, Can we also add a condition that checks for the time taken by the test for creation of 40 instances and ensure that the time taken is more than 1000 ms which I did see that is more than 1000 ms? I don't see any other way to test the delay. Also, you need to run yarn format on the module and check it in as the pipeline is failing currently.

@npalm npalm self-requested a review December 30, 2022 08:14
@GuptaNavdeep1983 GuptaNavdeep1983 self-requested a review December 30, 2022 11:00
@devsh4
Copy link
Contributor Author

devsh4 commented Dec 30, 2022

@GuptaNavdeep1983 Thanks for the quick review, fixed the formatting and updated the test to check the delay. Can you please review again?

Additional context for posterity: We have been running the module with this delay since 9 months now to run a pool of 200+ runners daily for our org in production and it is working seamlessly, without any unexpected lambda costs.

@GuptaNavdeep1983
Copy link
Contributor

GuptaNavdeep1983 commented Jan 2, 2023

@GuptaNavdeep1983 Thanks for the quick review, fixed the formatting and updated the test to check the delay. Can you please review again?

Additional context for posterity: We have been running the module with this delay since 9 months now to run a pool of 200+ runners daily for our org in production and it is working seamlessly, without any unexpected lambda costs.

@devsh4, Can you please change the require statement into import { performance } from 'perf_hooks';

@GuptaNavdeep1983 GuptaNavdeep1983 self-requested a review January 2, 2023 06:46
@devsh4
Copy link
Contributor Author

devsh4 commented Jan 2, 2023

@GuptaNavdeep1983 Updated, linter is fixed now.

@devsh4
Copy link
Contributor Author

devsh4 commented Jan 3, 2023

@GuptaNavdeep1983 @npalm can one you'll please approve the workflow? Fixed the linter/formatter and updated the branch to be up-to-date with main. Thanks!

@devsh4
Copy link
Contributor Author

devsh4 commented Jan 4, 2023

@GuptaNavdeep1983 Thanks a lot for reviewing!

@npalm I'm assuming it is you who will be merging this PR? If yes, thanks in advance 😄

@npalm npalm changed the title fix: [ISSUE-1841] Add delay to prevent ssm rate limits using setTimeout feat(runners): Add delay to prevent ssm rate limits using setTimeout Jan 4, 2023
@npalm
Copy link
Member

npalm commented Jan 4, 2023

close #1841

@npalm
Copy link
Member

npalm commented Jan 4, 2023

changed semantic commit type to feat instead of fix

@devsh4
Copy link
Contributor Author

devsh4 commented Jan 5, 2023

@npalm Thanks for reviewing! When can I expect the PR to be merged? Just curious regarding the process.

@npalm npalm merged commit 1461efd into philips-labs:main Jan 5, 2023
@devsh4 devsh4 deleted the fix/issue-1841 branch January 5, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants