good-first-issue icon indicating copy to clipboard operation
good-first-issue copied to clipboard

Handle rate limiting gracefully

Open bnb opened this issue 5 years ago • 12 comments

When testing to make sure nothing breaks, I semi-often get rate-limited by GitHub. This is because there are (IIRC) 50 queries accepted before 1-hour rate limiting kicks in.

Obviously this shouldn't be happening for a normal user (though I'd applaud a user that was working on dozens of good-first-issues), but it may happen for contributors.

Checklist:

  • [ ] Handle errors in the CLI gracefully
  • [ ] Allow consumers of the module to pass GitHub authentication, which we would then pass to @octokit/rest
  • [ ] Documentation for both of the above

bnb avatar Nov 17 '18 21:11 bnb

Found this in the docs about rate limits:

The Search API has a custom rate limit. For requests using Basic Authentication, OAuth, or client ID and secret, you can make up to 30 requests per minute. For unauthenticated requests, the rate limit allows you to make up to 10 requests per minute.

https://developer.github.com/v3/search/#rate-limit

ghost avatar Nov 18 '18 15:11 ghost

Also, not sure how much users will give GitHub credentials to avoid these limits. So not sure if it is worth adding an authentication functionality just for this? Otherwise I would like to give it a shot, found this article about a CLI application getting and storing a OAuth token etc.

ghost avatar Nov 18 '18 15:11 ghost

I believe it's 5k/hour or so – I have experience with this from building out NodeKitten.

bnb avatar Nov 20 '18 22:11 bnb

For implementation of this, I believe we can just accept an object and pass it directly to @octokit/rest.js, which is the lib we're using for GitHub. See the Authentication examples in their README.md for an example – in NodeKitten I used token authentication but we could just let the user decide how they want to authenticate by passing an object as @octokit/rest.js would expect.

bnb avatar Nov 20 '18 22:11 bnb

Oops didn't mean to close 😁

bnb avatar Nov 20 '18 22:11 bnb

I believe it's 5k/hour or so

if the rate is 5K :shamrock: I don't think we/any user at any point exceed this rate (except in case of bruting :wink: )

maddhruv avatar Nov 21 '18 02:11 maddhruv

I think the search that we use is limited at a lower rate than normal requests. Have tried it out, and can hit the limit if i make a dozen requests in a minute.

ghost avatar Nov 21 '18 07:11 ghost

For implementation of this, I believe we can just accept an object and pass it directly to @octokit/rest.js

If we don't store the authentication in a way, the user will have to login/authenticate each time we run? Don't think that is a fun UX if we ask that every time.

ghost avatar Nov 21 '18 07:11 ghost

@kennethvandenberghe so this is more for when the user require()s the module to use the output programmatically and less when the user is using it from the CLI.

IMO it's less needed on the CLI since I can't really think of a use case in which a user (not a contributor) is genuinely using the CLI to find a Good First Issue.

That said, we may want to add the ability for contributors to add a file locally like github-auth.json that includes personal auth tokens so we have less of an issue when working on contributing? Perhaps that could be split out into a different issue.

bnb avatar Nov 21 '18 14:11 bnb

  • As for a module consumption scenario - we can add auth token as an option to it
  • and for the cli, I don't think a user might end up making hundreds to request/hour, hence no need in the CLI

maddhruv avatar Nov 22 '18 01:11 maddhruv

So to conclude stuff here so i can start fixing this stuff finally, what is the goal specifically @bnb?

ghost avatar Nov 23 '18 19:11 ghost

@kennethvandenberghe enable the user to pass an object to the module when calling it that we will pass directly to @octokit/rest.js.

We don't need to be specific about what that object looks like, since @octokit/rest.js already has mechanisms for that – we just need to pass that object to @octokit/rest.js if it is provided.

Here's the examples from the @octokit/rest.js README: https://github.com/octokit/rest.js#authentication

bnb avatar Nov 24 '18 19:11 bnb