feat(workflow_engine): Add in hook for producing occurrences from the stateful detector#1
Conversation
… stateful detector (#80168) This adds a hook that can be implemented to produce an occurrence specific to the detector that is subclassing the StatefulDetector. Also change the signature of evaluate to return a dict keyed by groupkey instead of a list. This helps avoid the chance of duplicate results for the same group key. <!-- Describe your PR here. -->
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/sentry/workflow_engine/processors/test_detector.py">
<violation number="1" location="tests/sentry/workflow_engine/processors/test_detector.py:150">
P1: Bug: occurrence is built with group_key `"val1"` but the expected result uses group_key `"group_key"`. The fingerprints will differ, causing a test assertion mismatch. Should use `"group_key"` as the group_key for the mock occurrence.</violation>
</file>
<file name="src/sentry/workflow_engine/processors/detector.py">
<violation number="1" location="src/sentry/workflow_engine/processors/detector.py:128">
P3: Docstring is now stale: it says "returns a list of `DetectorEvaluationResult`" but the return type was changed to `dict[DetectorGroupKey, DetectorEvaluationResult]`. Update the docstring to reflect the new return type.</violation>
</file>
<file name="src/sentry/incidents/grouptype.py">
<violation number="1" location="src/sentry/incidents/grouptype.py:11">
P2: `MetricAlertDetectorHandler` inherits from `StatefulDetectorHandler` (which is abstract) but does not implement any of its 4 required abstract methods (`counter_names`, `get_dedupe_value`, `get_group_key_values`, `build_occurrence_and_event_data`). This class cannot be instantiated — Python will raise `TypeError` at runtime. If this is intentional as a WIP, consider keeping it explicitly abstract (e.g., add `abc.ABC` to the class or add a comment clarifying it's not intended to be instantiated yet).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| detector = self.create_detector_and_conditions(type=self.handler_state_type.slug) | ||
| data_packet = DataPacket("1", {"dedupe": 2, "group_vals": {None: 6}}) | ||
| results = process_detectors(data_packet, [detector]) | ||
| occurrence, event_data = build_mock_occurrence_and_event( |
There was a problem hiding this comment.
P1: Bug: occurrence is built with group_key "val1" but the expected result uses group_key "group_key". The fingerprints will differ, causing a test assertion mismatch. Should use "group_key" as the group_key for the mock occurrence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/sentry/workflow_engine/processors/test_detector.py, line 150:
<comment>Bug: occurrence is built with group_key `"val1"` but the expected result uses group_key `"group_key"`. The fingerprints will differ, causing a test assertion mismatch. Should use `"group_key"` as the group_key for the mock occurrence.</comment>
<file context>
@@ -73,64 +138,88 @@ def test(self):
+ detector = self.create_detector_and_conditions(type=self.handler_state_type.slug)
+ data_packet = DataPacket("1", {"dedupe": 2, "group_vals": {None: 6}})
results = process_detectors(data_packet, [detector])
+ occurrence, event_data = build_mock_occurrence_and_event(
+ detector.detector_handler, None, 6, PriorityLevel.HIGH
+ )
</file context>
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): |
There was a problem hiding this comment.
P2: MetricAlertDetectorHandler inherits from StatefulDetectorHandler (which is abstract) but does not implement any of its 4 required abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data). This class cannot be instantiated — Python will raise TypeError at runtime. If this is intentional as a WIP, consider keeping it explicitly abstract (e.g., add abc.ABC to the class or add a comment clarifying it's not intended to be instantiated yet).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/incidents/grouptype.py, line 11:
<comment>`MetricAlertDetectorHandler` inherits from `StatefulDetectorHandler` (which is abstract) but does not implement any of its 4 required abstract methods (`counter_names`, `get_dedupe_value`, `get_group_key_values`, `build_occurrence_and_event_data`). This class cannot be instantiated — Python will raise `TypeError` at runtime. If this is intentional as a WIP, consider keeping it explicitly abstract (e.g., add `abc.ABC` to the class or add a comment clarifying it's not intended to be instantiated yet).</comment>
<file context>
@@ -4,17 +4,12 @@
- ) -> list[DetectorEvaluationResult]:
- # TODO: Implement
- return []
+class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
+ pass
</file context>
| def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]: | ||
| def evaluate( | ||
| self, data_packet: DataPacket[T] | ||
| ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: |
There was a problem hiding this comment.
P3: Docstring is now stale: it says "returns a list of DetectorEvaluationResult" but the return type was changed to dict[DetectorGroupKey, DetectorEvaluationResult]. Update the docstring to reflect the new return type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/workflow_engine/processors/detector.py, line 128:
<comment>Docstring is now stale: it says "returns a list of `DetectorEvaluationResult`" but the return type was changed to `dict[DetectorGroupKey, DetectorEvaluationResult]`. Update the docstring to reflect the new return type.</comment>
<file context>
@@ -136,7 +123,9 @@ def __init__(self, detector: Detector):
- def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]:
+ def evaluate(
+ self, data_packet: DataPacket[T]
+ ) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
pass
</file context>
Test 10
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/sentry-coderabbit#10