Open MichaelMure opened 4 years ago
As far as I can tell, this package doesn't have any specific code path to handle the Github rate limiting.
That is true. However, note that you can always query the current rate limit by adding a rateLimit
field to your query. See https://developer.github.com/v4/query/#ratelimit.
It'd be nice to at least return a specific error on HTTP 429 so that retry/back-of could be implemented outside of this package.
Yes, this should be done. It's being tracked by issue #38.
Ideally this would be implemented within
githubv4
.
Maybe, in the long term. First step is to implement some solution. Then see if it can be generalized well enough. If so, we can consider factoring it into githubv4
.
GitHub API v3 package automatically detects when the returned GitHub error was due to rate limit being depleted, and computes how long until the rate limit will be reset. It's also smart enough not to talk to GitHub as long as the rate limit reset time hasn't passed yet. Hopefully it'll be possible to do similar things here with v4.
@dmitshur I was interested as well and I have opened a post on the github community: https://github.community/t5/GitHub-API-Development-and/Documentation-about-rate-limit-error-in-V4/m-p/52770#M4508
It is not clear from the documentation what the API will return when the error will occur.
FWIW, this is how it's done in git-bug: https://github.com/MichaelMure/git-bug/blob/a52c474f1184b59f109934be00f10c8088b890cd/bridge/github/client.go#L133-L134
I see docs for this in https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit
I'm currently getting a secondary rate limit with a Retry-After
header in my query (as well as sending me 502s). I've got a basic roundtripper to deal with it, but I'd appreciate more support from the library (in particular, I'd like to do easier OTEL Tracing integration).
Would you be open to a PR that returns a more strongly typed error from client.do
?
Right now the code is:
if resp.StatusCode != http.StatusOK {
body, _ := io.ReadAll(resp.Body)
return fmt.Errorf("non-200 OK status code: %v body: %q", resp.Status, body)
}
I'd like to change that to:
if resp.StatusCode != http.StatusOK {
body, _ := io.ReadAll(resp.Body)
// this struct would implement Error() like "non-200 OK status code: %v body: %q", e.Resp.Status, body"
// but clients would also be able to switch on the error type and inspect the response themselves
return NonHTTPStatusOKError{Resp: resp}
}
This would let users implement rate-limiting without using http.RoundTripper
For reference, here's my RoundTripper code (NOT production quality, it doesn't implment good chunks of what they want):
As far as I can tell, this package doesn't have any specific code path to handle the Github rate limiting.
It'd be nice to at least return a specific error on HTTP 429 so that retry/back-of could be implemented outside of this package. Ideally this would be implemented within
githubv4
.