Skip to content

refactor: to use octokit throttling plugin #378

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

refactor: to use octokit throttling plugin #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4b67562
refactor: to use octokit throttling plugin
moltar Jun 1, 2021
4186b1f
test: removes throttling related tests
moltar Jun 2, 2021
1093fde
test: remove unused imports
gr2m Jun 3, 2021
d38552f
style: xo
gr2m Jun 3, 2021
17adf7b
fix: do not set `request.retries` on the `Octokit` constructor
gr2m Jun 3, 2021
abd8286
style: xo
gr2m Jun 3, 2021
fd2dd3e
test: 404 are not retried, that tests should have failed before
gr2m Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 17 additions & 38 deletions lib/get-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,17 @@
const {memoize, get} = require('lodash');
const {Octokit} = require('@octokit/rest');
const pRetry = require('p-retry');
const Bottleneck = require('bottleneck');
const {throttling} = require('@octokit/plugin-throttling');
const {retry} = require('@octokit/plugin-retry');
const urljoin = require('url-join');
const HttpProxyAgent = require('http-proxy-agent');
const HttpsProxyAgent = require('https-proxy-agent');

const {RETRY_CONF, RATE_LIMITS, GLOBAL_RATE_LIMIT} = require('./definitions/rate-limit');
const SemanticReleaseOctokit = Octokit.plugin(throttling, retry);

/**
* Http error status for which to not retry.
*/
const SKIP_RETRY_CODES = new Set([400, 401, 403]);

/**
* Create or retrieve the throttler function for a given rate limit group.
*
* @param {Array} rate The rate limit group.
* @param {String} limit The rate limits per API endpoints.
* @param {Bottleneck} globalThrottler The global throttler.
*
* @return {Bottleneck} The throller function for the given rate limit group.
*/
const getThrottler = memoize((rate, globalThrottler) =>
new Bottleneck({minTime: get(RATE_LIMITS, rate)}).chain(globalThrottler)
);
const {RETRY_CONF} = require('./definitions/rate-limit');

module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy}) => {
const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
const globalThrottler = new Bottleneck({minTime: GLOBAL_RATE_LIMIT});
const github = new Octokit({
const github = new SemanticReleaseOctokit({
auth: `token ${githubToken}`,
baseUrl,
request: {
Expand All @@ -39,24 +21,21 @@ module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy}) => {
: new HttpsProxyAgent(proxy)
: undefined,
},
});
throttle: {
onRateLimit: (retryAfter, options) => {
github.log.warn(`Request quota exhausted for request ${options.method} ${options.url}`);

github.hook.wrap('request', (request, options) => {
const access = options.method === 'GET' ? 'read' : 'write';
const rateCategory = options.url.startsWith('/search') ? 'search' : 'core';
const limitKey = [rateCategory, RATE_LIMITS[rateCategory][access] && access].filter(Boolean).join('.');

return pRetry(async () => {
try {
return await getThrottler(limitKey, globalThrottler).wrap(request)(options);
} catch (error) {
if (SKIP_RETRY_CODES.has(error.status)) {
throw new pRetry.AbortError(error);
if (options.request.retryCount <= RETRY_CONF.retries) {
github.log.debug(`Will retry after ${retryAfter}.`);
return true;
}

throw error;
}
}, RETRY_CONF);
return false;
},
onAbuseLimit: (retryAfter, options) => {
github.log.warn(`Abuse detected for request ${options.method} ${options.url}`);
},
},
});

return github;
Expand Down
25 changes: 23 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
"Gregor Martynus (https://twitter.com/gr2m)"
],
"dependencies": {
"@octokit/plugin-retry": "^3.0.0",
"@octokit/plugin-throttling": "^3.4.0",
"@octokit/rest": "^18.0.0",
"@semantic-release/error": "^2.2.0",
"aggregate-error": "^3.0.0",
"bottleneck": "^2.18.1",
"debug": "^4.0.0",
"dir-glob": "^3.0.0",
"fs-extra": "^10.0.0",
Expand All @@ -30,7 +31,6 @@
"lodash": "^4.17.4",
"mime": "^2.4.3",
"p-filter": "^2.0.0",
"p-retry": "^4.0.0",
"url-join": "^4.0.0"
},
"devDependencies": {
Expand Down
125 changes: 1 addition & 124 deletions test/get-client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ const https = require('https');
const {promisify} = require('util');
const {readFile} = require('fs-extra');
const test = require('ava');
const {inRange} = require('lodash');
const {stub, spy} = require('sinon');
const {spy} = require('sinon');
const proxyquire = require('proxyquire');
const Proxy = require('proxy');
const serverDestroy = require('server-destroy');
const {Octokit} = require('@octokit/rest');
const rateLimit = require('./helpers/rate-limit');

const getClient = proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit});
Expand Down Expand Up @@ -114,124 +112,3 @@ test.serial('Do not use a proxy if set to false', async (t) => {

await promisify(server.destroy).bind(server)();
});

test('Use the global throttler for all endpoints', async (t) => {
const rate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': {Octokit: stub().returns(octokit)},
'./definitions/rate-limit': {RATE_LIMITS: {search: 1, core: 1}, GLOBAL_RATE_LIMIT: rate},
})({githubToken: 'token'});

/* eslint-disable unicorn/prevent-abbreviations */

const a = await github.repos.createRelease();
const b = await github.issues.createComment();
const c = await github.repos.createRelease();
const d = await github.issues.createComment();
const e = await github.search.issuesAndPullRequests();
const f = await github.search.issuesAndPullRequests();

// `issues.createComment` should be called `rate` ms after `repos.createRelease`
t.true(inRange(b - a, rate - 50, rate + 50));
// `repos.createRelease` should be called `rate` ms after `issues.createComment`
t.true(inRange(c - b, rate - 50, rate + 50));
// `issues.createComment` should be called `rate` ms after `repos.createRelease`
t.true(inRange(d - c, rate - 50, rate + 50));
// `search.issuesAndPullRequests` should be called `rate` ms after `issues.createComment`
t.true(inRange(e - d, rate - 50, rate + 50));
// `search.issuesAndPullRequests` should be called `rate` ms after `search.issuesAndPullRequests`
t.true(inRange(f - e, rate - 50, rate + 50));

/* eslint-enable unicorn/prevent-abbreviations */
});

test('Use the same throttler for endpoints in the same rate limit group', async (t) => {
const searchRate = 300;
const coreRate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': {Octokit: stub().returns(octokit)},
'./definitions/rate-limit': {RATE_LIMITS: {search: searchRate, core: coreRate}, GLOBAL_RATE_LIMIT: 1},
})({githubToken: 'token'});

/* eslint-disable unicorn/prevent-abbreviations */

const a = await github.repos.createRelease();
const b = await github.issues.createComment();
const c = await github.repos.createRelease();
const d = await github.issues.createComment();
const e = await github.search.issuesAndPullRequests();
const f = await github.search.issuesAndPullRequests();

// `issues.createComment` should be called `coreRate` ms after `repos.createRelease`
t.true(inRange(b - a, coreRate - 50, coreRate + 50));
// `repos.createRelease` should be called `coreRate` ms after `issues.createComment`
t.true(inRange(c - b, coreRate - 50, coreRate + 50));
// `issues.createComment` should be called `coreRate` ms after `repos.createRelease`
t.true(inRange(d - c, coreRate - 50, coreRate + 50));

// The first search should be called immediately as it uses a different throttler
t.true(inRange(e - d, -50, 50));
// The second search should be called only after `searchRate` ms
t.true(inRange(f - e, searchRate - 50, searchRate + 50));

/* eslint-enable unicorn/prevent-abbreviations */
});

test('Use different throttler for read and write endpoints', async (t) => {
const writeRate = 300;
const readRate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': {Octokit: stub().returns(octokit)},
'./definitions/rate-limit': {RATE_LIMITS: {core: {write: writeRate, read: readRate}}, GLOBAL_RATE_LIMIT: 1},
})({githubToken: 'token'});

const a = await github.repos.get();
const b = await github.repos.get();
const c = await github.repos.createRelease();
const d = await github.repos.createRelease();

// `repos.get` should be called `readRate` ms after `repos.get`
t.true(inRange(b - a, readRate - 50, readRate + 50));
// `repos.createRelease` should be called `coreRate` ms after `repos.createRelease`
t.true(inRange(d - c, writeRate - 50, writeRate + 50));
});

test('Use the same throttler when retrying', async (t) => {
const coreRate = 200;
const request = stub().callsFake(async () => {
const err = new Error();
err.time = Date.now();
err.status = 404;
throw err;
});
const octokit = new Octokit();
octokit.hook.wrap('request', request);
const github = proxyquire('../lib/get-client', {
'@octokit/rest': {Octokit: stub().returns(octokit)},
'./definitions/rate-limit': {
RETRY_CONF: {retries: 3, factor: 1, minTimeout: 1},
RATE_LIMITS: {core: coreRate},
GLOBAL_RATE_LIMIT: 1,
},
})({githubToken: 'token'});

await t.throwsAsync(github.repos.createRelease());
const {time: a} = await t.throwsAsync(request.getCall(0).returnValue);
const {time: b} = await t.throwsAsync(request.getCall(1).returnValue);
const {time: c} = await t.throwsAsync(request.getCall(2).returnValue);
const {time: d} = await t.throwsAsync(request.getCall(3).returnValue);

// Each retry should be done after `coreRate` ms
t.true(inRange(b - a, coreRate - 50, coreRate + 50));
t.true(inRange(c - b, coreRate - 50, coreRate + 50));
t.true(inRange(d - c, coreRate - 50, coreRate + 50));
});
14 changes: 11 additions & 3 deletions test/verify.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ test.serial(
await t.notThrowsAsync(
verify(
{proxy, assets, successComment, failTitle, failComment, labels},
{env, options: {repositoryUrl: `git+https://othertesturl.com/${owner}/${repo}.git`}, logger: t.context.logger}
{
env,
options: {repositoryUrl: `git+https://othertesturl.com/${owner}/${repo}.git`},
logger: t.context.logger,
}
)
);
t.true(github.isDone());
Expand Down Expand Up @@ -440,7 +444,11 @@ test('Throw SemanticReleaseError for missing github token', async (t) => {
const [error, ...errors] = await t.throwsAsync(
verify(
{},
{env: {}, options: {repositoryUrl: 'https://github.com/semantic-release/github.git'}, logger: t.context.logger}
{
env: {},
options: {repositoryUrl: 'https://github.com/semantic-release/github.git'},
logger: t.context.logger,
}
)
);

Expand Down Expand Up @@ -526,7 +534,7 @@ test.serial("Throw SemanticReleaseError if the repository doesn't exist", async
const owner = 'test_user';
const repo = 'test_repo';
const env = {GH_TOKEN: 'github_token'};
const github = authenticate(env).get(`/repos/${owner}/${repo}`).times(4).reply(404);
const github = authenticate(env).get(`/repos/${owner}/${repo}`).reply(404);

const [error, ...errors] = await t.throwsAsync(
verify({}, {env, options: {repositoryUrl: `https://github.com/${owner}/${repo}.git`}, logger: t.context.logger})
Expand Down