Automated Test: workflow-engine-stateful-detector-after #325
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. -->
📝 WalkthroughWalkthroughThe detector system transitions from stateless list-based evaluation to stateful dict-keyed results. The Changes
Sequence DiagramsequenceDiagram
participant DataPacket
participant ProcessDetectors as process_detectors()
participant DetectorHandler as StatefulDetectorHandler
participant BuildOccurrence as build_occurrence_and_event_data()
participant KafkaProducer
DataPacket->>ProcessDetectors: data_packet, detectors list
ProcessDetectors->>DetectorHandler: evaluate(data_packet)
DetectorHandler->>DetectorHandler: returns dict[DetectorGroupKey, DetectorEvaluationResult]
loop For each group_key, evaluation_result
rect rgba(100, 150, 255, 0.5)
alt new_status == OK
DetectorHandler->>KafkaProducer: StatusChangeMessage
else new_status != OK
DetectorHandler->>BuildOccurrence: (group_key, value, new_status)
BuildOccurrence->>BuildOccurrence: construct IssueOccurrence + event_data
BuildOccurrence-->>DetectorHandler: (IssueOccurrence, event_data)
DetectorHandler->>KafkaProducer: produce_occurrence_to_kafka(occurrence, event_data)
end
end
end
KafkaProducer-->>ProcessDetectors: acknowledgement
ProcessDetectors-->>DataPacket: list[tuple[Detector, dict]]
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/sentry/incidents/grouptype.py`:
- Around line 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.
In `@src/sentry/workflow_engine/processors/detector.py`:
- Around line 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.
In `@tests/sentry/workflow_engine/processors/test_detector.py`:
- Around line 190-192: The test passes inconsistent value for group_2 to
build_mock_occurrence_and_event: change the call that assigns occurrence_2,
event_data_2 (build_mock_occurrence_and_event(detector.detector_handler,
"group_2", 6, PriorityLevel.HIGH)) to use value=10 to match the data packet for
"group_2"; this keeps build_mock_occurrence_and_event, occurrence_2, and
event_data_2 consistent even if the value parameter becomes used later.
🧹 Nitpick comments (4)
src/sentry/workflow_engine/models/detector.py (1)
64-74: Minor: Error log message could be more informative.When
group_typeisNone, loggingstr(group_type)will always produce"None". Thedetector_typefield already captures the slug, so this is redundant but not incorrect.Optional: Remove redundant field from error log
if not group_type: logger.error( "No registered grouptype for detector", extra={ - "group_type": str(group_type), "detector_id": self.id, "detector_type": self.type, }, )src/sentry/workflow_engine/processors/detector.py (1)
226-233: Docstring is outdated.The docstring on line 230 still says "returns a list of
DetectorEvaluationResult" but the method now returns adict[DetectorGroupKey, DetectorEvaluationResult].Update docstring to match new return type
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 + Evaluates a given data packet and returns a dict of `DetectorEvaluationResult` keyed by group key. + There will be one entry for each group key result in the packet, unless the evaluation is skipped due to various rules. """tests/sentry/workflow_engine/processors/test_detector.py (2)
27-39: LGTM! Well-structured mock implementation.
MockDetectorStateHandlercorrectly implements all required abstract methods fromStatefulDetectorHandler. Consider adding theClassVarannotation forcounter_namesto satisfy the static analysis hint, though this is a minor style issue.Optional: Add ClassVar annotation per Ruff RUF012
+from typing import Any, ClassVar + class MockDetectorStateHandler(StatefulDetectorHandler[dict]): - counter_names = ["test1", "test2"] + counter_names: ClassVar[list[str]] = ["test1", "test2"]
244-275: Unusedvalueparameter in helper function.The
valueparameter (line 247) is declared but never used in the function body. If it's meant for future use, consider adding a comment. Otherwise, it could be removed to avoid confusion.Option 1: Add comment explaining future use
def build_mock_occurrence_and_event( handler: StatefulDetectorHandler, group_key: DetectorGroupKey, - value: int, + value: int, # Reserved for future use when occurrence data includes the triggering value new_status: PriorityLevel, ) -> tuple[IssueOccurrence, dict[str, Any]]:Option 2: Remove unused parameter
def build_mock_occurrence_and_event( handler: StatefulDetectorHandler, group_key: DetectorGroupKey, - value: int, new_status: PriorityLevel, ) -> tuple[IssueOccurrence, dict[str, Any]]:Note: This would require updating all call sites.
| # 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 |
There was a problem hiding this comment.
🧩 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 -5Repository: 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 -30Repository: 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 -100Repository: 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 -150Repository: 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 3Repository: 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 pyRepository: 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 -60Repository: 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 fromDetectorHandler)
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.
| @abc.abstractmethod | ||
| def build_occurrence_and_event_data( | ||
| self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel | ||
| ) -> tuple[IssueOccurrence, dict[str, Any]]: | ||
| pass |
There was a problem hiding this comment.
🧩 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 -20Repository: 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 10Repository: 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.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | ||
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | ||
| ) |
There was a problem hiding this comment.
Potential test data inconsistency: value parameter mismatch.
On line 191, build_mock_occurrence_and_event is called with value=6, but in the data packet on line 177, group_2 has value 10. While the value parameter is currently unused in build_mock_occurrence_and_event, this inconsistency could cause confusion or break tests if the value is used in the future.
Suggested fix: Use consistent value
occurrence_2, event_data_2 = build_mock_occurrence_and_event(
- detector.detector_handler, "group_2", 6, PriorityLevel.HIGH
+ detector.detector_handler, "group_2", 10, PriorityLevel.HIGH
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | |
| ) | |
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 10, PriorityLevel.HIGH | |
| ) |
🤖 Prompt for AI Agents
In `@tests/sentry/workflow_engine/processors/test_detector.py` around lines 190 -
192, The test passes inconsistent value for group_2 to
build_mock_occurrence_and_event: change the call that assigns occurrence_2,
event_data_2 (build_mock_occurrence_and_event(detector.detector_handler,
"group_2", 6, PriorityLevel.HIGH)) to use value=10 to match the data packet for
"group_2"; this keeps build_mock_occurrence_and_event, occurrence_2, and
event_data_2 consistent even if the value parameter becomes used later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Refactor
Tests