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.

Sign up for GitHub

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

Jump to bottom

feat(opsgenie): add custom priorities to issue alerts #56140

Merged
merged 7 commits into from
Sep 29, 2023
Merged

feat(opsgenie): add custom priorities to issue alerts #56140

merged 7 commits into from
Sep 29, 2023

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Sep 12, 2023

What it says in the title. Let users choose priorities for Opsgenie issue alerts.

@mifu67 mifu67 requested a review from a team as a code owner September 12, 2023 23:27
@mifu67 mifu67 requested review from leeandher and a team September 12, 2023 23:27
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 12, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023 •
edited
Loading

Codecov Report

Merging #56140 (ae99d88) into master (5693946) will increase coverage by 1.08%.
Report is 9 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head ae99d88 differs from pull request most recent head dc4267a. Consider uploading reports for the commit dc4267a to get more accurate results

@@            Coverage Diff             @@
##           master   #56140      +/-   ##
==========================================
+ Coverage   78.91%   80.00%   +1.08%     
==========================================
  Files        5114     5066      -48     
  Lines      220509   217981    -2528     
  Branches    37328    36889     -439     
==========================================
+ Hits       174005   174385     +380     
+ Misses      40879    38247    -2632     
+ Partials     5625     5349     -276
Files Coverage Δ
...ntry/integrations/opsgenie/actions/notification.py 90.76% <100.00%> (ø)
src/sentry/integrations/opsgenie/client.py 89.47% <100.00%> (-0.53%) ⬇️

... and 689 files with indirect coverage changes

leeandher
leeandher reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@leeandher leeandher 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! But we should probably add it for metric alerts at the same time so we don't need to feature flag it. Also if we could get some UI screenshots that'd be awesome.

src/sentry/integrations/opsgenie/actions/notification.py Outdated Show resolved Hide resolved
src/sentry/integrations/opsgenie/actions/notification.py Outdated Show resolved Hide resolved
src/sentry/integrations/opsgenie/actions/notification.py Outdated Show resolved Hide resolved
leeandher
leeandher reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@leeandher leeandher 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! But we should probably add it for metric alerts at the same time so we don't need to feature flag it. Also if we could get some UI screenshots that'd be awesome.

mifu67 and others added 2 commits September 13, 2023 13:40
@mifu67 @leeandher
Apply suggestions from code review
cdae845 
Co-authored-by: Leander Rodrigues <leander.rodrigues@sentry.io>
@mifu67
copy change
ae99d88
@mifu67
Copy link
Contributor Author

mifu67 commented Sep 13, 2023

This should be merged once someone adds metric alert priorities.

@mifu67 mifu67 marked this pull request as draft September 13, 2023 20:57
leeandher
leeandher approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@leeandher leeandher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested it, and seems to work as expected! We can merge this now and do metric alerts when it gets prioritized.

image
image
image

@mifu67 mifu67 marked this pull request as ready for review September 29, 2023 17:18
@leeandher
Copy link
Member

/gcbrun

@leeandher leeandher added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 29, 2023
@github-actions github-actions bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 29, 2023
@leeandher
Copy link
Member

/gcbrun

@leeandher leeandher added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Sep 29, 2023
@leeandher
Copy link
Member

/gcbrun

@leeandher leeandher merged commit 472bc57 into master Sep 29, 2023
49 of 51 checks passed
@leeandher leeandher deleted the mifu67/alert-rule-priorities branch September 29, 2023 19:43
@Dhrumil-Sentry
Copy link

@vivianyentran Can you please help update the docs for the Opsgenie integration- We can to call out that users can specify the priority with which an Issue Alert should be sent to Opsgenie

@github-actions github-actions bot locked and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@leeandher leeandher leeandher approved these changes

@sentaur-athena sentaur-athena sentaur-athena approved these changes

@nhsiehgit nhsiehgit nhsiehgit approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mifu67 @leeandher @Dhrumil-Sentry @nhsiehgit @sentaur-athena