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

API Rate Limit #24

Open
shiv810 opened this issue Oct 28, 2024 · 8 comments
Open

API Rate Limit #24

shiv810 opened this issue Oct 28, 2024 · 8 comments

Comments

@shiv810
Copy link
Collaborator

shiv810 commented Oct 28, 2024

The unlimited issue fetch depth is leading to rate limiting for the REST and GraphQL API calls. A maximum depth for issue fetching should be created and updated.

Action Run: Run

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

We should instead gracefully handle when rate limited and continue with what we can but also let the user know in the output that due to rate limits this isnt the full context possible for the response.

@Keyrxng
Copy link
Contributor

Keyrxng commented Oct 28, 2024

I don't think it's unlimited fetch depth that's causing it. This PR #21 introduced a package github-diff-tool which is used in this fn which is called with it's own Promise.all() within formatChatHistory() which uses it's own Promise.all().

Current call count using that package:

Octokit call count Map(9) {
  'rest.issues.get' => 8,
  'rest.issues.listComments' => 3,
  'repos.getContent' => 2,
  'rest.pulls.listReviewComments' => 2,
  'graphql' => 5,
  'rest.pulls.get' => 1,
  'rest.pulls.listFiles' => 8,
  'rest.repos.getContent' => 13,
  'repos.listLanguages' => 1
}

Call count without using that package without removing Promise.all():

Octokit call count Map(7) {
  'rest.issues.get' => 8,
  'rest.issues.listComments' => 3,
  'repos.getContent' => 2,
  'rest.pulls.listReviewComments' => 2,
  'graphql' => 5,
  'paginate' => 1,
  'repos.listLanguages' => 1
}
  1. Optimize usage of Promise.all() for the changes introduced in fix: ignore file path and diffs #21 that use the package. (I see it colours diffs etc. Is that stripped before fed to the model I assume?
  2. Remove usage of the package and simplify the PR diff/file fetching.
  3. Totally remove usage of Promise.all().

I tend to avoid using it completely the time benefits usually are not necessary especially for action plugins. Unless they involve API calls it's fine.

Copy link

Note

The following contributors may be suitable for this task:

0x4007

79% Match ubiquity/cloudflare-deploy-action#19

devpanther

77% Match ubiquity/ubiquibot#134

EresDev

77% Match ubiquity/devpool-directory-tasks#21

@0x4007
Copy link
Member

0x4007 commented Jan 20, 2025

[!NOTE]
The following contributors may be suitable for this task:

0x4007

79% Match ubiquity/cloudflare-deploy-action#19

devpanther

77% Match ubiquity/ubiquibot#134

EresDev

77% Match ubiquity/devpool-directory-tasks#21

It's not clear to me why this posted here now. @shiv810 any idea

@shiv810
Copy link
Collaborator Author

shiv810 commented Jan 20, 2025

It's not clear to me why this posted here now. @shiv810 any idea

I tried updating the issue, it might be because of that.

@shiv810
Copy link
Collaborator Author

shiv810 commented Jan 21, 2025

/ask explain this issue ?

Copy link

Issue #24: Unlimited Issue Fetch Depth Causing Rate Limiting

Overview:
The primary concern in Issue #24 is that the application's current implementation allows for an unlimited depth when fetching issues. This unrestricted fetching leads to excessive REST and GraphQL API calls, resulting in the application hitting GitHub's rate limits.

Detailed Explanation:

  1. Root Cause:

    • The use of the github-diff-tool package within functions like fetchPullRequestDiff and formatChatHistory involves extensive use of Promise.all(). This parallel processing approach significantly increases the number of API requests made to GitHub.
    • Specifically, the call counts without the package still include numerous API calls (e.g., rest.issues.get, graphql, repos.getContent), but the addition of github-diff-tool exacerbates the issue by introducing more simultaneous requests.
  2. Impact:

    • Exceeding GitHub's rate limits disrupts the application's ability to fetch necessary data, leading to failures in processing issues and pull requests.
    • Users experience errors such as 413 Request Entity Too Large and 404 Not Found, indicating that the application cannot retrieve or process the required information.
  3. Proposed Solutions:

    • Limit Fetch Depth: Implement a maximum depth for issue fetching to curtail the number of API calls. This restriction helps in staying within the allowed rate limits.
    • Optimize Promise.all(): Reevaluate the necessity of using Promise.all() in areas where it may not provide significant performance benefits. Reducing parallel requests can help manage the API call volume.
    • Remove or Simplify github-diff-tool: Consider eliminating the use of the github-diff-tool package or simplifying its integration to reduce the number of API interactions.
    • Graceful Rate Limiting Handling: Instead of allowing the application to be rate-limited, implement logic to detect rate limit responses. Upon reaching the limit, the application should notify users and handle the situation without crashing or throwing unhandled errors.
    • Configuration Flexibility: Allow configurations such as maximum token limits to be adjustable via environment variables or config settings, enabling better management of API usage based on different deployment environments.
  4. Current Status & Actions Taken:

    • Discussions indicate ongoing efforts to refactor the code to address these issues, including moving away from hard-coded values and improving error handling.
    • There is an emphasis on using GitHub's rate limit headers to dynamically adjust the application's behavior based on the remaining API call quota.

Conclusion:
Addressing Issue #24 is crucial for maintaining the application's reliability and ensuring it operates smoothly within GitHub's API constraints. Implementing the proposed solutions will help mitigate rate limiting problems, enhance performance, and provide a better user experience.

@0x4007
Copy link
Member

0x4007 commented Jan 21, 2025

Seems like a pretty solid overview of the problem. I appreciate the targeted advice of optimizing the use of Promise.all.

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

No branches or pull requests

3 participants