feat(workflow_engine): Add in hook for producing occurrences from the stateful detector

[ShashankFC]

Test [new PR number opened]nn---nReplicated from ai-code-review-evaluation/sentry-greptile#10

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 26s —— View job

---

Code Review

Todo List

• Read repository guidelines (CLAUDE.md)
• Review changed files and understand the implementation
• Analyze code quality, bugs, and best practices
• Provide comprehensive review feedback

---

Summary

This PR implements a hook for producing occurrences from the stateful detector. The changes introduce a new abstract method build_occurrence_and_event_data() that allows stateful detectors to create issue occurrences, which are then automatically produced to Kafka. The refactoring changes the return type from list[DetectorEvaluationResult] to dict[DetectorGroupKey, DetectorEvaluationResult] to better handle multiple group keys.

Changes Overview

1. src/sentry/incidents/grouptype.py: Simplified MetricAlertDetectorHandler to inherit from StatefulDetectorHandler
2. src/sentry/workflow_engine/models/detector.py: Added group_type property for cleaner code
3. src/sentry/workflow_engine/processors/detector.py: Main changes - added occurrence production hook and changed evaluation return type
4. tests/sentry/workflow_engine/processors/test_detector.py: Comprehensive test updates with occurrence validation

Code Quality Review

✅ Strengths

1. Good Abstraction: The new build_occurrence_and_event_data() abstract method properly enforces that all stateful detectors must implement occurrence generation
2. Type Safety: Better return type using dict instead of list prevents duplicate group keys
3. Comprehensive Tests: Test coverage is excellent with proper mocking and validation of Kafka production
4. Clean Refactoring: Removed duplicate detection logic in process_detectors() since dict keys naturally prevent duplicates

⚠️ Issues & Concerns

1. Incomplete Implementation in MetricAlertDetectorHandler (High Priority)

Location: src/sentry/incidents/grouptype.py:11-12

The MetricAlertDetectorHandler now extends StatefulDetectorHandler[QuerySubscriptionUpdate] but doesn't implement any of the required abstract methods:

• counter_names (property)
• get_dedupe_value()
• get_group_key_values()
• build_occurrence_and_event_data()

This will cause runtime errors when this handler is instantiated. Either:

• Implement all required methods, or
• Add placeholder implementations with pass or raise NotImplementedError() if this is intentionally incomplete

Impact: This code cannot be instantiated without runtime errors.

Fix this →

2. Type Annotation Issue

Location: src/sentry/workflow_engine/processors/detector.py:289

result: StatusChangeMessage | IssueOccurrence

This type annotation appears before the variable is assigned in both branches. While Python allows this, it would be clearer to initialize or restructure:

result: StatusChangeMessage | IssueOccurrence
if new_status == DetectorPriorityLevel.OK:
    result = StatusChangeMessage(...)
else:
    result, event_data = self.build_occurrence_and_event_data(...)

The issue is that event_data is initialized to None at line 288, but then gets reassigned from the tuple unpacking at line 299. This works but is slightly confusing.

3. Test Helper Function Location

Location: tests/sentry/workflow_engine/processors/test_detector.py:244-275

The build_mock_occurrence_and_event() helper function is defined in the middle of the file (after test classes but before other test classes). For better organization, it should be:

• Moved to the top of the file after imports, or
• Moved to a test utilities module if used across multiple test files

4. Missing Edge Case Handling

Location: src/sentry/workflow_engine/processors/detector.py:299-301

When new_status != DetectorPriorityLevel.OK, the code calls build_occurrence_and_event_data() which is an abstract method. There's no error handling if this method fails or returns unexpected data. Consider adding validation or try-catch.

5. Potential Type Safety Issue

Location: src/sentry/workflow_engine/processors/detector.py:167-170

The build_occurrence_and_event_data() method signature requires new_status: PriorityLevel, but at the call site (line 300), it's called with PriorityLevel(new_status) where new_status is a DetectorPriorityLevel. This conversion should be documented or validated to ensure the enum values align correctly.

📝 Minor Suggestions

1. Documentation: The new build_occurrence_and_event_data() abstract method could benefit from a docstring explaining:

   • What the parameters represent
   • What the return values should contain
   • Any constraints or requirements

2. Test Organization: Consider using @freeze_time() decorator consistently. Some test classes have it, others don't, which might lead to flaky time-based tests.

3. Consistency: The status_change_comparator function (line 316-324) is defined to patch StatusChangeMessage.__eq__. This is a clever workaround but might be clearer with a custom test assertion method or a dedicated test matcher.

🔍 Questions for Clarification

1. Is MetricAlertDetectorHandler intentionally left as a stub? The TODO comment suggests this is temporary, but the inheritance change requires implementation.

2. Should there be any rate limiting or validation on the Kafka production in create_issue_occurrence_from_result()?

3. The PR removes the duplicate group key detection logic. Was this causing issues in production, or is this purely a refactoring improvement?

Overall Assessment

Rating: ⚠️ Needs Revision

The architectural changes are sound and the test coverage is excellent. However, the incomplete implementation of MetricAlertDetectorHandler is a blocking issue that will cause runtime errors. Once the abstract methods are properly implemented (or stubbed with appropriate error messages), this PR will be in good shape.

The change from list to dict for evaluation results is a smart improvement that simplifies the code and prevents duplicate handling issues naturally through data structure choice.

---