Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add authorization token to search request to get higher rate limit? #470

Closed
duhd1993 opened this issue Jan 29, 2021 · 8 comments · Fixed by #473
Closed

Add authorization token to search request to get higher rate limit? #470

duhd1993 opened this issue Jan 29, 2021 · 8 comments · Fixed by #473

Comments

@duhd1993
Copy link
Contributor

Is there a rate limit imposed by Github? If so, is there a way to minimize its effect?
I included this in my readthedoc doc site. I find that when I go through doc pages quickly for a while, the comment box stops appearing. After a while, it appears again.

@TysonMN
Copy link

TysonMN commented Jan 29, 2021

I have seen this too. In addition to the comments not being displayed, I recall seeing an error message there.

Did you see an error message? If so, can you get a screenshot of it? I think there is more information when that happens in the developer console of the browser as well.

@duhd1993
Copy link
Contributor Author

duhd1993 commented Jan 29, 2021

I forgot to check the console. yes. there is a error message in the console. Indeed it's hitting rate limit. A 1 min reset period is set by utterances. Not sure internally what's the limit by github though.

{"message":"API rate limit exceeded for xxxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}

https://docs.github.com/en/rest/reference/search#rate-limit

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.

This is a little annoying. 30 requests per minute would be better.

@jdanyow
Copy link
Member

jdanyow commented Jan 29, 2021

Limits are set by GitHub, only option is to sign in and you'll get a much higher rate limit.

@duhd1993
Copy link
Contributor Author

The reason why it's so annoying is that Sign In does not help. utterance is still using search api with no authentication. Thus, 10 refresh/minute limit.

x-ratelimit-limit: 10
x-ratelimit-remaining: 9
x-ratelimit-reset: 1611900523
x-ratelimit-used: 1

This is after sign in. Could/should this be improved?

@duhd1993
Copy link
Contributor Author

duhd1993 commented Jan 29, 2021

Based on

utterances/src/github.ts

Lines 31 to 32 in c1897f2

if (!/^search\//.test(relativeUrl) && token.value !== null) {
request.headers.set('Authorization', `token ${token.value}`);

All request except for the search request is passed with the authorization token. Is there a reason for that?

@duhd1993 duhd1993 changed the title The rate limit from Github? Add authorization token to search request to get higher rate limit? Jan 29, 2021
@duhd1993
Copy link
Contributor Author

duhd1993 commented Feb 2, 2021

I tried hijacking the request and modifying the header to include the token. It worked.

x-ratelimit-limit: 30
x-ratelimit-remaining: 28
x-ratelimit-reset: 1612226658
x-ratelimit-used: 2

@jdanyow Could you explain why you choose not to pass token in search request? If possible, could you modify this? I can submit a PR if you allow.

@jdanyow
Copy link
Member

jdanyow commented Feb 2, 2021

I recall there was a bug or an issue with the documentation where authenticated requests had a higher limit but also a higher reset interval which meant it took much longer to recover after exceeding limit.

I've confirmed the rate limit reset interval is only a minute in both cases. A PR would be welcome, thanks!

@duhd1993
Copy link
Contributor Author

duhd1993 commented Feb 4, 2021

@jdanyow Could you merge the PR and deploy (to cloudflare workers, I guess)? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants