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

Fix various rate limiting issues #1424

Merged
merged 10 commits into from
Jul 31, 2020
Merged

Fix various rate limiting issues #1424

merged 10 commits into from
Jul 31, 2020

Conversation

hipstersmoothie
Copy link
Collaborator

@hipstersmoothie hipstersmoothie commented Jul 30, 2020

What Changed

previously we would just error out on the abuse limit, now we will retry

Why

#1422 (I'm not sure this PR solves for #1422)

📦 Published PR as canary version: under canary scope @auto-canary@9.49.3-canary.1424.17806.0

✨ Test out this PR locally via:

npm install @auto-canary/bot-list@9.49.3-canary.1424.17806.0
npm install @auto-canary/auto@9.49.3-canary.1424.17806.0
npm install @auto-canary/core@9.49.3-canary.1424.17806.0
npm install @auto-canary/all-contributors@9.49.3-canary.1424.17806.0
npm install @auto-canary/brew@9.49.3-canary.1424.17806.0
npm install @auto-canary/chrome@9.49.3-canary.1424.17806.0
npm install @auto-canary/cocoapods@9.49.3-canary.1424.17806.0
npm install @auto-canary/conventional-commits@9.49.3-canary.1424.17806.0
npm install @auto-canary/crates@9.49.3-canary.1424.17806.0
npm install @auto-canary/exec@9.49.3-canary.1424.17806.0
npm install @auto-canary/first-time-contributor@9.49.3-canary.1424.17806.0
npm install @auto-canary/gem@9.49.3-canary.1424.17806.0
npm install @auto-canary/gh-pages@9.49.3-canary.1424.17806.0
npm install @auto-canary/git-tag@9.49.3-canary.1424.17806.0
npm install @auto-canary/gradle@9.49.3-canary.1424.17806.0
npm install @auto-canary/jira@9.49.3-canary.1424.17806.0
npm install @auto-canary/maven@9.49.3-canary.1424.17806.0
npm install @auto-canary/npm@9.49.3-canary.1424.17806.0
npm install @auto-canary/omit-commits@9.49.3-canary.1424.17806.0
npm install @auto-canary/omit-release-notes@9.49.3-canary.1424.17806.0
npm install @auto-canary/released@9.49.3-canary.1424.17806.0
npm install @auto-canary/s3@9.49.3-canary.1424.17806.0
npm install @auto-canary/slack@9.49.3-canary.1424.17806.0
npm install @auto-canary/twitter@9.49.3-canary.1424.17806.0
npm install @auto-canary/upload-assets@9.49.3-canary.1424.17806.0
# or 
yarn add @auto-canary/bot-list@9.49.3-canary.1424.17806.0
yarn add @auto-canary/auto@9.49.3-canary.1424.17806.0
yarn add @auto-canary/core@9.49.3-canary.1424.17806.0
yarn add @auto-canary/all-contributors@9.49.3-canary.1424.17806.0
yarn add @auto-canary/brew@9.49.3-canary.1424.17806.0
yarn add @auto-canary/chrome@9.49.3-canary.1424.17806.0
yarn add @auto-canary/cocoapods@9.49.3-canary.1424.17806.0
yarn add @auto-canary/conventional-commits@9.49.3-canary.1424.17806.0
yarn add @auto-canary/crates@9.49.3-canary.1424.17806.0
yarn add @auto-canary/exec@9.49.3-canary.1424.17806.0
yarn add @auto-canary/first-time-contributor@9.49.3-canary.1424.17806.0
yarn add @auto-canary/gem@9.49.3-canary.1424.17806.0
yarn add @auto-canary/gh-pages@9.49.3-canary.1424.17806.0
yarn add @auto-canary/git-tag@9.49.3-canary.1424.17806.0
yarn add @auto-canary/gradle@9.49.3-canary.1424.17806.0
yarn add @auto-canary/jira@9.49.3-canary.1424.17806.0
yarn add @auto-canary/maven@9.49.3-canary.1424.17806.0
yarn add @auto-canary/npm@9.49.3-canary.1424.17806.0
yarn add @auto-canary/omit-commits@9.49.3-canary.1424.17806.0
yarn add @auto-canary/omit-release-notes@9.49.3-canary.1424.17806.0
yarn add @auto-canary/released@9.49.3-canary.1424.17806.0
yarn add @auto-canary/s3@9.49.3-canary.1424.17806.0
yarn add @auto-canary/slack@9.49.3-canary.1424.17806.0
yarn add @auto-canary/twitter@9.49.3-canary.1424.17806.0
yarn add @auto-canary/upload-assets@9.49.3-canary.1424.17806.0

@hipstersmoothie hipstersmoothie added the patch Increment the patch version when merged label Jul 30, 2020
@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #1424 into master will decrease coverage by 0.03%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1424      +/-   ##
==========================================
- Coverage   81.15%   81.12%   -0.04%     
==========================================
  Files          58       58              
  Lines        4261     4248      -13     
  Branches      936      935       -1     
==========================================
- Hits         3458     3446      -12     
  Misses        563      563              
+ Partials      240      239       -1     
Impacted Files Coverage Δ
packages/core/src/release.ts 94.30% <ø> (-0.03%) ⬇️
plugins/released/src/index.ts 87.95% <50.00%> (ø)
packages/core/src/git.ts 88.79% <57.14%> (-0.26%) ⬇️
plugins/first-time-contributor/src/index.ts 86.66% <75.00%> (+2.05%) ⬆️
packages/core/src/auto.ts 77.50% <100.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89481d6...d6e7be2. Read the comment docs.

latestTag = await this.git.getFirstCommit();
}

const commitsInRelease = await this.release.getCommits(latestTag);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this because getting all the commits in a canary release with no recent release can lead to hitting the rate limits. With canaries this is especially easy to hit because they are made so often.

);
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will now actually retry after abuse limit is hit

@@ -488,6 +491,7 @@ export default class Git {
}

/** Search to GitHub project's issue and pull requests */
@memoize()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a single run if the search query is the same it should be safe to memoize. The search endpoints have super low rate limits so this should help a bit.

@@ -505,10 +509,11 @@ export default class Git {
}

/** Run a graphql query on the GitHub project */
@memoize()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing goes for graphql queries. Easy to memoize.

async graphql<T>(query: string) {
this.logger.verbose.info("Querying Github using GraphQL:\n", query);

const data = await graphql<T>(query, {
const data = await this.github.graphql<T>(query, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to octokit's graphql function will make sure that it is rate/abuse limited properly

@@ -283,6 +283,7 @@ export default class Release {
* @param from - Tag or SHA to start at
* @param to - Tag or SHA to end at (defaults to HEAD)
*/
@memoize()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was iffy on adding memoization here. But since auto will never care about generated commits I think it's safe to memoize this function. This should also help with some log reduction

@hipstersmoothie hipstersmoothie changed the title retry after abuse limit Fix various rate limiting issues Jul 31, 2020
return author;
// prettier-ignore
// eslint-disable-next-line no-await-in-loop
const prs = await auto.git?.graphql<Record<string, any>>(`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call graphql API serially so we can use the memoized the graphql call

@@ -611,12 +612,9 @@ export default class Release {
hash: commit.hash,
});
} else if (commit.authorEmail) {
const author = await this.git.getUserByEmail(commit.authorEmail);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUserByEmail used search which is easily rate-limited, just omitting instead. It's 50/50 that the user has their email public anyway

@@ -14,9 +14,6 @@ export default class FirstTimeContributorPlugin implements IPlugin {

/** Tap into auto plugin points. */
apply(auto: Auto) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const cache: Record<string, Record<string, any>> = {};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rely on git client memoization instead of custom cache

@hipstersmoothie hipstersmoothie merged commit 8bf2f44 into master Jul 31, 2020
@hipstersmoothie hipstersmoothie deleted the abuse branch July 31, 2020 21:03
@adierkens
Copy link
Collaborator

🚀 PR was released in v9.49.3 🚀

@adierkens adierkens added the released This issue/pull request has been released. label Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants