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

[Github] Fix Rate limit error #2711

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Conversation

moxarth-rathod
Copy link
Contributor

@moxarth-rathod moxarth-rathod commented Jul 17, 2024

Closes #2572

Relates to #2636

This PR fixes the rate limiting issue in the Github connector while facing a 403 status code (Forbidden). The connector is able to fetch the documents after token has hit the rate limit.

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
  • if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Release Note

Fixing the rate limit issue while facing 403 - Forbidden error.

Copy link
Contributor

@navarone-feekery navarone-feekery left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing.
I want to confirm, does this PR actually close the issue #2636 ? Because reading that issue, it sounds like a somewhat different problem.

Comment on lines -804 to -805
if self.get_rate_limit_encountered(exception.status_code, exception):
await self._put_to_sleep(resource_type="graphql")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we no longer want to check rate limiting after a BadGraphQLRequest exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function raises QueryError instead of BadGraphQLRequest when rate limit is encountered.

)
return set()
return {scope.strip() for scope in scopes.split(",")}
except Exception as exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there not a specific exception to capture here? I think it's worthwhile to separate specific error exceptions and unexpected exceptions


return list(invalid_repos)
return list(invalid_repos)
except Exception as exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above; is there a specific exception we can try to catch first?

@moxarth-rathod
Copy link
Contributor Author

I want to confirm, does this PR actually close the issue #2636 ? Because reading that issue, it sounds like a somewhat different problem.

I have updated the description; added #2636 for Relates to tag. Because i think the issue has happened due to Rate limiting issue; not because of a private repository. We even informed this issue to the reporter - #2636 (comment).

Copy link
Contributor

@navarone-feekery navarone-feekery left a comment

Choose a reason for hiding this comment

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

Nice work 🚀

@seanstory
Copy link
Member

Since it's a bugfix, adding a label for the most recent minor to ensure a backport.

@moxarth-rathod moxarth-rathod enabled auto-merge (squash) August 12, 2024 04:57
@moxarth-rathod moxarth-rathod merged commit d048089 into main Aug 12, 2024
2 checks passed
@moxarth-rathod moxarth-rathod deleted the github-fix-rate-limit-error branch August 12, 2024 05:14
Copy link

💔 Failed to create backport PR(s)

Status Branch Result
8.15 Commit could not be cherrypicked due to conflicts

To backport manually run:
backport --pr 2711 --autoMerge --autoMergeMethod squash

@seanstory
Copy link
Member

@moxarth-elastic can you run the ☝️ above command, resolve the conflicts, and be sure to open the 8.15 backport?

@moxarth-rathod
Copy link
Contributor Author

@moxarth-elastic can you run the ☝️ above command, resolve the conflicts, and be sure to open the 8.15 backport?

Backport PR Link - #2746

seanstory pushed a commit that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github Sync job gets terminated if the API token is rate limited at start
4 participants