feat(workflow_engine): Add in hook for producing occurrences from the stateful detector#10
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. -->
WalkthroughThe changes refactor detector evaluation in the workflow engine from returning lists to returning dictionaries keyed by group keys. Abstract methods and class inheritance are updated to reflect this, and a new method for building issue occurrences and event data is introduced. Associated tests are overhauled to match the new API and improve coverage and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant DataPacket
participant Detector
participant DetectorHandler
participant StatefulDetectorHandler
participant TestSuite
DataPacket->>Detector: process_detectors(data_packet, detectors)
Detector->>DetectorHandler: evaluate(data_packet)
alt Stateful Handler
DetectorHandler->>StatefulDetectorHandler: evaluate(data_packet)
StatefulDetectorHandler->>StatefulDetectorHandler: build_occurrence_and_event_data(group_key, value, new_status)
StatefulDetectorHandler-->>Detector: dict[group_key, DetectorEvaluationResult]
else Stateless Handler
DetectorHandler-->>Detector: dict[group_key, DetectorEvaluationResult]
end
Detector-->>DataPacket: list of (Detector, dict[group_key, DetectorEvaluationResult])
TestSuite->>DetectorHandler: evaluate(data_packet)
TestSuite->>StatefulDetectorHandler: build_occurrence_and_event_data(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sentry/workflow_engine/models/detector.py (1)
3-3: Remove unnecessarybuiltinsimport and simplify type annotation.The
builtinsimport is unnecessary for type annotations. Python's built-intypecan be used directly.-import builtins import logging@property -def group_type(self) -> builtins.type[GroupType] | None: +def group_type(self) -> type[GroupType] | None: return grouptype.registry.get_by_slug(self.type)Also applies to: 59-60
tests/sentry/workflow_engine/processors/test_detector.py (1)
123-124: Consider using @freeze_time at the method level for better granularity.While using
@freeze_time()at the class level works, applying it at the method level for only the tests that need stable timestamps would be more precise and make the test requirements clearer.Also applies to: 422-423, 531-532
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/incidents/grouptype.py(1 hunks)src/sentry/workflow_engine/models/detector.py(3 hunks)src/sentry/workflow_engine/processors/detector.py(9 hunks)tests/sentry/workflow_engine/processors/test_detector.py(11 hunks)
🔇 Additional comments (13)
src/sentry/workflow_engine/models/detector.py (2)
58-61: Good refactoring to centralize group type retrieval.The introduction of the
group_typeproperty effectively centralizes the logic for retrieving the group type from the registry, avoiding code duplication and improving maintainability.
62-86: LGTM! Proper use of the centralized group_type property.The refactoring correctly uses the new
group_typeproperty instead of duplicating the registry lookup logic.src/sentry/incidents/grouptype.py (2)
7-7: Clean import statement refactoring.Good cleanup removing unused imports and retaining only the necessary
StatefulDetectorHandler.
11-12: Appropriate inheritance from StatefulDetectorHandler.The refactoring to inherit from
StatefulDetectorHandler[QuerySubscriptionUpdate]aligns with the new stateful detector pattern. The removal of the emptyevaluatemethod is correct since the parent class provides the implementation.src/sentry/workflow_engine/processors/detector.py (5)
47-71: Good refactoring to use dictionaries for detector results.The change from lists to dictionaries keyed by
DetectorGroupKeyimproves the API by:
- Providing O(1) lookup by group key
- Naturally preventing duplicate keys
- Making the data structure more intuitive
The removal of duplicate key error handling is appropriate since dictionaries inherently prevent duplicates.
125-129: Consistent update to abstract method signature.The
evaluatemethod's return type change todict[DetectorGroupKey, DetectorEvaluationResult]properly reflects the new evaluation result structure across the codebase.
166-170: Well-designed abstract method for building occurrences.The
build_occurrence_and_event_dataabstract method properly delegates occurrence creation to concrete implementations while providing all necessary context through its parameters.
226-244: Correct implementation of dictionary-based evaluation results.The
evaluatemethod properly implements the new contract by returning a dictionary keyed by group keys, maintaining consistency with the updated API.
288-308: Proper integration of the new occurrence building abstraction.The method correctly delegates occurrence creation to the
build_occurrence_and_event_dataabstract method for non-OK statuses while maintaining the existing StatusChangeMessage logic for resolutions.tests/sentry/workflow_engine/processors/test_detector.py (4)
27-40: Well-implemented mock handler for testing.The
MockDetectorStateHandlerproperly implements all abstract methods and provides a good foundation for testing the stateful detector behavior.
42-121: Excellent test base class design.The
BaseDetectorHandlerTesteffectively consolidates common test setup and provides reusable utilities, significantly improving test maintainability and reducing duplication.
244-276: Good helper function for consistent test data generation.The
build_mock_occurrence_and_eventfunction effectively centralizes the creation of test occurrences and event data, ensuring consistency across tests.
134-173: Comprehensive test coverage for the new dictionary-based API.The tests properly verify:
- Dictionary-based evaluation results
- Occurrence and event data generation
- Multi-group key handling
- Kafka production calls
Excellent adaptation to the new API contract.
Also applies to: 174-223
2 participants
Test 10
Summary by CodeRabbit
Refactor
Tests