-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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(integrations): custom pagerduty + opsgenie metric alert priorities FE #66929
feat(integrations): custom pagerduty + opsgenie metric alert priorities FE #66929
Conversation
26803fb
to
f035e4e
Compare
export const DefaultPriorities = { | ||
[ActionType.PAGERDUTY]: {[0]: 'critical', [1]: 'warning'}, | ||
[ActionType.OPSGENIE]: {[0]: 'P1', [1]: 'P2'}, | ||
}; |
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.
these are placeholders. maybe we should prefix them with severity
for Pagerduty or priority
for Opsgenie
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.
default pagerduty severities:
sentry/src/sentry/integrations/pagerduty/utils.py
Lines 100 to 106 in f035e4e
severity = "info" | |
if new_status == IncidentStatus.CRITICAL: | |
severity = "critical" | |
elif new_status == IncidentStatus.WARNING: | |
severity = "warning" | |
elif new_status == IncidentStatus.CLOSED: | |
severity = "info" |
default opsgenie priorities:
sentry/src/sentry/integrations/opsgenie/utils.py
Lines 34 to 36 in f035e4e
priority = "P1" | |
if new_status == IncidentStatus.WARNING: | |
priority = "P2" |
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.
generally LGTM - comments are mostly just nitpicks / minor improvements to the TS :)
{hasPriorityFlag && | ||
availableAction && | ||
(availableAction.type === 'opsgenie' || | ||
availableAction.type === 'pagerduty') ? ( |
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.
nit: might be nice to make this a const before the render, for readability for the html
const isSelectEnabled = hasPriorityFlag && availableAction &&
(availableAction.type === 'opsgenie' || availableAction.type === 'pagerduty');
...
{isSelectEnabled && <SelectControl .... />}
actionIdx | ||
)} | ||
/> | ||
) : null} |
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.
nit: no need to add null
here
) : null} | |
)} |
@@ -185,6 +185,17 @@ export const TargetLabel = { | |||
[TargetType.TEAM]: t('Team'), | |||
}; | |||
|
|||
export const PriorityOptions = { |
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.
export const PriorityOptions = { | |
export const PriorityOptions: Record<ActionType, string[]> = { |
@@ -185,6 +185,17 @@ export const TargetLabel = { | |||
[TargetType.TEAM]: t('Team'), | |||
}; | |||
|
|||
export const PriorityOptions = { | |||
[ActionType.PAGERDUTY]: ['critical', 'warning', 'error', 'info'], |
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.
might be nice to move these priorities to an enum rather than a inlining them
export enum PagerDutyPriorities {
CRITICAL = 'critical',
WARNING = 'warning',
ERROR = 'error',
INFO = 'info',
};
export enum OpsGeniePriorities {
P1: 'P1',
P2: 'P2',
...
}
This is also nice since you're using these strings later in DefaultPriories
too.
export const DefaultPriorities = {
[ActionType.PAGERDUTY]: {[0]: PagerDutyPriorities.CRITICAL, [1]: PagerDutyPriorities.WARNING},
[ActionType.OPSGENIE]: {[0]: OpsGeniePriorties.P1, [1]: OpsGeniePriorties.P2},
};
(availableAction.type === 'opsgenie' || | ||
availableAction.type === 'pagerduty') ? ( |
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.
(availableAction.type === 'opsgenie' || | |
availableAction.type === 'pagerduty') ? ( | |
(availableAction.type === 'opsgenie' || | |
availableAction.type === 'pagerduty') ? ( |
also suggest moving these opsgenie
and pagerduty
to constants at the top of the file:
const OPSGENIE = 'opsgenie';
const PAGERDUTY = 'pagerduty';
export const DefaultPriorities = { | ||
[ActionType.PAGERDUTY]: {[0]: 'critical', [1]: 'warning'}, | ||
[ActionType.OPSGENIE]: {[0]: 'P1', [1]: 'P2'}, |
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'd recommend adding some typing to this, then changing the data structure from an object to an array.
export const DefaultPriorities = { | |
[ActionType.PAGERDUTY]: {[0]: 'critical', [1]: 'warning'}, | |
[ActionType.OPSGENIE]: {[0]: 'P1', [1]: 'P2'}, | |
export const DefaultPriorities: Record<ActionType, string[]> = { | |
[ActionType.PAGERDUTY]: ['critical', 'warning'], | |
[ActionType.OPSGENIE]: ['P1', 'P2'], |
Completes custom metric alert priority work for Pagerduty and Opsgenie. The (feature flagged) frontend allows for settings these values, and defaults to the original logic:
critical
severity, warning =>warning
severityP1
priority, warning =>P2
priorityScreen.Recording.2024-03-13.at.14.51.31.mov
Note that we don't show the priority in the description of the action unless it has been explicitly set to something.
For #58830
For #54921