Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories #2635

gal-legit · 2023-01-19T18:47:31Z

Add check-before-do for secondary rate limit
Add optional hook for secondary limits
Add missing categories for primary rate limits

Notes:

As far as I could see, there is no documented API for runner registration - so I didn't handle this case.

Edit:
Following the conversation in this PR, the actual changes are:

Add check-before-do for secondary rate limit
Add missing categories for primary rate limits
Add examples for using go-github-ratelimit

gal-legit · 2023-01-19T20:03:30Z

I have just realized that the hook must get the client as a parameter (and it's reflected in the tests, where I use the wrong client for the retry). I'll push a fix for that.
edit: done
edit 2: I also pushed another commit that makes the hook more flexible.

codecov · 2023-01-20T17:56:35Z

Codecov Report

Merging #2635 (9d0b570) into master (a67a565) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
+ Coverage   98.05%   98.07%   +0.02%     
==========================================
  Files         130      130              
  Lines       11257    11302      +45     
==========================================
+ Hits        11038    11085      +47     
+ Misses        150      148       -2     
  Partials       69       69

Impacted Files	Coverage Δ
github/github.go	`97.92% <100.00%> (+0.46%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gmlewis · 2023-01-20T17:56:50Z

Hmmm... I'm not sure I'm happy about these changes, and I've got a heavy workload right now. Please give me a week to mull this over.

gmlewis

Thank you, @gal-legit .
I think this will be beneficial, but was initially unhappy with the added complexity in BareDo.
Let's move forward with the PR, and I apologize for the delay.

Please see if you can handle the currently-uncovered code path with a unit test if possible, and thank you for the very comprehensive tests you have already added!

github/github.go

gal-legit · 2023-01-26T14:34:46Z

hey @gal-legit, thanks for the feedback!

I feel you - this PR adds complexity and new behavior to the client, and I don't feel too comfortable with adding this "mess" to the very elegant and clean style of the client.

I think that we can actually benefit from the delay.
During this week, I tackled this issue for the project I'm working on.
It started as a meantime solution, but I now think that taking this approach is probably better,
and it allows us to drop the hook and let the users handle it (somewhat like caching/token).

Check out secondaryRateLimitWaiter here.
If you like the idea of dropping the hook and leaving it for the transport layer, I can remove all hook-related stuff from this PR.
Also, if you think that this reference is worth recommending (like the cache/etc.), I can move that secondary rate limit transport to a different repo so that everyone can use it as an external plugin.
Obviously, I'll also adjust it to fit general availability (remove the injecter and the log, add support for max-sleep, etc.).

Let me know what you think, and I'll continue from there.
Thanks!

gmlewis · 2023-01-26T14:46:59Z

Let me know what you think, and I'll continue from there.

Ah, genius! I think handling it in an external-to-this-repo RoundTripper is a thing of beauty!
Let's proceed that way, and then this PR could instead point to the external RoundTripper in its examples and/or comments.

Thank you, @gal-legit !

gal-legit · 2023-01-26T14:53:34Z

@gmlewis cool! I'll do the separation and update the PR with all the fixes and reference when it's ready.
I should keep the basic handling of the secondary rate limit in the client though. right?
i.e., saving it in the response and handling it in checkSecondaryRateLimitBeforeDo() to prevent remote requests, but with an implementation that looks just like checkRateLimitBeforeDo() so it isn't messy.

gmlewis · 2023-01-26T15:08:05Z

Hmmm... would that tie this "go-github" repo to the external RoundTripper repo?
Is there a way to add the functionality without adding an external dependency?

gal-legit · 2023-01-26T15:17:12Z

I meant in the context of the PR -- adding the handling to improve the client, regardless of the RoundTripper.
The current implementation of the RoundTripper uses go-github because the hosting project already does (so I didn't mind). The standalone version would parse the header and remove the dependency on go-github. So it shouldn't be tied anyway.

gmlewis · 2023-01-26T15:19:54Z

OK, cool... I think I understand, but it will become more clear as I see the PR update.
Thanks again, @gal-legit ! I appreciate your patience and willingness to make this improvement!

gal-legit · 2023-01-27T14:14:51Z

I have the new repo ready (https://github.com/gofri/go-github-ratelimit),
gonna have the changes to the PR ready soon

gmlewis · 2023-01-27T14:22:11Z

I have the new repo ready (https://github.com/gofri/go-github-ratelimit), gonna have the changes to the PR ready soon

Woohoo! I gave it its very first GitHub star! 😄

gal-legit · 2023-01-28T10:46:52Z

@gmlewis
Thanks for the star! :)

I pushed the new version.
I resolved the comments that are related to the hooks because it's now omitted. Also, I responded to (and applied) the fixes from the other comments.

gmlewis

Thank you, @gal-legit !

example/ratelimit/main.go

github/github.go

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

gal-legit · 2023-01-28T14:06:06Z

@gmlewis oops, done

gmlewis · 2023-01-28T14:09:35Z

We keep getting linter errors.

level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: inspect: failed to load package : could not load export data: no export data for \"github.com/gofri/go-github-ratelimit/github_ratelimit\"\n\n"

It looks like you need to make a versioned release (0.0.1 maybe?) of your new external package, then update this PR to explicitly pull in that version in the examples/go.mod file to clear up this problem.

gmlewis · 2023-01-28T14:40:26Z

It looks like there is a problem on Windows... I think it may have to do with how time comparisons are performed in your test.

    github_test.go:1461: abuseRateLimitErr RetryAfter = 2m3s, want 2m3s
--- FAIL: TestDo_rateLimit_abuseRateLimitError_retryAfter (0.00s)

Please check out time.Equal and see if you can fix this.

gal-legit · 2023-01-28T15:09:33Z

@gmlewis
My bad. It was actually a bug in the test:
I meant to test the delta between the expected and actual retry-after is less than a second,
but, I forgot to multiply by time.Second and the comparison was the other way around.
So it actually tested that the delta is bigger than a nanosecond. I guess that the timer has a lower resolution on Win so it caught it.
p.s. it's comparing time.Duration, so AFAIK it should be fine as long as I compare it to time.Second.

gmlewis · 2023-01-28T15:15:12Z

My bad. It was actually a bug in the test:

I find it bizarre that only the Windows version of the test caught the issue, and not the Linux build!?!?!?

OK, thanks. I have to step out but hopefully will get back to this PR later tonight.

gofri · 2023-01-28T15:25:43Z

LOL, I found it a bit embarrassing for me, being a Linux kernel engineer.
I guess that sometimes having a worse implementation might be good for testing 🤷

gmlewis · 2023-01-28T15:38:48Z

being a Linux kernel engineer.

Cool! I'd love to ask you about your favorite distros, but this is probably not the right forum. 😂

gmlewis · 2023-01-28T21:17:13Z

github/github_test.go

@@ -1136,6 +1141,36 @@ func TestDo_rateLimit(t *testing.T) {
 	}
 }

+func TestDo_rateLimitCategory(t *testing.T) {
+	if c := category(http.MethodGet, "/"); c != coreCategory {


Is there any reason that this is not a table-driven test?

I'm OK leaving it as-is if you want... since it would probably be the same number of lines, but I often find that table-driven tests are easier to read because you can just look at the data all in one place.

But no biggie... I'm fine either way.

tbh I just tried to follow TestRateLimits & TestRateLimits_overQuota,
but I fixed it to be table-driven (and fixed those two while at it)

gmlewis

Thank you, @gal-legit !
One minor tweak please, plus one question, otherwise LGTM.

After the tweak, we should be ready for an LGTM+Approval from any other contributor to the repo before merging.

gmlewis · 2023-01-28T21:18:46Z

github/github_test.go

+	_, err = client.Do(ctx, req, nil)
+
+	if err == nil {


Suggested change

_, err = client.Do(ctx, req, nil)

if err == nil {

if _, err := client.Do(ctx, req, nil); err == nil {

I thought it'd be a bit misleading to put it inside the if statement because we use err later (casting to *AbuseRateLimitError), but I fixed it anyway

gmlewis

Thank you, @gal-legit !

gmlewis · 2023-01-29T23:32:28Z

github/github_test.go

+		category rateLimitCategory
+	}{
+		{
+			http.MethodGet,


This is a tiny nit, and if you don't want to do this, I'm fine...
but in this repo we prefer to label the fields for clarity-at-a-quick-glance (e.g. method: http.MethodGet,, etc.).
I'm going to go ahead and approve, but if you want to update, great.

sure, np, I'll do that.
p.s. Ubuntu for desktop, RHEL/fedora-based for server 🙃

gmlewis

Thank you, @gal-legit !
LGTM.

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

valbeat

LGTM!

gmlewis · 2023-01-30T13:30:46Z

Thank you, @valbeat !
Merging.

gal-legit mentioned this pull request Jan 19, 2023

Rate limit does not cover all categories & secondary rate limit not preventing requests #2633

Closed

gmlewis requested changes Jan 26, 2023

View reviewed changes

github/github.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

gal-legit force-pushed the master branch from a3e7cd2 to 0dd861e Compare January 28, 2023 10:35

gal-legit closed this Jan 28, 2023

gal-legit force-pushed the master branch from 0dd861e to a67a565 Compare January 28, 2023 10:37

gal-legit added 3 commits January 28, 2023 12:38

Add issuing requests during secondary rate limits prevention and hook

811ef6f

Fix rate limit category detection

e0e8f4e

Add secondary rate limit example and guidance

2b1619c

gal-legit reopened this Jan 28, 2023

gal-legit changed the title ~~Add secondary rate limit handling (pre-do and hook) and fix rate limit categories~~ Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories Jan 28, 2023

gmlewis requested changes Jan 28, 2023

View reviewed changes

example/ratelimit/main.go Outdated Show resolved Hide resolved

github/github.go Outdated Show resolved Hide resolved

gal-legit and others added 2 commits January 28, 2023 16:04

Update example/ratelimit/main.go

4e531d6

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

pr fix

2164c94

fix go mod

f947fc7

bugfix test

3479534

gmlewis reviewed Jan 28, 2023

View reviewed changes

gmlewis requested changes Jan 28, 2023

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 28, 2023

tests improvement (pr fix)

151b046

gmlewis approved these changes Jan 29, 2023

View reviewed changes

named fields

9d0b570

gmlewis approved these changes Jan 30, 2023

View reviewed changes

valbeat approved these changes Jan 30, 2023

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jan 30, 2023

gmlewis merged commit da244a4 into google:master Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories #2635

Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories #2635

gal-legit commented Jan 19, 2023 •

edited

Loading

gal-legit commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 20, 2023 •

edited

Loading

gmlewis commented Jan 20, 2023

gmlewis left a comment

gal-legit commented Jan 26, 2023 •

edited

Loading

gmlewis commented Jan 26, 2023

gal-legit commented Jan 26, 2023

gmlewis commented Jan 26, 2023

gal-legit commented Jan 26, 2023

gmlewis commented Jan 26, 2023

gal-legit commented Jan 27, 2023

gmlewis commented Jan 27, 2023

gal-legit commented Jan 28, 2023 •

edited

Loading

gmlewis left a comment

gal-legit commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gal-legit commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gofri commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gmlewis Jan 28, 2023 •

edited

Loading

gal-legit Jan 29, 2023

gmlewis left a comment

gmlewis Jan 28, 2023

gal-legit Jan 29, 2023

gmlewis left a comment

gmlewis Jan 29, 2023

gal-legit Jan 30, 2023

gmlewis left a comment

valbeat left a comment

gmlewis commented Jan 30, 2023

Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories #2635

Add secondary rate limit handling (prevent remote requests) and fix primary rate limit categories #2635

Conversation

gal-legit commented Jan 19, 2023 • edited Loading

gal-legit commented Jan 19, 2023 • edited Loading

codecov bot commented Jan 20, 2023 • edited Loading

Codecov Report

gmlewis commented Jan 20, 2023

gmlewis left a comment

Choose a reason for hiding this comment

gal-legit commented Jan 26, 2023 • edited Loading

gmlewis commented Jan 26, 2023

gal-legit commented Jan 26, 2023

gmlewis commented Jan 26, 2023

gal-legit commented Jan 26, 2023

gmlewis commented Jan 26, 2023

gal-legit commented Jan 27, 2023

gmlewis commented Jan 27, 2023

gal-legit commented Jan 28, 2023 • edited Loading

gmlewis left a comment

Choose a reason for hiding this comment

gal-legit commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gal-legit commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gofri commented Jan 28, 2023

gmlewis commented Jan 28, 2023

gmlewis Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

gal-legit Jan 29, 2023

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis Jan 28, 2023

Choose a reason for hiding this comment

gal-legit Jan 29, 2023

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis Jan 29, 2023

Choose a reason for hiding this comment

gal-legit Jan 30, 2023

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

valbeat left a comment

Choose a reason for hiding this comment

gmlewis commented Jan 30, 2023

gal-legit commented Jan 19, 2023 •

edited

Loading

gal-legit commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 20, 2023 •

edited

Loading

gal-legit commented Jan 26, 2023 •

edited

Loading

gal-legit commented Jan 28, 2023 •

edited

Loading

gmlewis Jan 28, 2023 •

edited

Loading