-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(rules): Send a notification on create and edit #66285
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
Conversation
src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #66285 +/- ##
=======================================
Coverage 84.30% 84.30%
=======================================
Files 5311 5312 +1
Lines 237258 237329 +71
Branches 41035 41041 +6
=======================================
+ Hits 200014 200083 +69
- Misses 37025 37027 +2
Partials 219 219
|
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.
This approach looks good to me 👍
else: | ||
rule_text = f"{rule_url} in the *{project}* project was updated." | ||
|
||
# TODO add short summary of the trigger & filter conditions |
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.
This is a "maybe" TODO, discussing in Slack whether we want to add stuff to it or keep it simple. Simple is fine for this PR.
if not isinstance(action_inst, EventAction): | ||
self.logger.warning("Unregistered action %r", action["id"]) | ||
continue | ||
action_inst = instantiate_action(rule, action) |
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.
This I just moved into a shared function so I can use it with my new logic.
cdba74c
to
24d0a1b
Compare
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 think mostly looks good?
some non-blocking nits
rule: Rule, | ||
new: bool, | ||
) -> None: | ||
super().__init__() |
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.
any other kwargs to pass through to super init?
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.
Doesn't seem like it
from sentry.rules.base import CallbackFuture, EventState, RuleBase | ||
|
||
logger = logging.getLogger("sentry.rules") | ||
|
||
|
||
def instantiate_action(rule: Rule, action): |
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.
did we decide we watned this here in the base.py
file? Or create a separate helpers/utils file?
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 think we were only discussing moving it around because of circular imports and ended up not needing to address that.
from sentry.rules.base import CallbackFuture, EventState, RuleBase | ||
|
||
logger = logging.getLogger("sentry.rules") | ||
|
||
|
||
def instantiate_action(rule: Rule, action): | ||
from sentry.rules import rules |
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.
moving to separate helper file should avoid circular imports right?
Follow up to #66285 to add details about what changed happened when a rule was edited to the notification. <img width="748" alt="Screenshot 2024-03-12 at 4 40 24 PM" src="https://github.com/getsentry/sentry/assets/29959063/e0d7c5ca-35e5-4a66-bc67-e53c8d27dcce"> Note that I've made a couple follow up tickets to address poor rendering of issue type enums and frequency: getsentry/team-core-product-foundations#158 and getsentry/team-core-product-foundations#157 --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Follow up to #66285 to add details about what changed happened when a rule was edited to the notification. <img width="748" alt="Screenshot 2024-03-12 at 4 40 24 PM" src="https://github.com/getsentry/sentry/assets/29959063/e0d7c5ca-35e5-4a66-bc67-e53c8d27dcce"> Note that I've made a couple follow up tickets to address poor rendering of issue type enums and frequency: getsentry/team-core-product-foundations#158 and getsentry/team-core-product-foundations#157 --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Send a Slack notification when a rule with a Slack action is created or edited that confirms the rule was created or edited. In the future we'll add this for MSTeams and Discord.
I'm keeping the notification simple for now, but in the future we also may expand what's shown (e.g. adding the alert rule details to show the filters, conditions, and actions).
Closes #66237