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

feat: PagerDuty Rate Limiting #7218

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

shimonp21
Copy link
Contributor

@shimonp21 shimonp21 commented Jan 26, 2023

In my tests (toy account with 1000 incidents), this fixed the rate-limit errors that prevented cloudquery from getting data. Fetching '*' took 5m13s unfortunately.

I used the RateLimitHttpClient instead of just calling limiter.Wait before every call, since some calls are paginated by the SDK (i.e. the for loop is in the pagerduty-go sdk).

mention: #6981

@shimonp21 shimonp21 requested a review from a team as a code owner January 26, 2023 15:13
In my tests (toy account with 1000 incidents), this fixed the rate-limit errors that prevented cloudquery from getting data.
Fetching '*' took 5m13s unfortunately.
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

The code looks good @shimonp21. Have you considered using a retry with a backoff when the limit is reached (similar to here)? Otherwise with the approach in this PR we can't guarantee we use the max available quota

@shimonp21
Copy link
Contributor Author

@erezrokah I considered it - but because the limit is per org (https://developer.pagerduty.com/docs/ZG9jOjExMDI5NTUz-rate-limiting#what-are-our-limits), I strongly want to avoid "using all available quota" :).
I really want to avoid locking up user's accounts because of "too many requests".

@shimonp21 shimonp21 added the automerge Automatically merge once required checks pass label Jan 26, 2023
@kodiakhq kodiakhq bot merged commit 09fb388 into cloudquery:main Jan 26, 2023
kodiakhq bot pushed a commit that referenced this pull request Jan 26, 2023
🤖 I have created a release *beep* *boop*
---


## [1.3.0](plugins-source-pagerduty-v1.2.2...plugins-source-pagerduty-v1.3.0) (2023-01-26)


### Features

* Pagerduty `team_ids` config ([#7206](#7206)) ([29832cf](29832cf))
* Pagerduty `team_ids` config ([#7206](#7206)) ([883f6e6](883f6e6))
* PagerDuty Rate Limiting ([#7218](#7218)) ([09fb388](09fb388))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk to v1.29.0 ([#7121](#7121)) ([b7441c9](b7441c9))
* Fetch incidents with `dateRange="all"`, and add query examples to docs ([#7184](#7184)) ([0d84525](0d84525))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit that referenced this pull request Jan 29, 2023
amanenk pushed a commit to amanenk/cloudquery that referenced this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge once required checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants