Skip to content
Jump to bottom

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

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

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

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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's find the StatefulDetectorHandler definition to see what abstract methods it actually requires
find . -type f -name "*.py" | xargs grep -l "class StatefulDetectorHandler" | head -5

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 136

🏁 Script executed:

# Check the structure of the file in question
cat -n src/sentry/incidents/grouptype.py | head -30

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1347

🏁 Script executed:

# Search for MetricAlertDetectorHandler and MetricAlertFire usage
rg -n "MetricAlertDetectorHandler|MetricAlertFire" --type py -B 2 -A 2 | head -100

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1016

🏁 Script executed:

# Examine the StatefulDetectorHandler base class
cat -n src/sentry/workflow_engine/processors/detector.py | head -150

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 6305

🏁 Script executed:

# Search for abstract methods in the detector module
rg -n "@abstractmethod|ABC|ABCMeta" src/sentry/workflow_engine/processors/detector.py -B 1 -A 3

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 512

🏁 Script executed:

# Get the complete StatefulDetectorHandler class definition
cat -n src/sentry/workflow_engine/processors/detector.py | sed -n '135,250p'

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 5496

🏁 Script executed:

# Check if MetricAlertDetectorHandler implements any methods
cat -n src/sentry/incidents/grouptype.py | sed -n '10,20p'

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 603

🏁 Script executed:

# Check if MetricAlertFire is actually used/instantiated anywhere
rg -n "MetricAlertFire" --type py

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 155

🏁 Script executed:

# Check if detector_handler values are instantiated in the Detector model
rg -n "detector_handler" --type py -B 2 -A 2 | head -60

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 4723

Critical: Missing required abstract method implementations.

MetricAlertDetectorHandler extends StatefulDetectorHandler but does not implement any of the required abstract methods:

  • counter_names (property)
  • get_dedupe_value(data_packet)
  • get_group_key_values(data_packet)
  • build_occurrence_and_event_data(group_key, value, new_status)
  • evaluate(data_packet) (inherited from DetectorHandler)

This will raise a TypeError at instantiation: "Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods..."

When MetricAlertFire is used with a Detector, the detector.detector_handler property will attempt to instantiate MetricAlertDetectorHandler(self), causing runtime failure. The TODO comment indicates this is placeholder code, but it's currently a latent defect waiting to break once integrated into detector workflows.

🤖 Prompt for AI Agents 
In `@src/sentry/incidents/grouptype.py` around lines 10 - 12,
MetricAlertDetectorHandler currently has no concrete implementations and must
implement the abstract API from StatefulDetectorHandler/DetectorHandler; add
concrete method bodies for the following members on MetricAlertDetectorHandler:
the counter_names property, get_dedupe_value(data_packet),
get_group_key_values(data_packet), build_occurrence_and_event_data(group_key,
value, new_status), and evaluate(data_packet). Implement them with the correct
signatures (matching StatefulDetectorHandler[QuerySubscriptionUpdate] and
DetectorHandler) — for now simple stubs that return appropriate empty/default
values or raise NotImplementedError with clear messages are acceptable so the
class can be instantiated and later fleshed out; ensure names exactly match
counter_names, get_dedupe_value, get_group_key_values,
build_occurrence_and_event_data, and evaluate so Detector.detector_handler
instantiation no longer fails.



# 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
Comment on lines +166 to +170
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

# First, find the definition of StatefulDetectorHandler
echo "=== Searching for StatefulDetectorHandler definition ==="
rg "class StatefulDetectorHandler" --type py -A 10

# Then search for all classes that inherit from StatefulDetectorHandler
echo -e "\n=== Searching for subclasses of StatefulDetectorHandler ==="
rg "StatefulDetectorHandler" --type py -B 2 | grep -E "^[^:]*class " | head -20

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 1219

🏁 Script executed:

#!/bin/bash

# Search for classes that inherit from StatefulDetectorHandler
echo "=== Finding all subclasses of StatefulDetectorHandler ==="
rg "class \w+\(.*StatefulDetectorHandler" --type py

# Search specifically for MetricAlertDetectorHandler
echo -e "\n=== Searching for MetricAlertDetectorHandler ==="
rg "class MetricAlertDetectorHandler" --type py -A 50

# Search for build_occurrence_and_event_data implementations
echo -e "\n=== Searching for build_occurrence_and_event_data implementations ==="
rg "def build_occurrence_and_event_data" --type py -B 5 -A 10

Repository: admin-coderabbit/coderabbit-sentry-mirror

Length of output: 4581

Critical: MetricAlertDetectorHandler missing required implementation.

The new abstract method build_occurrence_and_event_data is not implemented in MetricAlertDetectorHandler (src/sentry/incidents/grouptype.py), which will cause a TypeError when attempting to instantiate the class. This is a concrete subclass of StatefulDetectorHandler, so it must provide an implementation.

🤖 Prompt for AI Agents 
In `@src/sentry/workflow_engine/processors/detector.py` around lines 166 - 170,
MetricAlertDetectorHandler, a concrete subclass of StatefulDetectorHandler, is
missing the required implementation of the abstract method
build_occurrence_and_event_data; implement build_occurrence_and_event_data(self,
group_key: DetectorGroupKey, value: int, new_status: PriorityLevel) ->
tuple[IssueOccurrence, dict[str, Any]] in the MetricAlertDetectorHandler class
so it returns an IssueOccurrence instance and a serializable event data dict;
follow the same structure and fields used by other detector handlers'
implementations (e.g., create and populate the IssueOccurrence with group_key
and priority/status, and return event metadata as a dict), matching the exact
method signature and types to satisfy the abstract base class.


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)
)
return DetectorEvaluationResult(
group_key=group_key,
is_active=is_active,
Expand Down