-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
fix(slack): add filtering for users affected by env if the rule has an env #80451
base: master
Are you sure you want to change the base?
Conversation
@@ -198,7 +206,7 @@ def get_tags( | |||
return fields | |||
|
|||
|
|||
def get_context(group: Group) -> str: | |||
def get_context(group: Group, rules: list[Rule] | None = None) -> str: |
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 logic is messy, but we remove Users Affected
if possible because it requires a Snuba query
if c == NotificationContextField.USERS_AFFECTED: | ||
v = SUPPORTED_CONTEXT_DATA[c](group, 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.
this is also messy and i'm not sure if there's a better way to do this without making every other function in SUPPORTED_CONTEXT_DATA
take in an unused rules
argument
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #80451 +/- ##
==========================================
- Coverage 78.33% 78.33% -0.01%
==========================================
Files 7206 7206
Lines 318682 318693 +11
Branches 43929 43932 +3
==========================================
- Hits 249641 249634 -7
- Misses 62677 62690 +13
- Partials 6364 6369 +5 |
Currently, we always fetch
Users Affected
across all environments, even if the rule has a non-nullenvironment_id
, which means the rule fires only if the event happened in a specific environment.There is already a function in which we can pass
environment_ids
to Snuba to get the count of users affected for specific envs, but we've been passingNone
by default. We can change this to pass theenvironment_id
of the rule(s) that fired the issue alert (this is somehow an array).I also added an enum for the different kinds of context fields that can appear in a Slack notification (
NotificationContextField
), so we can use that instead of strings.