Skip to content

🔧 chore(pagerduty): Support "default" for custom severity config#83564

Merged
iamrajjoshi merged 3 commits into
masterfrom
raj/chore/support-severity-key-for-pagerduty
Jan 21, 2025
Merged

🔧 chore(pagerduty): Support "default" for custom severity config#83564
iamrajjoshi merged 3 commits into
masterfrom
raj/chore/support-severity-key-for-pagerduty

Conversation

@iamrajjoshi

@iamrajjoshi iamrajjoshi commented Jan 16, 2025

Copy link
Copy Markdown
Collaborator

As part of ACI Notification Action, we need to consolidate the configuration of Actions between the metric and issue alert registries into a "superset". This PR will get us there for Pagerduty.

Currently we store configuration for Pagerduty AlertRuleTriggerActions in sentry_app_config as follows: {"priority":"critical"}. We have something similar for Issue Alerts, so we are good there.

However, these is 1 thing that isn't aligned:
Issue Alerts have the following option for severity/priority: "default", "critical", "warning", "error", "info"
Metric Alerts have: "critical", "warning", "error", "info"

Notice that "default" is missing. You can also confirm this by trying to configure a pager duty on both Issue/Metric UI - Metric Alert action doesn't have a default option.

Issue
image

Metric
image


In the future when Actions can be attached to Workflows that will invoke either Issue/Metric registries, we need the configuration to be the same for the API for Actions to be the same for all automations.

What this PR does is allows us to pass "default" as a severity to Metric Alert action handler and it will use the default severity behavior that is determined here:

severity = "info"
if new_status == IncidentStatus.CRITICAL:
severity = "critical"
elif new_status == IncidentStatus.WARNING:
severity = "warning"
elif new_status == IncidentStatus.CLOSED:
severity = "info"


I tried explaining the reasoning here, but you need more clarifications, lmk!

references:

PagerdutySeverity = Literal["default", "critical", "warning", "error", "info"]

https://www.notion.so/sentry/ACI-Notification-Type-Problem-16f8b10e4b5d80f5a41af031daf3459b#1708b10e4b5d800d8789e988e1c6c25e

closes https://getsentry.atlassian.net/browse/ACI-108

@iamrajjoshi iamrajjoshi self-assigned this Jan 16, 2025
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 16, 2025
@codecov

codecov Bot commented Jan 16, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #83564      +/-   ##
==========================================
+ Coverage   87.55%   87.59%   +0.04%     
==========================================
  Files        9409     9490      +81     
  Lines      537866   538926    +1060     
  Branches    21179    21179              
==========================================
+ Hits       470916   472098    +1182     
+ Misses      66602    66480     -122     
  Partials      348      348              

@iamrajjoshi iamrajjoshi marked this pull request as ready for review January 16, 2025 15:48
@iamrajjoshi iamrajjoshi requested review from a team as code owners January 16, 2025 15:48
@iamrajjoshi iamrajjoshi requested a review from a team January 16, 2025 18:46

severity = app_config.get("priority", None)
if severity is not None:
if severity is not None and severity in PAGERDUTY_DEFAULT_SEVERITY:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PAGERDUTY_DEFAULT_SEVERITY is a string?

Suggested change
if severity is not None and severity in PAGERDUTY_DEFAULT_SEVERITY:
if severity is not None and severity in PagerdutySeverity:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

whoops it needs to be severity == PAGERDUTY_DEFAULT_SEVERITY


@responses.activate
def test_custom_severity_with_default_severity(self):
# default closed incident severity is info, setting severity to default should be ignored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this test assert that we used the default severity mapping for metric alerts?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah it makes sure if the priority is default, (meaning we have added a config) it will still use the default priority.

@iamrajjoshi iamrajjoshi merged commit a5e9581 into master Jan 21, 2025
@iamrajjoshi iamrajjoshi deleted the raj/chore/support-severity-key-for-pagerduty branch January 21, 2025 17:10
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants