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

Throttling seems to be disabled despite the claims in readme #2168

Closed
2 of 7 tasks
postatum opened this issue Nov 15, 2021 · 6 comments · Fixed by #2224
Closed
2 of 7 tasks

Throttling seems to be disabled despite the claims in readme #2168

postatum opened this issue Nov 15, 2021 · 6 comments · Fixed by #2224
Labels
Type: Bug Something isn't working as documented

Comments

@postatum
Copy link

postatum commented Nov 15, 2021

Please avoid duplicates

Reproducible test case

const { Octokit } = require('octokit')
const { throttling } = require("@octokit/plugin-throttling")
Octokit.plugins.includes(throttling)

Please select the environment(s) that are relevant to your bug report

  • TypeScript
  • Enterprise
  • Browsers
  • Node
  • Deno

Version

[email protected]

What happened?

Hi.
Note: I'm not sure if this is a documentation issue or really a bug.

The README mentions throttling in these pieces:

Octokit implements request throttling using @octokit/plugin-throttling
By default, requests are retried once and warnings are logged in case hitting a rate or abuse limit

In both cases a given request is authenticated, retried, and throttled transparently by the octokit instance which also manages the accept and user-agent headers as needed.

Overall these seem to say "throttling is enabled by default". Though, the first piece is less clear about that than the second one.

Despite the above, throttling seems to be disabled in code: https://github.com/octokit/octokit.js/blob/v1.7.0/src/octokit.ts#L13

PS. I started researching this after I hit rate limit and throttling didn't seem to work.

Would you be interested in contributing a fix?

  • yes
@postatum postatum added the Type: Bug Something isn't working as documented label Nov 15, 2021
@Chocrates
Copy link

Hey @postatum I think this would be a documentation bug, throttling is handled in https://github.com/octokit/plugin-throttling.js

Sample of setting it up

const { throttling } = require("@octokit/plugin-throttling");
  const { retry } = require("@octokit/plugin-retry");
  const { Octokit } = require("@octokit/rest");
  const MyOctokit = Octokit.plugin(throttling).plugin(retry);

const client = new MyOctokit({
          auth: "token " + token,
          throttle: {
              onRateLimit: (retryAfter, options) => {
                  console.warn(
                      `Request quota exhausted for request ${options.method} ${options.url}`
                  );
                  console.log(
                      `Retrying after ${retryAfter} seconds! Retry Count: ${options.request.ret  ryCount}`
                  );
                  return true;
              },
>>            onAbuseLimit: (retryAfter, options) => {
                  console.warn(
                      `Abuse detected for request ${options.method} ${options.url}`
                  );
              },
≃         }, request: { retries: 1, doNotRetry: [403], },
      });

@Chocrates
Copy link

Additionally I don't think I agree that the docs are invalid, they mention that requests will retry once, code for that is here https://github.com/octokit/octokit.js/blob/v1.7.0/src/octokit.ts#L41

But that throttling needs to be done with the throttling plugin which is not included by default as you mentioned in the opening ticket.

@postatum
Copy link
Author

postatum commented Dec 6, 2021

I see. Thanks for the answer!

@Fs00
Copy link

Fs00 commented Feb 22, 2022

Additionally I don't think I agree that the docs are invalid, they mention that requests will retry once, code for that is here https://github.com/octokit/octokit.js/blob/v1.7.0/src/octokit.ts#L41

I've stumbled upon this recently and, after investigating a bit, it seems like that the code @Chocrates is referring to is not actually used.
The throttle.onRateLimit and throttle.onAbuseLimit options are being used by the throttling plugin here, and therefore if the throttling plugin is disabled, those functions will never be called.

Also, it seems like that the throttling plugin was disabled by mistake in #2116. That PR bumps the version of the throttling plugin to include the fix for #2115, but it also strangely disables the plugin without any notice.
Wouldn't it be best to re-enable it in order to fix this issue?

@gr2m
Copy link
Contributor

gr2m commented Jun 8, 2022

It's been a while, but it looks like the TypeScript problems no longer persist, so I re-enabled the throttling plugin in #2224

@gr2m gr2m closed this as completed in #2224 Jun 8, 2022
@github-actions
Copy link

github-actions bot commented Jun 8, 2022

🎉 This issue has been resolved in version 1.7.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants