From e554216a8ab776b389f3ce075bc6dfbdc20c63f6 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 16 Apr 2024 15:51:33 -0700 Subject: [PATCH 1/6] ref(rules): Add typing to rule files --- .../endpoints/project_rules_configuration.py | 2 +- src/sentry/buffer/redis.py | 2 +- src/sentry/rules/base.py | 15 +-- src/sentry/rules/conditions/base.py | 7 + .../rules/conditions/event_frequency.py | 123 +++++++++++------- src/sentry/rules/processing/processor.py | 34 +++-- 6 files changed, 113 insertions(+), 70 deletions(-) diff --git a/src/sentry/api/endpoints/project_rules_configuration.py b/src/sentry/api/endpoints/project_rules_configuration.py index 3519f238fc28ac..3de7e95b5608d1 100644 --- a/src/sentry/api/endpoints/project_rules_configuration.py +++ b/src/sentry/api/endpoints/project_rules_configuration.py @@ -39,7 +39,7 @@ def get(self, request: Request, project) -> Response: # TODO: conditions need to be based on actions for rule_type, rule_cls in rules: - node = rule_cls(project) + node = rule_cls(project=project) # skip over conditions if they are not in the migrated set for a project with alert-filters if project_has_filters and node.id in MIGRATED_CONDITIONS: continue diff --git a/src/sentry/buffer/redis.py b/src/sentry/buffer/redis.py index 7a1c02f9f845b4..05ba9fca6b81be 100644 --- a/src/sentry/buffer/redis.py +++ b/src/sentry/buffer/redis.py @@ -307,7 +307,7 @@ def push_to_hash( def get_hash( self, model: type[models.Model], field: dict[str, models.Model | str | int] - ) -> dict[str, str]: + ) -> list[dict[str, str]]: key = self._make_key(model, field) return self._execute_redis_operation(key, RedisOperation.HASH_GET_ALL) diff --git a/src/sentry/rules/base.py b/src/sentry/rules/base.py index 18fae675c0497c..aae7d45845c362 100644 --- a/src/sentry/rules/base.py +++ b/src/sentry/rules/base.py @@ -3,19 +3,21 @@ import abc import logging from collections import namedtuple -from collections.abc import Callable, Sequence -from typing import Any, ClassVar +from collections.abc import Callable, Mapping, Sequence +from typing import TYPE_CHECKING, Any, ClassVar from django import forms from sentry.eventstore.models import GroupEvent from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.models.rulefirehistory import RuleFireHistory from sentry.snuba.dataset import Dataset from sentry.types.condition_activity import ConditionActivity from sentry.types.rules import RuleFuture +if TYPE_CHECKING: + from sentry.models.rule import Rule + """ Rules apply either before an event gets stored, or immediately after. @@ -60,7 +62,7 @@ class RuleBase(abc.ABC): def __init__( self, project: Project, - data: dict[str, Any] | None = None, + data: Mapping[str, Any] | None = None, rule: Rule | None = None, rule_fire_history: RuleFireHistory | None = None, ) -> None: @@ -81,10 +83,7 @@ def get_option(self, key: str, default: str | None = None) -> str: return self.data.get(key, default) def get_form_instance(self) -> forms.Form: - data: dict[str, Any] | None = None - if self.had_data: - data = self.data - return self.form_cls(data) + return self.form_cls(self.data if self.had_data else None) def render_label(self) -> str: return self.label.format(**self.data) diff --git a/src/sentry/rules/conditions/base.py b/src/sentry/rules/conditions/base.py index 468147d864980d..09d371e6705d01 100644 --- a/src/sentry/rules/conditions/base.py +++ b/src/sentry/rules/conditions/base.py @@ -1,12 +1,19 @@ import abc from collections.abc import Sequence from datetime import datetime +from typing import TypedDict from sentry.eventstore.models import GroupEvent from sentry.rules.base import EventState, RuleBase from sentry.types.condition_activity import ConditionActivity +class GenericCondition(TypedDict): + # the ID in the rules registry that maps to a condition class + # e.g. "sentry.rules.conditions.every_event.EveryEventCondition" + id: str + + class EventCondition(RuleBase, abc.ABC): rule_type = "condition/event" diff --git a/src/sentry/rules/conditions/event_frequency.py b/src/sentry/rules/conditions/event_frequency.py index 958018df2058de..ad540ef380f68d 100644 --- a/src/sentry/rules/conditions/event_frequency.py +++ b/src/sentry/rules/conditions/event_frequency.py @@ -5,12 +5,13 @@ import logging import re from collections import defaultdict -from collections.abc import Callable, Mapping, Sequence +from collections.abc import Callable, Mapping from datetime import datetime, timedelta -from typing import Any +from typing import Any, Literal, NotRequired from django import forms from django.core.cache import cache +from django.db.models.enums import TextChoices from django.utils import timezone from sentry import release_health, tsdb @@ -20,7 +21,7 @@ from sentry.models.group import Group from sentry.receivers.rules import DEFAULT_RULE_LABEL, DEFAULT_RULE_LABEL_NEW from sentry.rules import EventState -from sentry.rules.conditions.base import EventCondition +from sentry.rules.conditions.base import EventCondition, GenericCondition from sentry.tsdb.base import TSDBModel from sentry.types.condition_activity import ( FREQUENCY_CONDITION_BUCKET_SIZE, @@ -31,7 +32,7 @@ from sentry.utils.iterators import chunked from sentry.utils.snuba import options_override -standard_intervals = { +STANDARD_INTERVALS: dict[str, tuple[str, timedelta]] = { "1m": ("one minute", timedelta(minutes=1)), "5m": ("5 minutes", timedelta(minutes=5)), "15m": ("15 minutes", timedelta(minutes=15)), @@ -40,7 +41,7 @@ "1w": ("one week", timedelta(days=7)), "30d": ("30 days", timedelta(days=30)), } -comparison_intervals = { +COMPARISON_INTERVALS: dict[str, tuple[str, timedelta]] = { "5m": ("5 minutes", timedelta(minutes=5)), "15m": ("15 minutes", timedelta(minutes=15)), "1h": ("one hour", timedelta(hours=1)), @@ -48,17 +49,36 @@ "1w": ("one week", timedelta(days=7)), "30d": ("30 days", timedelta(days=30)), } -COMPARISON_TYPE_COUNT = "count" -COMPARISON_TYPE_PERCENT = "percent" -comparison_types = { - COMPARISON_TYPE_COUNT: COMPARISON_TYPE_COUNT, - COMPARISON_TYPE_PERCENT: COMPARISON_TYPE_PERCENT, -} SNUBA_LIMIT = 10000 +class ComparisonType(TextChoices): + COUNT = "count" + PERCENT = "percent" + + +class EventFrequencyConditionData(GenericCondition): + """ + The base typed dict for all condition data representing EventFrequency issue + alert rule conditions + """ + + # Either the count or percentage. + value: int + # The interval to compare the value against such as 5m, 1h, 3w, etc. + # e.g. # of issues is more than {value} in {interval}. + interval: str + # NOTE: Some of the earliest COUNT conditions were created without the + # comparisonType field, although modern rules will always have it. + comparisonType: NotRequired[Literal[ComparisonType.COUNT, ComparisonType.PERCENT]] + # The previous interval to compare the curr interval against. This is only + # present in PERCENT conditions. + # e.g. # of issues is 50% higher in {interval} compared to {comparisonInterval} + comparisonInterval: NotRequired[str] + + class EventFrequencyForm(forms.Form): - intervals = standard_intervals + intervals = STANDARD_INTERVALS interval = forms.ChoiceField( choices=[ (key, label) @@ -69,13 +89,13 @@ class EventFrequencyForm(forms.Form): ) value = forms.IntegerField(widget=forms.TextInput()) comparisonType = forms.ChoiceField( - choices=tuple(comparison_types.items()), + choices=ComparisonType, required=False, ) comparisonInterval = forms.ChoiceField( choices=[ (key, label) - for key, (label, _) in sorted(comparison_intervals.items(), key=lambda item: item[1][1]) + for key, (label, _) in sorted(COMPARISON_INTERVALS.items(), key=lambda item: item[1][1]) ], required=False, ) @@ -88,8 +108,8 @@ def clean(self) -> dict[str, Any] | None: # Don't store an empty string here if the value isn't passed if cleaned_data.get("comparisonInterval") == "": del cleaned_data["comparisonInterval"] - cleaned_data["comparisonType"] = cleaned_data.get("comparisonType") or COMPARISON_TYPE_COUNT - if cleaned_data["comparisonType"] == COMPARISON_TYPE_PERCENT and not cleaned_data.get( + cleaned_data["comparisonType"] = cleaned_data.get("comparisonType") or ComparisonType.COUNT + if cleaned_data["comparisonType"] == ComparisonType.PERCENT and not cleaned_data.get( "comparisonInterval" ): msg = forms.ValidationError("comparisonInterval is required when comparing by percent") @@ -99,10 +119,15 @@ def clean(self) -> dict[str, Any] | None: class BaseEventFrequencyCondition(EventCondition, abc.ABC): - intervals = standard_intervals + intervals = STANDARD_INTERVALS form_cls = EventFrequencyForm - def __init__(self, *args: Any, **kwargs: Any) -> None: + def __init__( + self, + data: EventFrequencyConditionData | None = None, + *args: Any, + **kwargs: Any, + ) -> None: self.tsdb = kwargs.pop("tsdb", tsdb) self.form_fields = { "value": {"type": "number", "placeholder": 100}, @@ -118,7 +143,9 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: }, } - super().__init__(*args, **kwargs) + # MyPy refuses to make TypedDict compatible with MutableMapping + # https://github.com/python/mypy/issues/4976 + super().__init__(data=data, *args, **kwargs) # type:ignore[misc] def _get_options(self) -> tuple[str | None, float | None]: interval, value = None, None @@ -150,19 +177,19 @@ def passes_activity_frequency( if not (interval and value is not None): return False interval_delta = self.intervals[interval][1] - comparison_type = self.get_option("comparisonType", COMPARISON_TYPE_COUNT) + comparison_type = self.get_option("comparisonType", ComparisonType.COUNT) # extrapolate if interval less than bucket size # if comparing percent increase, both intervals will be increased, so do not extrapolate value if interval_delta < FREQUENCY_CONDITION_BUCKET_SIZE: - if comparison_type != COMPARISON_TYPE_PERCENT: + if comparison_type != ComparisonType.PERCENT: value *= int(FREQUENCY_CONDITION_BUCKET_SIZE / interval_delta) interval_delta = FREQUENCY_CONDITION_BUCKET_SIZE result = bucket_count(activity.timestamp - interval_delta, activity.timestamp, buckets) - if comparison_type == COMPARISON_TYPE_PERCENT: - comparison_interval = comparison_intervals[self.get_option("comparisonInterval")][1] + if comparison_type == ComparisonType.PERCENT: + comparison_interval = COMPARISON_INTERVALS[self.get_option("comparisonInterval")][1] comparison_end = activity.timestamp - comparison_interval comparison_result = bucket_count( @@ -175,7 +202,7 @@ def passes_activity_frequency( def get_preview_aggregate(self) -> tuple[str, str]: raise NotImplementedError - def query(self, event: GroupEvent, start: datetime, end: datetime, environment_id: str) -> int: + def query(self, event: GroupEvent, start: datetime, end: datetime, environment_id: int) -> int: """ Queries Snuba for a unique condition for a single group. """ @@ -190,7 +217,7 @@ def query(self, event: GroupEvent, start: datetime, end: datetime, environment_i return query_result def query_hook( - self, event: GroupEvent, start: datetime, end: datetime, environment_id: str + self, event: GroupEvent, start: datetime, end: datetime, environment_id: int ) -> int: """ Abstract method that specifies how to query Snuba for a single group @@ -199,7 +226,7 @@ def query_hook( raise NotImplementedError def batch_query( - self, group_ids: Sequence[int], start: datetime, end: datetime, environment_id: str + self, group_ids: set[int], start: datetime, end: datetime, environment_id: int ) -> dict[int, int]: """ Queries Snuba for a unique condition for multiple groups. @@ -215,7 +242,7 @@ def batch_query( return batch_query_result def batch_query_hook( - self, group_ids: Sequence[int], start: datetime, end: datetime, environment_id: str + self, group_ids: set[int], start: datetime, end: datetime, environment_id: int ) -> dict[int, int]: """ Abstract method that specifies how to query Snuba for multiple groups @@ -223,7 +250,7 @@ def batch_query_hook( """ raise NotImplementedError - def get_rate(self, event: GroupEvent, interval: str, environment_id: str) -> int: + def get_rate(self, event: GroupEvent, interval: str, environment_id: int) -> int: _, duration = self.intervals[interval] end = timezone.now() # For conditions with interval >= 1 hour we don't need to worry about read your writes @@ -233,9 +260,9 @@ def get_rate(self, event: GroupEvent, interval: str, environment_id: str) -> int option_override_cm = options_override({"consistent": False}) with option_override_cm: result: int = self.query(event, end - duration, end, environment_id=environment_id) - comparison_type = self.get_option("comparisonType", COMPARISON_TYPE_COUNT) - if comparison_type == COMPARISON_TYPE_PERCENT: - comparison_interval = comparison_intervals[self.get_option("comparisonInterval")][1] + comparison_type = self.get_option("comparisonType", ComparisonType.COUNT) + if comparison_type == ComparisonType.PERCENT: + comparison_interval = COMPARISON_INTERVALS[self.get_option("comparisonInterval")][1] comparison_end = end - comparison_interval # TODO: Figure out if there's a way we can do this less frequently. All queries are # automatically cached for 10s. We could consider trying to cache this and the main @@ -255,7 +282,7 @@ def get_snuba_query_result( model: TSDBModel, start: datetime, end: datetime, - environment_id: str, + environment_id: int, referrer_suffix: str, ) -> Mapping[int, int]: result: Mapping[int, int] = tsdb_function( @@ -278,7 +305,7 @@ def get_chunked_result( groups: list[Group], start: datetime, end: datetime, - environment_id: str, + environment_id: int, referrer_suffix: str, ) -> dict[int, int]: batch_totals: dict[int, int] = defaultdict(int) @@ -319,7 +346,7 @@ class EventFrequencyCondition(BaseEventFrequencyCondition): label = "The issue is seen more than {value} times in {interval}" def query_hook( - self, event: GroupEvent, start: datetime, end: datetime, environment_id: str + self, event: GroupEvent, start: datetime, end: datetime, environment_id: int ) -> int: sums: Mapping[int, int] = self.get_snuba_query_result( tsdb_function=self.tsdb.get_sums, @@ -334,7 +361,7 @@ def query_hook( return sums[event.group_id] def batch_query_hook( - self, group_ids: Sequence[int], start: datetime, end: datetime, environment_id: str + self, group_ids: set[int], start: datetime, end: datetime, environment_id: int ) -> dict[int, int]: batch_sums: dict[int, int] = defaultdict(int) groups = Group.objects.filter(id__in=group_ids) @@ -376,7 +403,7 @@ class EventUniqueUserFrequencyCondition(BaseEventFrequencyCondition): label = "The issue is seen by more than {value} users in {interval}" def query_hook( - self, event: GroupEvent, start: datetime, end: datetime, environment_id: str + self, event: GroupEvent, start: datetime, end: datetime, environment_id: int ) -> int: totals: Mapping[int, int] = self.get_snuba_query_result( tsdb_function=self.tsdb.get_distinct_counts_totals, @@ -391,7 +418,7 @@ def query_hook( return totals[event.group_id] def batch_query_hook( - self, group_ids: Sequence[int], start: datetime, end: datetime, environment_id: str + self, group_ids: set[int], start: datetime, end: datetime, environment_id: int ) -> dict[int, int]: batch_totals: dict[int, int] = defaultdict(int) groups = Group.objects.filter(id__in=group_ids) @@ -428,7 +455,7 @@ def get_preview_aggregate(self) -> tuple[str, str]: return "uniq", "user" -percent_intervals = { +PERCENT_INTERVALS: dict[str, tuple[str, timedelta]] = { "1m": ("1 minute", timedelta(minutes=1)), "5m": ("5 minutes", timedelta(minutes=5)), "10m": ("10 minutes", timedelta(minutes=10)), @@ -436,7 +463,7 @@ def get_preview_aggregate(self) -> tuple[str, str]: "1h": ("1 hour", timedelta(minutes=60)), } -percent_intervals_to_display = { +PERCENT_INTERVALS_TO_DISPLAY: dict[str, tuple[str, timedelta]] = { "5m": ("5 minutes", timedelta(minutes=5)), "10m": ("10 minutes", timedelta(minutes=10)), "30m": ("30 minutes", timedelta(minutes=30)), @@ -446,12 +473,12 @@ def get_preview_aggregate(self) -> tuple[str, str]: class EventFrequencyPercentForm(EventFrequencyForm): - intervals = percent_intervals_to_display + intervals = PERCENT_INTERVALS_TO_DISPLAY interval = forms.ChoiceField( choices=[ (key, label) for key, (label, duration) in sorted( - percent_intervals_to_display.items(), + PERCENT_INTERVALS_TO_DISPLAY.items(), key=lambda key____label__duration: key____label__duration[1][1], ) ] @@ -462,7 +489,7 @@ def clean(self) -> dict[str, Any] | None: cleaned_data = super().clean() if ( cleaned_data - and cleaned_data["comparisonType"] == COMPARISON_TYPE_COUNT + and cleaned_data["comparisonType"] == ComparisonType.COUNT and cleaned_data.get("value", 0) > 100 ): self.add_error( @@ -479,7 +506,7 @@ class EventFrequencyPercentCondition(BaseEventFrequencyCondition): logger = logging.getLogger("sentry.rules.event_frequency") def __init__(self, *args: Any, **kwargs: Any) -> None: - self.intervals = percent_intervals + self.intervals = PERCENT_INTERVALS self.form_cls = EventFrequencyPercentForm super().__init__(*args, **kwargs) @@ -490,14 +517,14 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: "choices": [ (key, label) for key, (label, duration) in sorted( - percent_intervals_to_display.items(), + PERCENT_INTERVALS_TO_DISPLAY.items(), key=lambda key____label__duration: key____label__duration[1][1], ) ], } def get_session_count( - self, project_id: int, environment_id: str, start: datetime, end: datetime + self, project_id: int, environment_id: int, start: datetime, end: datetime ) -> int: cache_key = f"r.c.spc:{project_id}-{environment_id}" session_count_last_hour = cache.get(cache_key) @@ -516,12 +543,12 @@ def get_session_count( def get_session_interval(self, session_count: int, interval: str) -> int | None: if session_count >= MIN_SESSIONS_TO_FIRE: - interval_in_minutes = percent_intervals[interval][1].total_seconds() // 60 + interval_in_minutes = PERCENT_INTERVALS[interval][1].total_seconds() // 60 return int(session_count / (60 / interval_in_minutes)) return None def query_hook( - self, event: GroupEvent, start: datetime, end: datetime, environment_id: str + self, event: GroupEvent, start: datetime, end: datetime, environment_id: int ) -> int: project_id = event.project_id session_count_last_hour = self.get_session_count(project_id, environment_id, start, end) @@ -556,7 +583,7 @@ def query_hook( return 0 def batch_query_hook( - self, group_ids: Sequence[int], start: datetime, end: datetime, environment_id: str + self, group_ids: set[int], start: datetime, end: datetime, environment_id: int ) -> dict[int, int]: batch_percents: dict[int, int] = defaultdict(int) groups = Group.objects.filter(id__in=group_ids) diff --git a/src/sentry/rules/processing/processor.py b/src/sentry/rules/processing/processor.py index 3d7a0e60b40925..84596802bcb01a 100644 --- a/src/sentry/rules/processing/processor.py +++ b/src/sentry/rules/processing/processor.py @@ -25,6 +25,8 @@ from sentry.utils.hashlib import hash_values from sentry.utils.safe import safe_execute +logger = logging.getLogger("sentry.rules") + SLOW_CONDITION_MATCHES = ["event_frequency"] @@ -38,7 +40,13 @@ def get_match_function(match_name: str) -> Callable[..., bool] | None: return None -def is_condition_slow(condition: Mapping[str, str]) -> bool: +def is_condition_slow( + condition: Mapping[str, Any], +) -> bool: + """ + Returns whether a condition is considered slow. Note that the slow condition + mapping take the form on EventFrequencyConditionData. + """ for slow_conditions in SLOW_CONDITION_MATCHES: if slow_conditions in condition["id"]: return True @@ -46,8 +54,6 @@ def is_condition_slow(condition: Mapping[str, str]) -> bool: class RuleProcessor: - logger = logging.getLogger("sentry.rules") - def __init__( self, event: GroupEvent, @@ -126,7 +132,7 @@ def bulk_get_rule_status(self, rules: Sequence[Rule]) -> Mapping[int, GroupRuleS if missing_rule_ids: # Shouldn't happen, but log just in case - self.logger.error( + logger.error( "Failed to fetch some GroupRuleStatuses in RuleProcessor", extra={"missing_rule_ids": missing_rule_ids, "group_id": self.group.id}, ) @@ -138,16 +144,19 @@ def bulk_get_rule_status(self, rules: Sequence[Rule]) -> Mapping[int, GroupRuleS return rule_statuses def condition_matches( - self, condition: dict[str, Any], state: EventState, rule: Rule + self, + condition: Mapping[str, Any], + state: EventState, + rule: Rule, ) -> bool | None: condition_cls = rules.get(condition["id"]) if condition_cls is None: - self.logger.warning("Unregistered condition %r", condition["id"]) + logger.warning("Unregistered condition %r", condition["id"]) return None - condition_inst = condition_cls(self.project, data=condition, rule=rule) + condition_inst = condition_cls(project=self.project, data=condition, rule=rule) if not isinstance(condition_inst, (EventCondition, EventFilter)): - self.logger.warning("Unregistered condition %r", condition["id"]) + logger.warning("Unregistered condition %r", condition["id"]) return None passes: bool = safe_execute( condition_inst.passes, @@ -160,7 +169,7 @@ def condition_matches( def get_rule_type(self, condition: Mapping[str, Any]) -> str | None: rule_cls = rules.get(condition["id"]) if rule_cls is None: - self.logger.warning("Unregistered condition or filter %r", condition["id"]) + logger.warning("Unregistered condition or filter %r", condition["id"]) return None rule_type: str = rule_cls.rule_type @@ -236,8 +245,9 @@ def apply_rule(self, rule: Rule, status: GroupRuleStatus) -> None: if not predicate_func(predicate_iter): return else: - self.logger.error( - f"Unsupported {name}_match {match!r} for rule {rule.id}", + log_string = f"Unsupported {name}_match {match!r} for rule {rule.id}" + logger.error( + log_string, filter_match, rule.id, extra={**logging_details}, @@ -286,7 +296,7 @@ def activate_downstream_actions( notification_uuid=notification_uuid, ) if results is None: - self.logger.warning("Action %s did not return any futures", action["id"]) + logger.warning("Action %s did not return any futures", action["id"]) continue for future in results: From 0b138a7cd0ebc05aa6869404587a38a290f822b0 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 16 Apr 2024 16:16:42 -0700 Subject: [PATCH 2/6] use mutablemapping --- src/sentry/rules/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/rules/base.py b/src/sentry/rules/base.py index aae7d45845c362..a33244dc992f9d 100644 --- a/src/sentry/rules/base.py +++ b/src/sentry/rules/base.py @@ -3,7 +3,7 @@ import abc import logging from collections import namedtuple -from collections.abc import Callable, Mapping, Sequence +from collections.abc import Callable, MutableMapping, Sequence from typing import TYPE_CHECKING, Any, ClassVar from django import forms @@ -62,7 +62,7 @@ class RuleBase(abc.ABC): def __init__( self, project: Project, - data: Mapping[str, Any] | None = None, + data: MutableMapping[str, Any] | None = None, rule: Rule | None = None, rule_fire_history: RuleFireHistory | None = None, ) -> None: From b288ed86362d88707d148f5d5d6de38c15a05dc2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 16 Apr 2024 16:37:49 -0700 Subject: [PATCH 3/6] update another mutablemapping --- src/sentry/rules/processing/processor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/rules/processing/processor.py b/src/sentry/rules/processing/processor.py index 84596802bcb01a..73a3a871627944 100644 --- a/src/sentry/rules/processing/processor.py +++ b/src/sentry/rules/processing/processor.py @@ -145,7 +145,7 @@ def bulk_get_rule_status(self, rules: Sequence[Rule]) -> Mapping[int, GroupRuleS def condition_matches( self, - condition: Mapping[str, Any], + condition: MutableMapping[str, Any], state: EventState, rule: Rule, ) -> bool | None: From a8139b7b65dd76dea1e957da7321bd0cc6ec9a3e Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 16 Apr 2024 17:27:13 -0700 Subject: [PATCH 4/6] sadly cant get this to work --- src/sentry/rules/conditions/base.py | 7 ----- .../rules/conditions/event_frequency.py | 28 +++---------------- 2 files changed, 4 insertions(+), 31 deletions(-) diff --git a/src/sentry/rules/conditions/base.py b/src/sentry/rules/conditions/base.py index 09d371e6705d01..468147d864980d 100644 --- a/src/sentry/rules/conditions/base.py +++ b/src/sentry/rules/conditions/base.py @@ -1,19 +1,12 @@ import abc from collections.abc import Sequence from datetime import datetime -from typing import TypedDict from sentry.eventstore.models import GroupEvent from sentry.rules.base import EventState, RuleBase from sentry.types.condition_activity import ConditionActivity -class GenericCondition(TypedDict): - # the ID in the rules registry that maps to a condition class - # e.g. "sentry.rules.conditions.every_event.EveryEventCondition" - id: str - - class EventCondition(RuleBase, abc.ABC): rule_type = "condition/event" diff --git a/src/sentry/rules/conditions/event_frequency.py b/src/sentry/rules/conditions/event_frequency.py index ad540ef380f68d..445b2a57d39f40 100644 --- a/src/sentry/rules/conditions/event_frequency.py +++ b/src/sentry/rules/conditions/event_frequency.py @@ -5,9 +5,9 @@ import logging import re from collections import defaultdict -from collections.abc import Callable, Mapping +from collections.abc import Callable, Mapping, MutableMapping from datetime import datetime, timedelta -from typing import Any, Literal, NotRequired +from typing import Any from django import forms from django.core.cache import cache @@ -21,7 +21,7 @@ from sentry.models.group import Group from sentry.receivers.rules import DEFAULT_RULE_LABEL, DEFAULT_RULE_LABEL_NEW from sentry.rules import EventState -from sentry.rules.conditions.base import EventCondition, GenericCondition +from sentry.rules.conditions.base import EventCondition from sentry.tsdb.base import TSDBModel from sentry.types.condition_activity import ( FREQUENCY_CONDITION_BUCKET_SIZE, @@ -57,26 +57,6 @@ class ComparisonType(TextChoices): PERCENT = "percent" -class EventFrequencyConditionData(GenericCondition): - """ - The base typed dict for all condition data representing EventFrequency issue - alert rule conditions - """ - - # Either the count or percentage. - value: int - # The interval to compare the value against such as 5m, 1h, 3w, etc. - # e.g. # of issues is more than {value} in {interval}. - interval: str - # NOTE: Some of the earliest COUNT conditions were created without the - # comparisonType field, although modern rules will always have it. - comparisonType: NotRequired[Literal[ComparisonType.COUNT, ComparisonType.PERCENT]] - # The previous interval to compare the curr interval against. This is only - # present in PERCENT conditions. - # e.g. # of issues is 50% higher in {interval} compared to {comparisonInterval} - comparisonInterval: NotRequired[str] - - class EventFrequencyForm(forms.Form): intervals = STANDARD_INTERVALS interval = forms.ChoiceField( @@ -124,7 +104,7 @@ class BaseEventFrequencyCondition(EventCondition, abc.ABC): def __init__( self, - data: EventFrequencyConditionData | None = None, + data: MutableMapping | None = None, *args: Any, **kwargs: Any, ) -> None: From 17827de527091c0abf705e52b23e096bbae90c09 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 17 Apr 2024 10:27:45 -0700 Subject: [PATCH 5/6] fix test, don't pass data --- src/sentry/rules/conditions/event_frequency.py | 7 ++----- tests/sentry/rules/processing/test_processor.py | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/sentry/rules/conditions/event_frequency.py b/src/sentry/rules/conditions/event_frequency.py index 445b2a57d39f40..b1cdb697557566 100644 --- a/src/sentry/rules/conditions/event_frequency.py +++ b/src/sentry/rules/conditions/event_frequency.py @@ -5,7 +5,7 @@ import logging import re from collections import defaultdict -from collections.abc import Callable, Mapping, MutableMapping +from collections.abc import Callable, Mapping from datetime import datetime, timedelta from typing import Any @@ -104,7 +104,6 @@ class BaseEventFrequencyCondition(EventCondition, abc.ABC): def __init__( self, - data: MutableMapping | None = None, *args: Any, **kwargs: Any, ) -> None: @@ -123,9 +122,7 @@ def __init__( }, } - # MyPy refuses to make TypedDict compatible with MutableMapping - # https://github.com/python/mypy/issues/4976 - super().__init__(data=data, *args, **kwargs) # type:ignore[misc] + super().__init__(*args, **kwargs) # type:ignore[misc] def _get_options(self) -> tuple[str | None, float | None]: interval, value = None, None diff --git a/tests/sentry/rules/processing/test_processor.py b/tests/sentry/rules/processing/test_processor.py index 46c2177b563f59..5334dbbc071d35 100644 --- a/tests/sentry/rules/processing/test_processor.py +++ b/tests/sentry/rules/processing/test_processor.py @@ -560,7 +560,7 @@ def test_last_active_too_recent(self): results = list(rp.apply()) assert len(results) == 0 - @mock.patch("sentry.rules.processing.processor.RuleProcessor.logger") + @mock.patch("sentry.rules.processing.processor.logger") def test_invalid_predicate(self, mock_logger): filter_data = {"id": "tests.sentry.rules.processing.test_processor.MockFilterTrue"} From 9ae758a32511d1d443c62c2710669ae4d98a69e3 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 17 Apr 2024 10:37:42 -0700 Subject: [PATCH 6/6] rm ignore --- src/sentry/rules/conditions/event_frequency.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/rules/conditions/event_frequency.py b/src/sentry/rules/conditions/event_frequency.py index b1cdb697557566..dc292456fffb5c 100644 --- a/src/sentry/rules/conditions/event_frequency.py +++ b/src/sentry/rules/conditions/event_frequency.py @@ -122,7 +122,7 @@ def __init__( }, } - super().__init__(*args, **kwargs) # type:ignore[misc] + super().__init__(*args, **kwargs) def _get_options(self) -> tuple[str | None, float | None]: interval, value = None, None