feat(aci): event frequency condition handler #82551

cathteng · 2024-12-23T23:47:44Z

Add base event frequency condition handler, event frequency count and event frequency percent condition handler.

Each existing condition class technically handles two distinct condition types: getting the count for an interval, and comparing percent increase of the count for an interval with the same interval X time in the past.

The base condition handler borrows elements from BaseEventFrequencyCondition related to making the bulk Snuba query -- it might not yet contain everything necessary for delayed processing, but is enough for tests.

codecov · 2024-12-24T00:36:22Z

Codecov Report

Attention: Patch coverage is 92.15686% with 16 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...handlers/condition/event_frequency_base_handler.py	80.64%	12 Missing ⚠️
...ine/handlers/condition/event_frequency_handlers.py	92.68%	3 Missing ⚠️
...rc/sentry/workflow_engine/models/data_condition.py	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #82551      +/-   ##
==========================================
- Coverage   87.52%   87.49%   -0.03%     
==========================================
  Files        9488     9411      -77     
  Lines      537867   537366     -501     
  Branches    21174    21174              
==========================================
- Hits       470750   470179     -571     
- Misses      66760    66830      +70     
  Partials      357      357

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

cathteng · 2024-12-26T19:18:17Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+    def batch_query(
+        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).values(
+            "id", "type", "project_id", "project__organization_id"
+        )
+        error_issue_ids, generic_issue_ids = self.get_error_and_generic_group_ids(groups)
+        organization_id = self.get_value_from_groups(groups, "project__organization_id")
+
+        if error_issue_ids and organization_id:
+            error_sums = self.get_chunked_result(
+                tsdb_function=tsdb.backend.get_sums,
+                model=get_issue_tsdb_group_model(GroupCategory.ERROR),
+                group_ids=error_issue_ids,
+                organization_id=organization_id,
+                start=start,
+                end=end,
+                environment_id=environment_id,
+                referrer_suffix="batch_alert_event_frequency",
+            )
+            batch_sums.update(error_sums)
+
+        if generic_issue_ids and organization_id:
+            generic_sums = self.get_chunked_result(
+                tsdb_function=tsdb.backend.get_sums,
+                # this isn't necessarily performance, just any non-error category
+                model=get_issue_tsdb_group_model(GroupCategory.PERFORMANCE),
+                group_ids=generic_issue_ids,
+                organization_id=organization_id,
+                start=start,
+                end=end,
+                environment_id=environment_id,
+                referrer_suffix="batch_alert_event_frequency",
+            )
+            batch_sums.update(generic_sums)
+
+        return batch_sums


copied from

sentry/src/sentry/rules/conditions/event_frequency.py

Line 413 in 45e358f

class EventFrequencyCondition(BaseEventFrequencyCondition):

🤔 - i wonder if we should have these defined with the DataSources? I'm trying to think about how this module will grow and change over time.

I think it makes sense to have it here, but as i think about what would we need to change if we do Snuba Query -> EAP, we'd have to make a lot of changes to the workflow engine to support that here. If we had this setup by type, we could move the code into the area and hook in here with a registry.

I'm not sold on ^ as a design, but i'm a little worried about how many condition handlers we'll have here, and how tightly coupled they'll be to the workflow engine.

discussed offline, we should probably discuss how these conditions will look for the different product verticals, especially for those where a single occurrence != there was an issue at X time (crons, metric issues, uptime) 🤔

although i will say it does seem like all kinds of issues go through this logic today.

i generalized it more to account for each kind of GroupCategory

https://github.com/getsentry/sentry/pull/82551/files#diff-2e86091b040ad68bbaef3b93a4a700e8e52dcb01f7d3070e33a0870418520c8bR109-R123

src/sentry/workflow_engine/models/data_condition.py

cathteng · 2025-01-02T19:24:23Z

tests/sentry/workflow_engine/handlers/condition/test_event_frequency_handlers.py

+        assert dc.condition_group == dcg
+
+
+class EventFrequencyQueryTest(EventFrequencyQueryTestBase):


this is the same as

sentry/tests/snuba/rules/conditions/test_event_frequency.py

Line 133 in e85b3f9

class EventFrequencyQueryTest(EventFrequencyQueryTestBase):

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

src/sentry/workflow_engine/models/data_condition.py

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

saponifi3d · 2025-01-06T19:18:04Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+    def batch_query(
+        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).values(
+            "id", "type", "project_id", "project__organization_id"
+        )
+        error_issue_ids, generic_issue_ids = self.get_error_and_generic_group_ids(groups)
+        organization_id = self.get_value_from_groups(groups, "project__organization_id")
+
+        if error_issue_ids and organization_id:
+            error_sums = self.get_chunked_result(
+                tsdb_function=tsdb.backend.get_sums,
+                model=get_issue_tsdb_group_model(GroupCategory.ERROR),
+                group_ids=error_issue_ids,
+                organization_id=organization_id,
+                start=start,
+                end=end,
+                environment_id=environment_id,
+                referrer_suffix="batch_alert_event_frequency",
+            )
+            batch_sums.update(error_sums)
+
+        if generic_issue_ids and organization_id:
+            generic_sums = self.get_chunked_result(
+                tsdb_function=tsdb.backend.get_sums,
+                # this isn't necessarily performance, just any non-error category
+                model=get_issue_tsdb_group_model(GroupCategory.PERFORMANCE),
+                group_ids=generic_issue_ids,
+                organization_id=organization_id,
+                start=start,
+                end=end,
+                environment_id=environment_id,
+                referrer_suffix="batch_alert_event_frequency",
+            )
+            batch_sums.update(generic_sums)
+
+        return batch_sums


🤔 - i wonder if we should have these defined with the DataSources? I'm trying to think about how this module will grow and change over time.

I think it makes sense to have it here, but as i think about what would we need to change if we do Snuba Query -> EAP, we'd have to make a lot of changes to the workflow engine to support that here. If we had this setup by type, we could move the code into the area and hook in here with a registry.

I'm not sold on ^ as a design, but i'm a little worried about how many condition handlers we'll have here, and how tightly coupled they'll be to the workflow engine.

tests/sentry/workflow_engine/handlers/condition/test_event_frequency_handlers.py

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

saponifi3d · 2025-01-07T22:55:53Z

src/sentry/workflow_engine/models/data_condition.py

+    EVENT_FREQUENCY = "event_frequency"
+    EVENT_UNIQUE_USER_FREQUENCY = "event_unique_user_frequency"
+    EVENT_FREQUENCY_PERCENT = "event_frequency_percent"
+    EVENT_UNIQUE_USER_FREQUENCY_WITH_CONDITIONS = "event_unique_user_frequency_with_conditions"


🤔 do we need different condition handlers for each of these to access the specific data to compare against? Are there more to come for event frequency? How would we know which one of these conditions to use in the UI from a single UI component?

these are what the frequency conditions can look like in the UI today... today we do some funky stuff to get it to look like this
https://github.com/getsentry/sentry/blob/0d73e5379eca69ae2da588836732ef1add3aee91/static/app/views/alerts/utils/constants.tsx#L1-L35

but there are actually only these 4 conditions/handlers for event frequency conditions

after looking at this more i realized each condition class basically handles 2 conditions: event count and percent frequency. i added accounting for this in a single condition class because we are able to share some queries in the percent case with event count in delayed processing (percent has 2 queries, 1 of which can overlap with even count)

src/sentry/workflow_engine/models/data_condition.py

tests/sentry/workflow_engine/handlers/condition/test_base.py

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

ceorourke · 2025-01-10T01:23:43Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+):
+    @staticmethod
+    def evaluate_value(value: list[int], comparison: Any) -> DataConditionResult:
+        comparison_type = comparison["comparison_type"]


can you type comparison as a dict?

🤔 we should probably add a json schema to the comparison field? it'd be nice if we could do this in the base frequency class to ensure these new conditions are correct.

because this is so generic, comparison can be anything that reasonably splats into models.JSONField...

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

cathteng · 2025-01-13T21:19:18Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+@condition_handler_registry.register(Condition.EVENT_FREQUENCY_COUNT)
+class EventFrequencyCountHandler(EventFrequencyConditionHandler, DataConditionHandler[int]):
+    @staticmethod
+    def evaluate_value(value: int, comparison: Any) -> DataConditionResult:
+        return value > comparison["value"]
+
+
+@condition_handler_registry.register(Condition.EVENT_FREQUENCY_PERCENT)
+class EventFrequencyPercentHandler(EventFrequencyConditionHandler, DataConditionHandler[list[int]]):
+    @staticmethod
+    def evaluate_value(value: list[int], comparison: Any) -> DataConditionResult:
+        if len(value) != 2:
+            return False
+        return percent_increase(value[0], value[1]) > comparison["value"]


this is the main part

@saponifi3d i think i need to add some validation on all DataCondition.comparison fields, will think about it in an upcoming PR

saponifi3d

overall, lgtm, it'd be nice if we address the comparison schema comment before merging though.

saponifi3d · 2025-01-11T00:11:13Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+):
+    @staticmethod
+    def evaluate_value(value: list[int], comparison: Any) -> DataConditionResult:
+        comparison_type = comparison["comparison_type"]


🤔 we should probably add a json schema to the comparison field? it'd be nice if we could do this in the base frequency class to ensure these new conditions are correct.

saponifi3d · 2025-01-14T20:16:15Z

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py

+from sentry.workflow_engine.types import DataConditionHandler, DataConditionResult
+
+
+class EventFrequencyConditionHandler(BaseEventFrequencyConditionHandler):


nit: if we're going to have all these classes in 1 file, might as well include the base here too (otherwise, i'd split each of these classes as a separate file, but that's a bit JS-y too)

there are 4 existing condition classes that are related but represent 2 conditions (count and percent change), and i am grouping together these 2 very similar conditions

saponifi3d · 2025-01-14T20:33:00Z

tests/sentry/workflow_engine/handlers/condition/test_event_frequency_handlers.py

+        self.assert_slow_cond_passes(dc, 1001)
+        self.assert_slow_cond_does_not_pass(dc, 999)


Suggested change

self.assert_slow_cond_passes(dc, 1001)

self.assert_slow_cond_does_not_pass(dc, 999)

self.assert_slow_cond_passes(dc, [1001])

self.assert_slow_cond_does_not_pass(dc, [999])

^ should fix the mypy error.

…handlers.py

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 23, 2024

vercel bot deployed to Preview December 23, 2024 23:50 View deployment

vercel bot deployed to Preview December 24, 2024 00:12 View deployment

cathteng commented Dec 26, 2024

View reviewed changes

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

cathteng commented Dec 26, 2024

View reviewed changes

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

cathteng commented Dec 26, 2024

View reviewed changes

cathteng marked this pull request as ready for review December 26, 2024 19:55

cathteng requested a review from a team December 26, 2024 19:55

cathteng commented Dec 31, 2024

View reviewed changes

src/sentry/workflow_engine/models/data_condition.py Outdated Show resolved Hide resolved

cathteng commented Jan 2, 2025

View reviewed changes

cathteng marked this pull request as draft January 2, 2025 20:00

cathteng commented Jan 2, 2025

View reviewed changes

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

cathteng force-pushed the cathy/aci/event-frequency-handler branch from 888db48 to eb54786 Compare January 3, 2025 19:45

cathteng marked this pull request as ready for review January 3, 2025 19:45

vercel bot deployed to Preview January 3, 2025 19:50 View deployment

cathteng requested a review from ceorourke January 6, 2025 17:24

saponifi3d reviewed Jan 6, 2025

View reviewed changes

ceorourke reviewed Jan 6, 2025

View reviewed changes

tests/sentry/workflow_engine/handlers/condition/test_event_frequency_handlers.py Outdated Show resolved Hide resolved

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview January 6, 2025 20:03 View deployment

cathteng force-pushed the cathy/aci/event-frequency-handler branch from 0a6dbc1 to 21456b7 Compare January 6, 2025 22:43

vercel bot deployed to Preview January 6, 2025 22:46 View deployment

vercel bot deployed to Preview January 6, 2025 23:09 View deployment

cathteng requested a review from a team January 6, 2025 23:51

vercel bot deployed to Preview January 6, 2025 23:57 View deployment

saponifi3d reviewed Jan 8, 2025

View reviewed changes

cathteng force-pushed the cathy/aci/event-frequency-handler branch from 878b423 to 5a74923 Compare January 9, 2025 18:27

vercel bot deployed to Preview January 9, 2025 18:31 View deployment

vercel bot deployed to Preview January 9, 2025 21:34 View deployment

vercel bot deployed to Preview January 9, 2025 23:28 View deployment

cathteng requested review from a team, saponifi3d and ceorourke January 9, 2025 23:44

ceorourke reviewed Jan 10, 2025

View reviewed changes

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

ceorourke reviewed Jan 10, 2025

View reviewed changes

cathteng force-pushed the cathy/aci/event-frequency-handler branch from 990504d to 8d75328 Compare January 13, 2025 21:14

cathteng commented Jan 13, 2025

View reviewed changes

src/sentry/workflow_engine/handlers/condition/event_frequency_handlers.py Outdated Show resolved Hide resolved

cathteng commented Jan 13, 2025

View reviewed changes

cathteng requested a review from a team January 13, 2025 21:19

vercel bot deployed to Preview January 13, 2025 21:23 View deployment

saponifi3d approved these changes Jan 14, 2025

View reviewed changes

cathteng and others added 11 commits January 15, 2025 11:57

event frequency handler

7ea9306

fix typing

109dd63

small refactor

6981495

generalize fetching snuba data by GroupCategory

657876b

reorganize inheritance

85fc470

nit fix id

7ccb9d8

account for percent condition

30644e8

add special asserts with appropriate types for slow conditions

9fcaf0e

enumerate all frequency conditions -- count and percent

f38943b

Update src/sentry/workflow_engine/handlers/condition/event_frequency_…

1005798

…handlers.py

add json schema for event frequency

e7504cf

cathteng force-pushed the cathy/aci/event-frequency-handler branch from ffa1ca5 to e7504cf Compare January 15, 2025 21:41

cathteng enabled auto-merge (squash) January 15, 2025 21:42

vercel bot deployed to Preview January 15, 2025 21:45 View deployment

cathteng merged commit 8cff0dc into master Jan 15, 2025
48 checks passed

cathteng deleted the cathy/aci/event-frequency-handler branch January 15, 2025 22:08

andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025

feat(aci): event frequency condition handler (#82551)

f4b12ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aci): event frequency condition handler #82551

feat(aci): event frequency condition handler #82551

cathteng commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

cathteng Dec 26, 2024

saponifi3d Jan 6, 2025

cathteng Jan 6, 2025

cathteng Jan 6, 2025

cathteng Jan 2, 2025

saponifi3d Jan 6, 2025

saponifi3d Jan 7, 2025

cathteng Jan 8, 2025

cathteng Jan 9, 2025

ceorourke Jan 10, 2025

saponifi3d Jan 11, 2025 •

edited

Loading

cathteng Jan 13, 2025

cathteng Jan 13, 2025

cathteng Jan 14, 2025

saponifi3d left a comment •

edited

Loading

saponifi3d Jan 11, 2025 •

edited

Loading

saponifi3d Jan 14, 2025

cathteng Jan 15, 2025 •

edited

Loading

saponifi3d Jan 14, 2025

		assert dc.condition_group == dcg


		class EventFrequencyQueryTest(EventFrequencyQueryTestBase):

		from sentry.workflow_engine.types import DataConditionHandler, DataConditionResult


		class EventFrequencyConditionHandler(BaseEventFrequencyConditionHandler):

		self.assert_slow_cond_passes(dc, 1001)
		self.assert_slow_cond_does_not_pass(dc, 999)

feat(aci): event frequency condition handler #82551

feat(aci): event frequency condition handler #82551

Conversation

cathteng commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d left a comment • edited Loading

Choose a reason for hiding this comment

saponifi3d Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathteng Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathteng commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

saponifi3d Jan 11, 2025 •

edited

Loading

saponifi3d left a comment •

edited

Loading

saponifi3d Jan 11, 2025 •

edited

Loading

cathteng Jan 15, 2025 •

edited

Loading