-
Notifications
You must be signed in to change notification settings - Fork 36
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: respect X-Ratelimit-Reset when retrying API requests #434
Conversation
Just a friendly ping @jazanne since you approved my coworker's PR a few weeks ago. Also, 👋 from Fishtown since I see you're also Philly-based. |
hey @jonathan-ostrander thanks for this change! I'm working on getting it through the reviewing process. just out of curiosity, how are you all using code refs? via GitHub action, or the CLI directly? |
We use the GitHub Action. I was actually going to follow-up with a feature request issue and another PR for adding an option to scan all projects in an organization instead of a manually configured list. Right now we hack around this problem by having a step hit LD's API to list all of our projects and then we run the find code refs action for each of those. Currently it works alright, but ends up failing ~10% of the time because of rate limit issues. |
@jonathan-ostrander that's really useful context. Just to clarify, monorepo configuration is too inflexible for you all, and would require a lot of manual updates and changes. Does that mean that your actions are configured to scan all directories for flags in all projects, instead of a subset of directories for some projects/flags? |
@jazanne Yes, we don't create LD projects that often but when we do it would be nice to avoid the manual step of adding another project to a list in a GHA. That's a less important follow-up though. This change is a lot more helpful for us because currently ~10% of our actions are failing on the 429s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together! After reviewing with the team, it's important that we keep the default retry strategy LinearJitter
in place to handle cases where rate limit headers may not be available
launchdarkly#434 attempted to handle the rate limit from the LD API but the LD API client uses `http.DefaultClient` by default. This change passes in the client with `RateLimitBackoff` set into the call to init the LD API client.
#434 attempted to handle the rate limit from the LD API but the LD API client uses `http.DefaultClient` by default. This change passes in the client with `RateLimitBackoff` set into the call to init the LD API client.
Fixes #433. Fallback to default behavior (the
RetryWaitMin
andRetryWaitMax
lines were removed because the hardcoded values were already the default ingo-retryablehttp
) in the case whereX-Ratelimit-Reset
is not available for some reason.