feat(workflow_engine): Add in hook for producing occurrences from the stateful detector#15
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. -->
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors detector evaluation to return per-group results as dictionaries keyed by Changes
Sequence DiagramsequenceDiagram
participant DataSource as Data Source
participant Detector as Detector Processor
participant Handler as DetectorHandler
participant StatefulHandler as StatefulDetectorHandler
participant Registry as GroupType Registry
DataSource->>Detector: DataPacket
Detector->>Handler: evaluate(data_packet)
alt Stateful Handler
Handler->>StatefulHandler: evaluate(data_packet)
StatefulHandler->>StatefulHandler: Check status transition
alt OK Status
StatefulHandler-->>Handler: dict[DetectorGroupKey,<br/>StatusChangeMessage]
else Non-OK Status
StatefulHandler->>StatefulHandler: build_occurrence_and_event_data<br/>(group_key, value, new_status)
StatefulHandler-->>Handler: dict[DetectorGroupKey,<br/>DetectorEvaluationResult<br/>(occurrence, event_data)]
end
else Regular Handler
Handler-->>Detector: dict[DetectorGroupKey,<br/>DetectorEvaluationResult]
end
Detector->>Registry: group_type = get_by_slug(type)
Registry-->>Detector: GroupType class
Detector-->>DataSource: list[tuple[Detector,<br/>dict[DetectorGroupKey,<br/>DetectorEvaluationResult]]]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/sentry/incidents/grouptype.py:
- Around line 10-12: MetricAlertDetectorHandler currently subclasses
StatefulDetectorHandler but lacks required abstract members causing TypeError on
instantiation; implement the missing property/method stubs: add a counter_names
property, and implement get_dedupe_value(self, packet: DataPacket) -> Any,
get_group_key_values(self, packet: DataPacket) -> DetectorGroupKey, and
build_occurrence_and_event_data(self, packet: DataPacket) ->
Tuple[IssueOccurrence, Dict[str, Any]] (or the appropriate return types) inside
the MetricAlertDetectorHandler class, using signatures matching
StatefulDetectorHandler, and import the required symbols (Any, IssueOccurrence,
DataPacket, DetectorGroupKey) so the class satisfies the abstract interface and
can be instantiated by MetricAlertFire.detector_handler(self).
In @tests/sentry/workflow_engine/processors/test_detector.py:
- Around line 190-192: The test passes an incorrect value for group_2 to
build_mock_occurrence_and_event: change the third argument in the call using
detector.detector_handler for "group_2" from 6 to 10 so it matches the test
packet's "group_vals" (i.e., call
build_mock_occurrence_and_event(detector.detector_handler, "group_2", 10,
PriorityLevel.HIGH)); this keeps the test consistent and prevents future
surprises if the helper starts using the value.
🧹 Nitpick comments (2)
src/sentry/workflow_engine/processors/detector.py (1)
229-233: Docstring is outdated - still references "list" instead of "dict".The docstring says "returns a list of
DetectorEvaluationResult" but the method now returnsdict[DetectorGroupKey, DetectorEvaluationResult].🔎 Proposed fix
def evaluate( self, data_packet: DataPacket[T] ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: """ - Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. + Evaluates a given data packet and returns a dict of `DetectorEvaluationResult` keyed by group key. There will be one result 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 (1)
244-275: Consider adding a note about the unusedvalueparameter.The
valueparameter is part of the interface signature but currently unused in the mock implementation. A brief comment would clarify this is intentional for interface compliance.🔎 Proposed change
def build_mock_occurrence_and_event( handler: StatefulDetectorHandler, group_key: DetectorGroupKey, - value: int, + value: int, # Unused; kept for interface compatibility with build_occurrence_and_event_data new_status: PriorityLevel, ) -> tuple[IssueOccurrence, dict[str, Any]]:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/incidents/grouptype.pysrc/sentry/workflow_engine/models/detector.pysrc/sentry/workflow_engine/processors/detector.pytests/sentry/workflow_engine/processors/test_detector.py
🧰 Additional context used
🧬 Code graph analysis (4)
src/sentry/workflow_engine/models/detector.py (1)
src/sentry/issues/grouptype.py (2)
GroupType(141-223)get_by_slug(104-107)
src/sentry/incidents/grouptype.py (1)
src/sentry/workflow_engine/processors/detector.py (1)
StatefulDetectorHandler(135-407)
src/sentry/workflow_engine/processors/detector.py (2)
src/sentry/types/group.py (1)
PriorityLevel(63-80)src/sentry/workflow_engine/models/detector.py (1)
Detector(23-86)
tests/sentry/workflow_engine/processors/test_detector.py (3)
src/sentry/issues/producer.py (1)
PayloadType(25-34)src/sentry/workflow_engine/processors/detector.py (12)
StatefulDetectorHandler(135-407)counter_names(144-148)get_dedupe_value(151-156)get_group_key_values(159-164)build_occurrence_and_event_data(167-170)evaluate(126-129)evaluate(226-244)DetectorEvaluationResult(35-44)process_detectors(47-70)build_fingerprint(172-176)commit_state_updates(131-132)commit_state_updates(356-358)src/sentry/workflow_engine/models/detector.py (2)
Detector(23-86)detector_handler(63-86)
🪛 Ruff (0.14.10)
tests/sentry/workflow_engine/processors/test_detector.py
28-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
60-60: Unused method argument: data_packet
(ARG002)
247-247: Unused function argument: value
(ARG001)
🔇 Additional comments (9)
src/sentry/workflow_engine/models/detector.py (2)
58-60: LGTM! Clean property extraction.The new
group_typeproperty nicely encapsulates the registry lookup, making the code more DRY and enabling reuse (e.g., in tests viahandler.detector.group_type). Usingbuiltins.type[GroupType]correctly avoids shadowing with the model'stypefield.
62-74: LGTM! Refactored to use the new property.The
detector_handlerproperty now cleanly usesself.group_type, and the error logging still correctly captures the relevant context when the group type is missing.src/sentry/workflow_engine/processors/detector.py (3)
166-170: LGTM! Well-defined abstract hook for occurrence generation.The new
build_occurrence_and_event_dataabstract method provides a clean extension point for subclasses to produce detector-specific occurrences. The signature correctly accepts the group key, value, and new status to build the occurrence.
47-70: LGTM! Process flow correctly adapted for dict-based results.The
process_detectorsfunction properly iterates overdetector_results.values()and accumulates results per detector. The commit of state updates after processing each detector ensures consistency.
298-301: The type conversion is safe. The code explicitly checksif new_status == DetectorPriorityLevel.OK:on line 297, ensuring theelseblock (wherePriorityLevel(new_status)is called) only executes whennew_statusis one ofLOW,MEDIUM, orHIGH—all of which have identical integer values in both enums.tests/sentry/workflow_engine/processors/test_detector.py (3)
27-39: LGTM! Clean mock implementation of StatefulDetectorHandler.The
MockDetectorStateHandlercorrectly implements all abstract methods required byStatefulDetectorHandler, providing a solid test fixture.Regarding the static analysis hint about
counter_names: whileClassVarannotation would be more precise for a class-level attribute, this is acceptable for test code where the distinction is less critical.
82-84: LGTM! Proper cleanup of the StatusChangeMessage comparator patch.The tearDown correctly exits the mock patch context, ensuring clean test isolation.
145-172: LGTM! Comprehensive test for stateful detector results.The test properly verifies the end-to-end flow including the occurrence production to Kafka, validating both the returned results and the side effects.
src/sentry/incidents/grouptype.py (1)
17-27: Type_id = 8001 is unique across all GroupType subclasses. Verified in production code across both src/sentry/issues/grouptype.py and src/sentry/incidents/grouptype.py. All other GroupType implementations use type_ids in different ranges (1, 1010–1019, 2001–2011, 4001, 6001, 7001).
| # 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.
Missing required abstract method implementations will cause runtime errors.
MetricAlertDetectorHandler extends StatefulDetectorHandler but does not implement the required abstract methods:
counter_names(property)get_dedupe_value()get_group_key_values()build_occurrence_and_event_data()
This will raise a TypeError when instantiating MetricAlertDetectorHandler via MetricAlertFire.detector_handler(self).
🔎 Suggested stub implementations
class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]):
- pass
+ @property
+ def counter_names(self) -> list[str]:
+ return []
+
+ def get_dedupe_value(self, data_packet: DataPacket[QuerySubscriptionUpdate]) -> int:
+ raise NotImplementedError("TODO: Implement dedupe extraction")
+
+ def get_group_key_values(self, data_packet: DataPacket[QuerySubscriptionUpdate]) -> dict[str, int]:
+ raise NotImplementedError("TODO: Implement group key extraction")
+
+ def build_occurrence_and_event_data(
+ self, group_key: DetectorGroupKey, value: int, new_status: PriorityLevel
+ ) -> tuple[IssueOccurrence, dict[str, Any]]:
+ raise NotImplementedError("TODO: Implement occurrence building")You'll also need to add the missing imports:
from typing import Any
from sentry.issues.issue_occurrence import IssueOccurrence
from sentry.workflow_engine.models import DataPacket
from sentry.workflow_engine.types import DetectorGroupKey🤖 Prompt for AI Agents
In @src/sentry/incidents/grouptype.py around lines 10 - 12,
MetricAlertDetectorHandler currently subclasses StatefulDetectorHandler but
lacks required abstract members causing TypeError on instantiation; implement
the missing property/method stubs: add a counter_names property, and implement
get_dedupe_value(self, packet: DataPacket) -> Any, get_group_key_values(self,
packet: DataPacket) -> DetectorGroupKey, and
build_occurrence_and_event_data(self, packet: DataPacket) ->
Tuple[IssueOccurrence, Dict[str, Any]] (or the appropriate return types) inside
the MetricAlertDetectorHandler class, using signatures matching
StatefulDetectorHandler, and import the required symbols (Any, IssueOccurrence,
DataPacket, DetectorGroupKey) so the class satisfies the abstract interface and
can be instantiated by MetricAlertFire.detector_handler(self).
| 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.
Incorrect value passed to build_mock_occurrence_and_event for group_2.
The test packet defines "group_vals": {"group_1": 6, "group_2": 10}, but build_mock_occurrence_and_event is called with value=6 for group_2 instead of value=10. While this doesn't affect the test outcome (since value is unused in the helper), it's misleading and could cause issues if the helper is updated to use the value.
🔎 Proposed fix
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 an incorrect value for group_2 to
build_mock_occurrence_and_event: change the third argument in the call using
detector.detector_handler for "group_2" from 6 to 10 so it matches the test
packet's "group_vals" (i.e., call
build_mock_occurrence_and_event(detector.detector_handler, "group_2", 10,
PriorityLevel.HIGH)); this keeps the test consistent and prevents future
surprises if the helper starts using the value.
2 participants
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