Skip to content

Add opt-in rate limit support on endpoints returning 302s - #3411

Merged
gmlewis merged 2 commits into
google:masterfrom
wmouchere:master
Jan 2, 2025
Merged

Add opt-in rate limit support on endpoints returning 302s#3411
gmlewis merged 2 commits into
google:masterfrom
wmouchere:master

Conversation

@wmouchere

@wmoucherewmouchere commented Dec 31, 2024

Copy link
Copy Markdown
Contributor

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/http client usually follows 302s when calling Do, we instead currently use the RoundTripper directly 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 the Do type 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 Client a bit to share the core BareDo logic between calls that care about redirection details and calls that don't. I added an unexported bareDoUntilFound method that behaves similarly to the old roundTripWithOptionalFollowRedirect but with all the rate limit and error handling that other endpoints have.

@google-cla

Copy link
Copy Markdown

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

codecovBot commented Dec 31, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 93.29609% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.22%. Comparing base (2b8c7fa) to head (5182066).
Report is 212 commits behind head on master.

Files with missing linesPatch %Lines
github/github.go87.14%6 Missing and 3 partials ⚠️
github/repos_contents.go86.36%2 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment threadgithub/actions_artifacts.go Outdated
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.
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Dec 31, 2024

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks fantastic, @wmouchere !
Beautifully well-done - thank you!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@wmouchere

Copy link
Copy Markdown
ContributorAuthor

Thanks a lot for the quick review @gmlewis!

  • Changes you mentioned on the happy path flow are done 👍
  • I should have updated all the %s I added to %v, but left the %d where its used to format a http status. I noticed that this was the pattern used in most places in the library.
  • I did my best to increase coverage by testing handling of unexpected status codes. I would have wanted to test invalid Location headers but net/http actually validates them upstream which changes the flow a bit and makes it hard to test.

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Jan 2, 2025
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @Letiste !
Merging.

Sign up for freeto 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.

Calls based on roundTripWithOptionalFollowRedirect do not respect rate limits

3 participants

@wmouchere@gmlewis@Letiste