Handle Rate Limits in Octocrab

[programLyrique]

I sometimes get hit by secondary rate limits when searching code and I would like to know how long exactly I should wait before retrying (I am doing exponential backoff currently...).
According to the github documentation, I should try after retry-after seconds if this is the response header, or the x-ratelimit-reset header.

Is there a way to retrieve that in octocrab?
I would have expected to find it in the errors field of GithubError but errors is always None in the case of secondary rate limits for me.

[XAMPPRocky]

Thank you for your issue!

Personally I don't think this should be something that users have to worry about for the most part. It's an implementation detail of the client. Our current handling of errors should also do this for you.

[jrozner]

I've also been running into this not just with Octocrab but also with the official Ruby library. I think there is something specific with the search API that is different.

[programLyrique]

There is actually specific rate limits for search: https://docs.github.com/en/rest/search?apiVersion=2022-11-28#about-search

[XAMPPRocky]

So just to clarify for anyone interested. I would accept a PR that updates the get / post / delete / put methods in the octocrab client with a retry task that on 403 errors, checks the x-ratelimit headers, and sets up a delay to retry based on those headers. https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limiting

@programLyrique I should have also mentioned there is a ratelimit method on Octocrab to get your rate limits manually. This endpoint does not impose rate limits, so you can always check it.

[programLyrique]

It seems that ratelimit endpoint just shows the primary rate limits, not the secondary ones (and I don't think Github provides any more information than the x-ratelimit about the secondary ones).

Here is a request I do immediatly after getting hit by a secondary rate limit error (and just before another one, as my exponential backoff has not reached a long enough duration):

RateLimit { resources: Resources { core: Rate { limit: 5000, used: 0, remaining: 5000, reset: 1682344037 }, 
search: Rate { limit: 30, used: 0, remaining: 30, reset: 1682340497 }, 
graphql: Some(Rate { limit: 5000, used: 0, remaining: 5000, reset: 1682344037 }), integration_manifest: Some(Rate { limit: 5000, used: 0, remaining: 5000, reset: 1682344037 }), 
scim: Some(Rate { limit: 15000, used: 0, remaining: 15000, reset: 1682344037 }), 
source_import: Some(Rate { limit: 100, used: 0, remaining: 100, reset: 1682340497 }), 
code_scanning_upload: Some(Rate { limit: 1000, used: 0, remaining: 1000, reset: 1682344037 }), actions_runner_registration: Some(Rate { limit: 10000, used: 0, remaining: 10000, reset: 1682344037 }) }, 
rate: Rate { limit: 5000, used: 0, remaining: 5000, reset: 1682344037 } }

As you can see, for search, it actually shows 0 request used, and 30 remaining (I would have expected to see at least 1 request used as I had a successful request).

I will have a look at how to add retrying based on x-ratelimit and try to make a PR...

[XAMPPRocky]

@programLyrique I actually forgot that someone did implement this already but I haven't accept the PR 😅

#310

[jrozner]

I have some code that I wrote that used this API and simply added increasing wait times up until 1 minute in between page requests and was never able to get it to succeed after the first page. It always hit the secondary rate limit. Have you been able to get it to succeed? I can't imagine needing to wait longer than 1 minute.

[programLyrique]

@XAMPPRocky

I actually forgot that someone did implement this already but I haven't accept the PR sweat_smile

#310

Yes, I actually saw it and had found a feature flag now with some retry capability that can be specified with add_retry_config but had not seen the new comments on that PR.

I think the best would be to use the code that extracts the headers of that PR, and add them in the retry of that file.
I will wait to see if @goodspark does it!

[programLyrique]

@jrozner

I have some code that I wrote that used this API and simply added increasing wait times up until 1 minute in between page requests and was never able to get it to succeed after the first page. It always hit the secondary rate limit. Have you been able to get it to succeed? I can't imagine needing to wait longer than 1 minute.

The ratelimit API does not give me usable results as even when the error is that I used all the resources, I always have the maximum number of requests remaining. So I just implemented an exponential backoff with an arbitrary base duration (30s) I think, and usually, after 2 or 3 retries, it worked. I also always waited 1s between every request. The rate limit is over 1h so you might have to wait for 1h if you do too many requests in a short burst.

[goodspark]

Search is a bit different. It resets a lot faster (depending on if your request is authenticated). https://docs.github.com/en/rest/search?apiVersion=2022-11-28#rate-limit So it makes sense it would work after 2-3 retries if you start at 30s (bc generally you'll be over 1min by then).

[Swiddis]

After running into this I was looking over the best practices for GitHub's REST API and saw that, among other things, they say to avoid making concurrent requests. I suspect, then, that the robust solution here would be to add an (as a builder method?) internal mpsc::Channel and use the rate limit headers to determine when to next pull from the queue.

The main issue with implementing that as a caller directly is that we don't get access to these headers from the outside for most methods. Though you can emulate it by calling to rate_limit and setting up a Ratelimiter based on the results