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: rate limit GitLab requests when fetching tokens #3126

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cuserox
Copy link
Contributor

@cuserox cuserox commented Sep 5, 2024

Why does this PR exist?

Originally flagged by members of the community, where the plugin would intermittently fail to fetch all the tokens from their GitLab repository.

Specifically, the calls to the /projects/*/repository/files/*/raw endpoint in GitLab, which is used by the plugin when read()ing from the repository.

We use the @gitbeaker/rest library as a wrapper to interact with GitLab, in this case, the equivalent of calling the above endpoint is when this.gitLabClient.RepositoryFiles.showRaw(...) is invoked. This library offers a rateLimit configuration to be passed to the client.

Gathering data

Cloning the user's repository, there were around ~140 requests in .56s (equating to 250 rps) to the .../raw endpoint. GitLab's documentation states that "the rate limit on raw endpoints defaults to 300 requests per minute", the same as 5 rps.

What does this pull request do?

Adds a rateLimit configuration to the GitLab client when it's instantiated: '/projects/*/repository/files/*/raw': 4 (4 rps = 250 requests per minute)

The time that it took to wait for all raw files to be fetched below were calculated by using the native time method, 5 runs locally, like so:

console.time('this.gitlabClient.RepositoryFiles.showRaw');

const jsonFileContents = await Promise.all(jsonFiles.map((treeItem) => (
   this.gitlabClient.RepositoryFiles.showRaw(this.projectId!, treeItem.path, this.branch)
)));

console.timeEnd('this.gitlabClient.RepositoryFiles.showRaw');
  • Before rate limiting: average of 1.5s
  • After rate limiting: average of 1.9s

Testing this change

⚠️ Note: testing in my own cloned repository based on the user's tokens did not end up in timing out, even before rate limiting!

🤔 If someone can test before and after this change, for safety of not slowing down other users too much.

Copy link

changeset-bot bot commented Sep 5, 2024

⚠️ No Changeset found

Latest commit: bf7c8fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@cuserox cuserox force-pushed the fix/rate-limit-gitlab-raw-file-fetching branch from 3c1abf7 to 9c65e89 Compare September 5, 2024 11:18
Copy link
Contributor

github-actions bot commented Sep 5, 2024

⤵️ 📦 ✨ The artifact was successfully created! Want to test it? Download it here 👀 🎁

@cuserox cuserox force-pushed the fix/rate-limit-gitlab-raw-file-fetching branch from 9c65e89 to bf7c8fc Compare September 5, 2024 11:19
Copy link
Contributor

github-actions bot commented Sep 5, 2024

Commit SHA:a2efd0d502d6399d466f3cf58cc3fe2c62f6b369
No changes to code coverage between the base branch and the head branch

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 this pull request may close these issues.

1 participant