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

[gal-legit]

Hey,
First, thanks for all the great work on this client!

I noticed a couple of issues with the rate limits (please let me know if you think I should split it into two issues):

1. PR Add fields to RateLimits struct #2340 added support for all rate limit categories.
   as @gmlewis pointed out in this discussion, all categories should be handled in the catgory() function.
   The current code is:

func category(path string) rateLimitCategory {
	switch {
	default:
		return coreCategory
	case strings.HasPrefix(path, "/search/"):
		return searchCategory
	}
}

which is used in:
rateLimitCategory := category(req.URL.Path)
this variable is used in 2 points:
c.checkRateLimitBeforeDo(req, rateLimitCategory)
and
c.rateLimits[rateLimitCategory] = response.Rate

Now, since all categories (but search) are aggregated into the Core category on every request,
the result is that checkRateLimitBeforeDo might prevent the user from issuing a request, although the rate limit wasn't reached.
I can try to add partial support for that, at least for the fields that are in the documentation or are easy to deduce (e.g. code_scan_upload / integration_manifest)

2. The secondary rate limit is handled in CheckResponse(), but not in checkRateLimitBeforeDo().
   As a result, there is no built-in protection against the secondary rate limit, which would be valuable.
   I can try to add support for that as well, but I'm not sure to which request the secondary rate limit applies:

• The very same request
• All requests in the same category.
• Any request from that user.

I hope someone here might know the answer.

On the same note, I'd be glad to hear your thoughts on implementing an optional 'wait for secondary rate limit' configuration in the client. IMO the use case of waiting for the secondary rate limit is much more common than waiting for the hour-long rate limit (which also isn't supported).

[gmlewis]

If I'm reading https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#secondary-rate-limits correctly, then the secondary rate limit applies to any following request from that user, and is intended to prevent abuse of their servers (by not following their documented guidelines).

Thank you, @gal-legit ! Assigning this issue to you.

[gal-legit]

@gmlewis thanks!
I submitted a PR for the check-secondary-before-do part @ #2634.

I'd appreciate your input on my suggestion for an optional user handling for that before I go into writing the code.
I'll try to elaborate on my suggested design:

1. Add the functional options pattern to NewClient().
2. Through it, introduce an option: secondaryRateLimitCallback(secondaryResetTime time.Time, req *http.Request) *AbuseRateLimitError.
3. Provide the implementation from the current PR as the default functionality.

This way, clients of the library can opt-in to implement their own handler (e.g. sleep with print) whilst the non-blocking behavior remains the default - and the library doesn't introduce long-blocking actions.
I'd understand the preference to implement it through the context (like bypassRateLimitCheck), but that feels a bit unnatural to me - so please let me know what you think.

also, regarding the categories() note - do you think that I should open it in a separate issue?

[gmlewis]

also, regarding the categories() note - do you think that I should open it in a separate issue?

I'm fine either way.

[gmlewis]

@gmlewis thanks! I submitted a PR for the check-secondary-before-do part @ #2634.

I'd appreciate your input on my suggestion for an optional user handling for that before I go into writing the code. I'll try to elaborate on my suggested design:

1. Add the functional options pattern to NewClient().
2. Through it, introduce an option: secondaryRateLimitCallback(secondaryResetTime time.Time, req *http.Request) *AbuseRateLimitError.
3. Provide the implementation from the current PR as the default functionality.

This way, clients of the library can opt-in to implement their own handler (e.g. sleep with print) whilst the non-blocking behavior remains the default - and the library doesn't introduce long-blocking actions. I'd understand the preference to implement it through the context (like bypassRateLimitCheck), but that feels a bit unnatural to me - so please let me know what you think.

I've looked at #2634 and at your above proposal, and both sound good to me.
My only comment at this point is to also please be sure to write unit tests to cover the new code paths.

Thank you, @gal-legit !

[gal-legit]

@gmlewis cool, thanks!
Unit tests sound fair enough. I'll have to check out the tests code to see how to do that properly, so meanwhile, I pushed the opts-based implementation to the PR and I'll just add tests for both.

[gal-legit]

@gmlewis I submitted #2635 to fix all that we discussed here (and unit tests).
Let me know if you're familiar with an API for runner registration

[gmlewis]

Let me know if you're familiar with an API for runner registration

I'm not sure what you mean by "runner registration" or how it relates to this issue, sorry.