-
Notifications
You must be signed in to change notification settings - Fork 61
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
[Integration][GCP] Added Rate Limiting for ProjectsV3GetRequestsPerMinutePerProject Quota #1304
base: main
Are you sure you want to change the base?
Conversation
…cover-projects-v-3-get-requests-per-minute-per-project
…cover-projects-v-3-get-requests-per-minute-per-project
…cover-projects-v-3-get-requests-per-minute-per-project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work... 👏🏽 left some comments
@@ -311,22 +327,27 @@ async def feed_event_to_resource( | |||
config: Optional[ProtoConfig] = None, | |||
) -> RAW_ITEM: | |||
resource = None | |||
live_event_projects_rate_limiter, _ = await resolve_request_controllers( | |||
AssetTypesWithSpecialHandling.PROJECT, method="get" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is scalable enough, there are other quota's under the same tree that use "get" and "search" prefix, see if you can pass the quota id itself in there in place of method, and rename "method" to "quota_id"
integrations/gcp/gcp_core/utils.py
Outdated
@@ -181,10 +187,36 @@ def get_service_account_project_id() -> str: | |||
|
|||
|
|||
async def get_quotas_for_project( | |||
project_id: str, kind: str | |||
project_id: str, kind: str, **kwargs: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_id: str, kind: str, **kwargs: Any | |
project_id: str, kind: str, quota_id: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets be more precise here
integrations/gcp/gcp_core/utils.py
Outdated
) -> Tuple["AsyncLimiter", "BoundedSemaphore"]: | ||
try: | ||
match kind: | ||
case AssetTypesWithSpecialHandling.PROJECT: | ||
method = kwargs.get("method") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method = kwargs.get("method") |
integrations/gcp/gcp_core/utils.py
Outdated
) -> Tuple["AsyncLimiter", "BoundedSemaphore"]: | ||
try: | ||
match kind: | ||
case AssetTypesWithSpecialHandling.PROJECT: | ||
method = kwargs.get("method") | ||
if method == "search": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if method == "search": | |
if quota_id == "apiSearchAllResourcesQpmPerProject": |
integrations/gcp/main.py
Outdated
@@ -106,7 +109,10 @@ async def resync_organizations(kind: str) -> ASYNC_GENERATOR_RESYNC_TYPE: | |||
|
|||
@ocean.on_resync(kind=AssetTypesWithSpecialHandling.PROJECT) | |||
async def resync_projects(kind: str) -> ASYNC_GENERATOR_RESYNC_TYPE: | |||
async for batch in search_all_projects(): | |||
resync_projects_rate_limiter, _ = await resolve_request_controllers( | |||
kind, method="search" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust
integrations/gcp/main.py
Outdated
@@ -62,7 +62,10 @@ async def _resolve_resync_method_for_resource( | |||
case AssetTypesWithSpecialHandling.ORGANIZATION: | |||
return search_all_organizations() | |||
case AssetTypesWithSpecialHandling.PROJECT: | |||
return search_all_projects() | |||
project_rate_limiter, _ = await resolve_request_controllers( | |||
kind, method="search" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust
…cover-projects-v-3-get-requests-per-minute-per-project
…cover-projects-v-3-get-requests-per-minute-per-project
Description
What
ProjectsV3GetRequestsPerMinutePerProject
quota.Why
429 Quota Exceeded
errors.How
ProjectsV3GetRequestsPerMinutePerProject
quota using the existing rate limiterresolve_request_controllers
method to fetch and apply this specific quota.Type of change
Please leave one option from the following and delete the rest:
All tests should be run against the port production environment(using a testing org).
Core testing checklist
Integration testing checklist
examples
folder in the integration directory.Preflight checklist
Screenshots
Include screenshots from your environment showing how the resources of the integration will look.
API Documentation
Provide links to the API documentation used for this integration.