Skip to content
Jump to bottom

Automated Test: workflow-engine-stateful-detector-after #316

Closed
wants to merge 1 commit into from
+249 −151
Closed

Automated Test: workflow-engine-stateful-detector-after #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/sentry/incidents/grouptype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,12 @@
from sentry.issues.grouptype import GroupCategory, GroupType
from sentry.ratelimits.sliding_windows import Quota
from sentry.types.group import PriorityLevel
from sentry.workflow_engine.models import DataPacket
from sentry.workflow_engine.processors.detector import DetectorEvaluationResult, DetectorHandler
from sentry.workflow_engine.processors.detector import StatefulDetectorHandler


# TODO: This will be a stateful detector when we build that abstraction
class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]):
def evaluate(
self, data_packet: DataPacket[QuerySubscriptionUpdate]
) -> list[DetectorEvaluationResult]:
# TODO: Implement
return []
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
pass
Comment on lines 10 to +12
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, search for MetricAlertDetectorHandler and metric_alert_fire usage
echo "=== Searching for MetricAlertDetectorHandler usage ==="
rg -n "MetricAlertDetectorHandler" --type py -C 2

echo -e "\n=== Searching for metric_alert_fire usage ==="
rg -n "metric_alert_fire" --type py -C 2

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1129

🏁 Script executed:

#!/bin/bash
# Check the StatefulDetectorHandler base class to understand its abstract methods
echo "=== Finding StatefulDetectorHandler definition ==="
rg -n "class StatefulDetectorHandler" --type py -A 30

echo -e "\n=== Looking for abstract methods in StatefulDetectorHandler ==="
rg -n "@abstractmethod\|def counter_names\|def get_dedupe_value\|def get_group_key_values\|def build_occurrence_and_event_data" --type py -B 2 -A 3

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 2988

🏁 Script executed:

#!/bin/bash
# Examine the full grouptype.py file to see the complete context
echo "=== Full grouptype.py file ==="
cat -n src/sentry/incidents/grouptype.py | head -50

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1378

🏁 Script executed:

#!/bin/bash
# Search for where detector_handler is actually used/instantiated
echo "=== Searching for detector_handler usage/instantiation ==="
rg -n "\.detector_handler\(|detector_handler(" --type py -B 2 -A 3

echo -e "\n=== Searching for Detector class and how it uses detector_handler ==="
rg -n "class Detector" --type py -A 50 | head -80

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 8103

🏁 Script executed:

#!/bin/bash
# Search for where Detector model instantiates group_type.detector_handler
echo "=== Looking for detector instantiation patterns ==="
rg -n "group_type\.detector_handler\|self\.detector_handler\|detector\.detector_handler" --type py -B 2 -A 3

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 136

🏁 Script executed:

#!/bin/bash
# Check if there's any code that actually uses MetricAlertFire/metric_alert_fire
echo "=== Searching for MetricAlertFire or metric_alert_fire usage ==="
rg -n "MetricAlertFire\|metric_alert_fire" --type py -C 3

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 149

🏁 Script executed:

#!/bin/bash
# Search for how detector_handler is accessed/called
echo "=== Searching for detector_handler property access ==="
rg -n "detector_handler" --type py -B 2 -A 2

echo -e "\n=== Looking for Detector model definition ==="
rg -n "class Detector" --type py -B 2 -A 30 | head -100

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 14602

🏁 Script executed:

#!/bin/bash
# Check if there's any indication this GroupType is currently enabled/used
echo "=== Searching for GroupType registry or usage ==="
rg -n "MetricAlertFire\|8001\|metric_alert_fire" --type py

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 134

🏁 Script executed:

#!/bin/bash
# Verify the exact instantiation code to be 100% certain
echo "=== Detector.detector_handler property instantiation ==="
sed -n '63,86p' src/sentry/workflow_engine/models/detector.py

echo -e "\n=== StatefulDetectorHandler abstract methods ==="
sed -n '135,175p' src/sentry/workflow_engine/processors/detector.py

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 2511

Stale TODO and incomplete abstract method implementations.

Line 10: The TODO comment states "This will be a stateful detector when we build that abstraction" — but MetricAlertDetectorHandler already extends StatefulDetectorHandler. This comment is misleading and should be updated or removed.

More critically, StatefulDetectorHandler declares four abstract methods: counter_names, get_dedupe_value, get_group_key_values, and build_occurrence_and_event_data. Since MetricAlertDetectorHandler contains only pass, attempting to instantiate it (which occurs in Detector.detector_handler via group_type.detector_handler(self)) will raise TypeError: Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods [...].

While MetricAlertFire is not currently wired to any detectors (as noted in the comment on line 15), this incomplete code should be addressed before merge. Implement the required abstract methods or keep MetricAlertDetectorHandler as a concrete DetectorHandler subclass until the abstraction is ready.

🤖 Prompt for AI Agents 
In `@src/sentry/incidents/grouptype.py` around lines 10 - 12, Remove or update the
misleading TODO and make MetricAlertDetectorHandler concrete by either (A)
implementing the four abstract methods declared on StatefulDetectorHandler —
counter_names, get_dedupe_value, get_group_key_values, and
build_occurrence_and_event_data — with appropriate logic or stubs that delegate
to the existing MetricAlertFire behavior, or (B) if the stateful abstraction
isn't ready, change MetricAlertDetectorHandler to inherit from the concrete
DetectorHandler instead of StatefulDetectorHandler so it no longer requires
those abstract methods; ensure Detector.detector_handler can instantiate
MetricAlertDetectorHandler without raising a TypeError.



# Example GroupType and detector handler for metric alerts. We don't create these issues yet, but we'll use something
Expand Down
8 changes: 7 additions & 1 deletion src/sentry/workflow_engine/models/detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import builtins
import logging
from typing import TYPE_CHECKING

Expand All @@ -9,6 +10,7 @@
from sentry.backup.scopes import RelocationScope
from sentry.db.models import DefaultFieldsModel, FlexibleForeignKey, region_silo_model
from sentry.issues import grouptype
from sentry.issues.grouptype import GroupType
from sentry.models.owner_base import OwnerModel

if TYPE_CHECKING:
Expand Down Expand Up @@ -53,9 +55,13 @@ def project_id(self):
# XXX: Temporary property until we add `project_id` to the model.
return 1

@property
def group_type(self) -> builtins.type[GroupType] | None:
return grouptype.registry.get_by_slug(self.type)

@property
def detector_handler(self) -> DetectorHandler | None:
group_type = grouptype.registry.get_by_slug(self.type)
group_type = self.group_type
if not group_type:
logger.error(
"No registered grouptype for detector",
Expand Down
45 changes: 22 additions & 23 deletions src/sentry/workflow_engine/processors/detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka
from sentry.issues.status_change_message import StatusChangeMessage
from sentry.models.group import GroupStatus
from sentry.types.group import PriorityLevel
from sentry.utils import metrics, redis
from sentry.utils.function_cache import cache_func_for_models
from sentry.utils.iterators import chunked
Expand Down Expand Up @@ -45,7 +46,7 @@ class DetectorEvaluationResult:

def process_detectors(
data_packet: DataPacket, detectors: list[Detector]
) -> list[tuple[Detector, list[DetectorEvaluationResult]]]:
) -> list[tuple[Detector, dict[DetectorGroupKey, DetectorEvaluationResult]]]:
results = []

for detector in detectors:
Expand All @@ -55,25 +56,11 @@ def process_detectors(
continue

detector_results = handler.evaluate(data_packet)
detector_group_keys = set()

for result in detector_results:
if result.group_key in detector_group_keys:
# This shouldn't happen - log an error and continue on, but we should investigate this.
logger.error(
"Duplicate detector state group keys found",
extra={
"detector_id": detector.id,
"group_key": result.group_key,
},
)
continue

for result in detector_results.values():
if result.result is not None:
create_issue_occurrence_from_result(result)

detector_group_keys.add(result.group_key)

if detector_results:
results.append((detector, detector_results))

Expand Down Expand Up @@ -136,7 +123,9 @@ def __init__(self, detector: Detector):
self.conditions = []

@abc.abstractmethod
def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]:
def evaluate(
self, data_packet: DataPacket[T]
) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
pass

def commit_state_updates(self):
Expand Down Expand Up @@ -174,6 +163,12 @@ def get_group_key_values(self, data_packet: DataPacket[T]) -> dict[str, int]:
"""
pass

@abc.abstractmethod
def build_occurrence_and_event_data(
self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel
) -> tuple[IssueOccurrence, dict[str, Any]]:
pass

def build_fingerprint(self, group_key) -> list[str]:
"""
Builds a fingerprint to uniquely identify a detected issue
Expand Down Expand Up @@ -228,7 +223,9 @@ def get_state_data(
)
return results

def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]:
def evaluate(
self, data_packet: DataPacket[T]
) -> dict[DetectorGroupKey, DetectorEvaluationResult]:
"""
Evaluates a given data packet and returns a list of `DetectorEvaluationResult`.
There will be one result for each group key result in the packet, unless the
Expand All @@ -237,13 +234,13 @@ def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]
dedupe_value = self.get_dedupe_value(data_packet)
group_values = self.get_group_key_values(data_packet)
all_state_data = self.get_state_data(list(group_values.keys()))
results = []
results = {}
for group_key, group_value in group_values.items():
result = self.evaluate_group_key_value(
group_key, group_value, all_state_data[group_key], dedupe_value
)
if result:
results.append(result)
results[result.group_key] = result
return results

def evaluate_group_key_value(
Expand Down Expand Up @@ -289,7 +286,7 @@ def evaluate_group_key_value(
is_active = new_status != DetectorPriorityLevel.OK
self.enqueue_state_update(group_key, is_active, new_status)
event_data = None
result = None
result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
# If we've determined that we're now ok, we just want to resolve the issue
result = StatusChangeMessage(
Expand All @@ -298,8 +295,10 @@ def evaluate_group_key_value(
new_status=GroupStatus.RESOLVED,
new_substatus=None,
)

# TODO: Add hook here for generating occurrence
else:
result, event_data = self.build_occurrence_and_event_data(
group_key, value, PriorityLevel(new_status)
)
Comment on lines +298 to +301
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Compare the enum values of DetectorPriorityLevel and PriorityLevel
echo "=== DetectorPriorityLevel ==="
rg -n "class DetectorPriorityLevel" -A 20 --type py

echo -e "\n=== PriorityLevel ==="
rg -n "class PriorityLevel" -A 20 --type py

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 5279

Fix unsafe enum conversion PriorityLevel(new_status) — DetectorPriorityLevel.OK has no PriorityLevel equivalent.

DetectorPriorityLevel includes an OK member (value 0) that does not exist in PriorityLevel, which only defines LOW (25), MEDIUM (50), and HIGH (75). Passing DetectorPriorityLevel.OK to PriorityLevel() will raise ValueError at runtime.

Handle the OK case separately or map it explicitly to a valid PriorityLevel member.

🤖 Prompt for AI Agents 
In `@src/sentry/workflow_engine/processors/detector.py` around lines 298 - 301,
The code currently calls PriorityLevel(new_status) inside the else branch of the
detector logic which will raise ValueError when new_status is
DetectorPriorityLevel.OK because OK (0) has no PriorityLevel equivalent; update
the logic in the code that calls build_occurrence_and_event_data (the call
shown) to first check for DetectorPriorityLevel.OK and handle it explicitly
(e.g., skip conversion or map OK to a specific PriorityLevel member) or
implement an explicit mapping function from DetectorPriorityLevel to
PriorityLevel and use that instead of directly constructing
PriorityLevel(new_status); ensure you reference the DetectorPriorityLevel.OK
case and replace the direct PriorityLevel(new_status) usage with the safe branch
or mapper before passing the value into build_occurrence_and_event_data.

return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
Expand Down