Add opt-in rate limit support on endpoints returning 302s #3411
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Refactor `Client` to add a new `http.Client` dedicated to calls that expect a 302 success response. Reuse the existing `BareDo` rate limit logic with that new client. Add a feature flag to opt in to using this instead of the usual `roundTripWithOptionalFollowRedirect` logic.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3411 +/- ##
==========================================
- Coverage 97.72% 92.22% -5.51%
==========================================
Files 153 173 +20
Lines 13390 14937 +1547
==========================================
+ Hits 13085 13775 +690
- Misses 215 1068 +853
- Partials 90 94 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you, @wmouchere - this is looking great!
I'm requesting one minor change across all your new methods, please.
Also, if you could increase the code coverage for the new methods (see Codecov feedback in the GitHub UI), that would be great, but not absolutely necessary, as the uncovered cases are indeed going to be rare error paths.
Additionally, I'm noticing that you use %d and %s in format strings, and it is very common in idiomatic Go to simply use %v everywhere unless there is a strong need to do otherwise (which then catches the eye as an indication that something unusual is going on here). It would be nice (but not required) if you switch to %v if you don't mind.
Fix the ...WithRateLimit funcs so that the happy path flows to the end of the method. Add unit tests for bareDoUntilFound and ...WithRateLimit funcs to validate behavior when receiving unexpected http status codes.
There was a problem hiding this comment.
This looks fantastic, @wmouchere !
Beautifully well-done - thank you!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
|
Thanks a lot for the quick review @gmlewis!
|
|
Thank you, @Letiste ! |
Fixes #3406
Some endpoints of the GitHub HTTP API return a 302 response in successful scenarios, often to redirect to archives or artifacts download links. Because the
net/httpclient usually follows 302s when callingDo, we instead currently use theRoundTripperdirectly for those endpoint to check after each call if the current redirection is a 302. The problem is that the library supports rate limit headers only in theDotype of calls.This PR adds an opt-in feature flag to enable rate limit headers support on those endpoints which end with a 302 redirection. I refactored
Clienta bit to share the coreBareDologic between calls that care about redirection details and calls that don't. I added an unexportedbareDoUntilFoundmethod that behaves similarly to the oldroundTripWithOptionalFollowRedirectbut with all the rate limit and error handling that other endpoints have.