-
Notifications
You must be signed in to change notification settings - Fork 62
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
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
…cover-projects-v-3-get-requests-per-minute-per-project
Lets test on the different selector configurations we have and ensure real time works as expected |
Co-authored-by: Michael Kofi Armah <[email protected]>
Co-authored-by: Michael Kofi Armah <[email protected]>
…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
…cover-projects-v-3-get-requests-per-minute-per-project
integrations/gcp/gcp_core/utils.py
Outdated
async def set_get_project_limiter() -> None: | ||
global GET_PROJECT_LIMITER | ||
get_project_quota_id = "ProjectV3GetRequestsPerMinutePerProject" | ||
GET_PROJECT_LIMITER, _ = await resolve_request_controllers( | ||
AssetTypesWithSpecialHandling.PROJECT, quota_id=get_project_quota_id | ||
) |
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 not set this if the integration is meant to run ONCE
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.
Done
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.
Left some comments
integrations/gcp/gcp_core/utils.py
Outdated
|
||
|
||
@ocean.on_start() | ||
async def set_get_project_limiter() -> 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.
async def set_get_project_limiter() -> None: | |
async def on_start() -> None: |
integrations/gcp/gcp_core/utils.py
Outdated
@ocean.on_start() | ||
async def set_get_project_limiter() -> 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.
move to main.py
integrations/gcp/main.py
Outdated
asset_resource_data = await feed_event_to_resource( | ||
asset_type, | ||
asset_name, | ||
asset_project, | ||
asset_data, | ||
config, | ||
) |
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.
pass the get project rate limiter from here
…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, few changes requested
Co-authored-by: Michael Kofi Armah <[email protected]>
…cover-projects-v-3-get-requests-per-minute-per-project
Co-authored-by: Michael Kofi Armah <[email protected]>
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.
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.
- Don't create multiple places where we handle the limiter, always get it from the same place
- Keep the changelog as accurate as possible- You've changed a core part of the integration, make sure to keep the changelog updated correctly.
- Explicit is always better than implicit
- The dependency injection in the folder with the limiter looks abit redundant to me, I think that get_single_project should be the only one to compute it's rate_limiter.
asset_name, | ||
asset_project, | ||
asset_data, | ||
PROJECT_V3_GET_REQUESTS_RATE_LIMITER, |
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.
Explain in a comment why are you giving a static value and not dynamic value based on asset_type
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.
The implementation instantiated this quota handling class once on_start and created the AsyncLimiter
and then subsequently uses this limiter for every real time event. So they can be shared by this events and the limit can hold
@@ -38,6 +39,8 @@ | |||
resolve_request_controllers, | |||
) | |||
|
|||
PROJECT_V3_GET_REQUESTS_RATE_LIMITER: AsyncLimiter |
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.
Remove this
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.
This is how we initialized the limiter to ensured that even when event context cycle has been completed the limiter persist and it is aware of the used quota and the available quota
global PROJECT_V3_GET_REQUESTS_RATE_LIMITER | ||
if not ocean.event_listener_type == "ONCE": | ||
get_project_quota_id = "ProjectV3GetRequestsPerMinutePerProject" | ||
PROJECT_V3_GET_REQUESTS_RATE_LIMITER, _ = await resolve_request_controllers( |
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.
Remove this as well
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.
This part of the code instantiates the realtime event rate limiter once on_start so that it can be subsequently used for every realtime event.
asset_name, | ||
asset_project, | ||
asset_data, | ||
PROJECT_V3_GET_REQUESTS_RATE_LIMITER, |
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_V3_GET_REQUESTS_RATE_LIMITER, | |
project_get_requests_per_minute_per_project.limiter(asset_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.
This is a dynamic implementation, which means every call to the endpoint creates a new instance of the limiter. This implementation doesn't seem to work well with realtime event
@@ -214,15 +216,21 @@ async def search_all_organizations() -> ASYNC_GENERATOR_RESYNC_TYPE: | |||
|
|||
|
|||
async def get_single_project( | |||
project_name: str, config: Optional[ProtoConfig] = None | |||
project_name: str, | |||
rate_limiter: AsyncLimiter, |
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.
Why inject the rate_limiter? you should just use the project_get_requests_per_minute_per_project always, no?
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.
Same as above
Co-authored-by: Matan <[email protected]>
Co-authored-by: Matan <[email protected]>
Co-authored-by: Matan <[email protected]>
…cover-projects-v-3-get-requests-per-minute-per-project
…cts-v-3-get-requests-per-minute-per-project' of https://github.com/port-labs/ocean into PORT-12220-bug-extend-gcp-quota-handling-to-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.