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

[Integration][PagerDuty] Fix Rate limit from querying Pagerduty service analytics #1085

Merged
merged 64 commits into from
Nov 21, 2024

Conversation

lordsarcastic
Copy link
Contributor

Description

What - Implemented rate limiter on PagerDuty

Why - Lots of requests are failing as 429.

How - Used a single sephamore since concurrent requests are not advised

Type of change

  • Bug fix (non-breaking change which fixes an issue)

All tests should be run against the port production environment(using a testing org).

Core testing checklist

  • Integration able to create all default resources from scratch
  • Resync finishes successfully
  • Resync able to create entities
  • Resync able to update entities
  • Resync able to detect and delete entities
  • Scheduled resync able to abort existing resync and start a new one
  • Tested with at least 2 integrations from scratch
  • Tested with Kafka and Polling event listeners
  • Tested deletion of entities that don't pass the selector

Integration testing checklist

  • Integration able to create all default resources from scratch
  • Resync able to create entities
  • Resync able to update entities
  • Resync able to detect and delete entities
  • Resync finishes successfully
  • If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the examples folder in the integration directory.
  • If resource kind is updated, run the integration with the example data and check if the expected result is achieved
  • If new resource kind is added or updated, validate that live-events for that resource are working as expected
  • Docs PR link here

Preflight checklist

  • Handled rate limiting
  • Handled pagination
  • Implemented the code in async
  • Support Multi account

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.

@lordsarcastic lordsarcastic requested a review from a team as a code owner October 18, 2024 17:07
@lordsarcastic lordsarcastic changed the title Bug | Rate limit from querying Pagerduty service analytics [Integration][PagerDuty] Bug | Rate limit from querying Pagerduty service analytics Oct 18, 2024
Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

looks good overall, left some suggestions

integrations/pagerduty/clients/pagerduty.py Outdated Show resolved Hide resolved
integrations/pagerduty/helpers/rate_limiter.py Outdated Show resolved Hide resolved
integrations/pagerduty/helpers/rate_limiter.py Outdated Show resolved Hide resolved
integrations/pagerduty/helpers/rate_limiter.py Outdated Show resolved Hide resolved
integrations/pagerduty/clients/pagerduty.py Outdated Show resolved Hide resolved
integrations/pagerduty/clients/pagerduty.py Outdated Show resolved Hide resolved
…s and errors,

Cleans up get_service_analytics to be more focused on get service analytics functionality.
- Use send_api_request in paginate_request_to_pager_duty, get_singular_from_pager_duty, get_incident_analytics, create_webhooks_if_not_exists
- Simplify exception handling remove redundant _handle_rate_limiting
- Centralize API request logic in send_api_request
@github-actions github-actions bot added size/L and removed size/M labels Oct 30, 2024
@shariff-6 shariff-6 requested a review from PeyGis October 30, 2024 17:36
Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

this looks good. just one minor comment

integrations/pagerduty/clients/pagerduty.py Outdated Show resolved Hide resolved
@shariff-6 shariff-6 requested a review from PeyGis October 30, 2024 18:08
Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

lgtm

integrations/pagerduty/main.py Outdated Show resolved Hide resolved
integrations/pagerduty/CHANGELOG.md Outdated Show resolved Hide resolved
integrations/pagerduty/CHANGELOG.md Outdated Show resolved Hide resolved
@Tankilevitch Tankilevitch changed the title [Integration][PagerDuty] Bug | Rate limit from querying Pagerduty service analytics [Integration][PagerDuty] Fix Rate limit from querying Pagerduty service analytics Nov 20, 2024
Comment on lines 81 to 82
data = await self.send_api_request(
endpoint=data_key, query_params={"offset": offset, **(params or {})}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused how data_key maps to an endpoint

Copy link
Contributor

Choose a reason for hiding this comment

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

the Pagerduty API is organized well based on resources and it follows some pattern. For example, to get incidents, you may hit https://api.pagerduty.com/incident. the response schema will be something like this

{
"incidents": [
{
"incident_number": 1,
"title": "Authentication Issues",
"description": "Authentication Issues",
"created_at": "2023-05-15T13:57:24Z",
"updated_at": "2023-05-15T13:57:24Z",
"status": "triggered",
"incident_key": "a4b73cf5d5dd4e9f96a7f0b5a2fc7773",
...}]}

that's why endpoint and data_key (the root key from the response) are the same

Copy link
Contributor

Choose a reason for hiding this comment

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

@shariff-6 let's rename the data_key to point to avoid this confusion

Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

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

LGTM

@Tankilevitch
Copy link
Contributor

Missing pyproject.toml

- Renamed `data_key` parameter to `resource` in `paginate_request_to_pager_duty` for clarity.
- Updated all function calls to use `resource` instead of `data_key`.
Copy link
Member

@matan84 matan84 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@matan84 matan84 left a comment

Choose a reason for hiding this comment

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

requested one change

Copy link
Member

Choose a reason for hiding this comment

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

Why did this change

Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@matan84 matan84 left a comment

Choose a reason for hiding this comment

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

lgtm

@PeyGis PeyGis merged commit 6e438e6 into main Nov 21, 2024
18 checks passed
@PeyGis PeyGis deleted the port-10931 branch November 21, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants