Sentry / PR #10

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

getsentry/sentry#80168
Golden recall: 50% Extra findings: 4

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 MetricAlertDetectorHandler extends StatefulDetectorHandler without abstract methods → TypeError at instantiation HIGH ✓ Found FriendlyReviewer: "does not implement required abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data)" (grouptype.py L14).
#2 Docstring says returns list of DetectorEvaluationResult but method now returns dict keyed by DetectorGroupKey LOW × Missed FriendlyReviewer did not flag the docstring inconsistency.

Supplementary Findings (7 findings)

Finding File Severity Legitimate?
Type mismatch: _bulk_commit_detector_state compares IntEnum priority to a string CharField → always True → unnecessary DB writes detector.py:408 HIGH ✓ Real bug — avoidable DB writes on every commit
enqueue_dedupe_update called before condition group check → dedupe slot consumed without evaluation detector.py:314 HIGH ✓ Real bug — loss of deduplicated data
build_mock_occurrence_and_event hardcodes same id/event_id → assert_has_calls(any_order=True) unreliable test_detector.py:253 MEDIUM ✓ Test fragility
Test test_state_results_multi_group with any_order=True unreliable (hardcoded IDs) test_detector.py:298 MEDIUM ✓ Same cause
build_mock_occurrence_and_event accepts value but never uses it test_detector.py:255 LOW ✓ Misleading parameter
test_results_on_change: expected value (6) ≠ actual value (100) because helper ignores value test_detector.py:274 LOW ✓ Misleading test
No dedicated test for deduplication at process_detectors level test_detector.py:368 LOW ✓ Missing coverage

Honest Analysis

FriendlyReviewer found the critical golden bug (#1 — missing abstract methods on MetricAlertDetectorHandler) but missed Golden #2 (docstring mismatch on return type). Golden recall sits at 50% — the tool catches runtime errors well but doesn't flag stale annotations. That's a blind spot worth tracking.

The 2 supplementary HIGH findings are legitimate — the IntEnum vs string comparison causing unnecessary DB writes, and the deduplication slot being consumed before the condition gate — both real defects. But they're not crash bugs; they're performance and data-integrity issues that need fixing but won't bring down production immediately. The framing as "2 additional critical bugs" needs that nuance.

The medium and low findings are all in test files: hardcoded IDs, unused parameters, missing coverage. They're correct observations, but none of them touch production logic. A human reviewer would likely flag the most important one (hardcoded IDs making assertions unreliable) and skip the rest as noise. FriendlyReviewer reports everything it finds, which is honest but noisy.

The post-verification on the IntEnum bug — confirming the Sentry team later fixed it — shows FriendlyReviewer can find real issues the mainline project wasn't aware of. That's the tool working as intended. But it doesn't change the fact that 1 out of 2 golden bugs was missed, and the supplementary finds, while real, are all in test code or non-production paths.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
3 high, 2 medium, 3 low

This merge request introduces the hook for producing occurrences from stateful detectors. Key changes include extending MetricAlertDetectorHandler from StatefulDetectorHandler, converting evaluate() return type to a dict, adding the build_occurrence_and_event_data abstract method, and wiring occurrence creation in evaluate_group_key_value. The review identified several critical issues that must be addressed before merging: missing abstract method implementations causing runtime errors, a type mismatch bug in priority comparison leading to unnecessary DB updates, a deduplication slot consumption bug, and test reliability issues with hardcoded IDs and misleading parameters. These findings require changes to ensure correctness and maintainability.

These might need a close look

  • 🔴 src/sentry/incidents/grouptype.py (L14)
    MetricAlertDetectorHandler extends StatefulDetectorHandler but does not implement required abstract methods (counter_names, get_dedupe_value, get_group_key_values, build_occurrence_and_event_data), which will cause a TypeError at instantiation. Implement stubs or clarify with a TODO.
  • 🔴 src/sentry/workflow_engine/processors/detector.py (L408)
    Type mismatch in _bulk_commit_detector_state: comparing an IntEnum priority to a CharField string representation will always be True, causing unnecessary DB updates on every commit. Convert both to the same type before comparing.
  • 🔴 src/sentry/workflow_engine/processors/detector.py (L314)
    Bug: enqueue_dedupe_update is called before the condition group check. If the condition group is missing, the dedupe slot is consumed but no evaluation occurs, causing subsequent data with the same dedupe value to be skipped. Move the call after the check or only enqueue when evaluation happens.

Worth checking

  • 🟡 tests/sentry/workflow_engine/processors/test_detector.py (L253)
    build_mock_occurrence_and_event hardcodes the same id and event_id for every call, so two calls produce equal IssueOccurrence objects. This makes assert_has_calls with any_order=True unreliable, potentially masking missing occurrences. Generate unique IDs per call (e.g., using uuid4).
  • 🟡 tests/sentry/workflow_engine/processors/test_detector.py (L298)
    Assertion in test_state_results_multi_group with any_order=True is unreliable due to hardcoded IDs and identical event_data; the mock may match both expected calls against a single actual call. Fix with unique IDs as noted above.

Small things (take or leave)

  • 🔵 tests/sentry/workflow_engine/processors/test_detector.py (L255)
    build_mock_occurrence_and_event accepts a value parameter but never uses it. This is misleading and masks test weakness; either use the value in evidence_data/event_data or remove the parameter with a comment.
  • 🔵 tests/sentry/workflow_engine/processors/test_detector.py (L274)
    In test_results_on_change, the expected value (6) differs from the actual value used in evaluate (100) because the helper ignores value. This is misleading and will silently break if value is used in the future. Fix the helper or add a comment.
  • 🔵 tests/sentry/workflow_engine/processors/test_detector.py (L368)
    No test exists for deduplication behavior at the process_detectors level when the same group_key appears twice in a data packet. Consider adding a test for documentation and coverage.