Sentry / PR #10

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

getsentry/sentry#80168
Golden recall: 100% Extra findings: 3

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 MetricAlertDetectorHandler inherits from StatefulDetectorHandler but only contains pass, failing to implement its required abstract methods: counter_names (property), get_dedupe_value(), get_group_key_values(), and build_occurrence_and_event_data(). This will cause a TypeError at runtime when the class is instantiated HIGH ✓ Found "The new abstract method build_occurrence_and_event_data in StatefulDetectorHandler (detector.py) leaves the placeholder MetricAlertDetectorHandler abstract and uninstantiable, causing Detector.detector_handler to raise TypeError for any detector of type metric_alert_fire. Provide stub implementations of the abstract members or leave detector_handler unset" (grouptype.py L11)
#2 Docstring says this returns a list of DetectorEvaluationResult, but the method now returns a dict keyed by DetectorGroupKey. Consider updating the docstring to match the new return type LOW ✓ Found "The evaluate docstring still says it returns a list of DetectorEvaluationResult, but the method now returns a dict keyed by group key. Update the docstring to match the new contract" (detector.py L225)

Supplementary Findings (3 findings)

Finding File Severity Legitimate?
Redis dedupe guard is check-then-act (TOCTOU): concurrent workers can both pass it and emit duplicate occurrences; a crash between emit and state commit reprocesses the packet detector.py:58 MEDIUM ✓ Real race, listed as a legitimate unscored finding in the report
test_state_results_multi_group passes value=6 for group_2 while the packet value is 10 — the test wouldn't catch a wrong per-group value test_detector.py:192 LOW ✓ Accurate test-fidelity point
Stale TODO comment ("will be a stateful detector") directly above a class that already extends StatefulDetectorHandler grouptype.py:9 LOW ✓ Accurate; the comment became stale with this PR

Honest Analysis

FriendlyReviewer found both golden comments — 100% recall — on a dataset of only two goldens. Both hits are precise: golden #1 (the placeholder MetricAlertDetectorHandler left abstract → TypeError on instantiation) matches the grouptype.py L11 finding with the same mechanism and the same remediation, and golden #2 (the stale evaluate docstring, detector.py L225) matches essentially word for word.

The review's best unscored finding is the Redis dedupe TOCTOU (detector.py L58): the guard is check-then-act — dedupe_value is read before evaluation, the new value is written only later in _bulk_commit_redis_state, and the occurrence is pushed to Kafka before commit_state_updates. Concurrent workers can both pass the guard and emit duplicate occurrences, and a crash between emit and state commit reprocesses the packet. It is more consequential than golden #2; the dataset simply has no golden for it.

Weaknesses: five findings is a light haul for a feature touching the detector pipeline, and the two LOW test-fidelity items (mock value=6 vs packet value 10, the stale TODO comment) are nit-level. The review is also mildly self-contradictory — 'test coverage is good' in the summary, then a coverage gap in the checklist — and the checklist's 'Partially covered' verdict on the dedupe goal reads more like a design objection than a PR defect.

Honest bottom line: the 100% recall scores the two dataset goldens correctly, and the TOCTOU finding shows depth beyond the dataset. Small PRs with well-curated goldens are where FR looks best, provided the perfect score is not read as perfection.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
1 high, 1 medium, 3 low 1 ok, 1 mitigated

MR implements the new occurrence-production hook and switches evaluate to return a dict keyed by group key, which dedupes within a single evaluation. The hook implementation is sound and test coverage is good. However, the new abstract hook makes the placeholder MetricAlertDetectorHandler uninstantiable, which will crash process_detectors for metric-alert fire detectors; the Redis dedupe guard still has a TOCTOU race that can emit duplicate occurrences across workers; and a few doc/test fidelity issues remain. Overall the change requires follow-up before merge.

These might need a close look

  • 🔴 src/sentry/incidents/grouptype.py (L11)
    The new abstract method build_occurrence_and_event_data in StatefulDetectorHandler (detector.py) leaves the placeholder MetricAlertDetectorHandler abstract and uninstantiable, causing Detector.detector_handler to raise TypeError for any detector of type metric_alert_fire. Provide stub implementations of the abstract members or leave detector_handler unset on MetricAlertFire until the companion stateful implementation lands.

Worth checking

  • 🟡 src/sentry/workflow_engine/processors/detector.py (L58)
    The Redis dedupe guard is check-then-act: dedupe_value is read before evaluation, the new value is written only later in _bulk_commit_redis_state, and the occurrence is pushed to Kafka before commit_state_updates. Concurrent workers can both pass the guard and emit duplicate occurrences, and a crash between emit and state commit reprocesses the packet. Make the dedupe check atomic (e.g. SETNX/compare-and-swap) or commit state before emitting.
Small things (take or leave)
  • 🔵 src/sentry/workflow_engine/processors/detector.py (L225)
    The evaluate docstring still says it returns a list of DetectorEvaluationResult, but the method now returns a dict keyed by group key. Update the docstring to match the new contract.
  • 🔵 tests/sentry/workflow_engine/processors/test_detector.py (L192)
    In test_state_results_multi_group, build_mock_occurrence_and_event is called with value=6 for group_2 while the packet value is 10. The current mock ignores value, so the test passes, but it would not catch a regression where the hook receives the wrong per-group value. Pass the real packet value (10) for group_2 to make the test faithful to the hook contract.
  • 🔵 src/sentry/incidents/grouptype.py (L9)
    The comment # TODO: This will be a stateful detector when we build that abstraction is now stale because the class directly below it already extends StatefulDetectorHandler. Reword it to describe the remaining work (implementing the abstract occurrence/state methods) rather than implying the abstraction still needs to be built.

✅ Feature-level checklist

Looks good

  • ✅ Add a hook that can be implemented to produce an occurrence specific to the detector subclassing the StatefulDetector.
    The build_occurrence_and_event_data abstract hook is added and flows through DetectorEvaluationResult into occurrence creation. The hook is only invoked when status is not OK, and the priority conversion is type-safe. The placeholder handler issue is separate from the hook's existence.

Partially covered

  • ⚠️ Change the signature of evaluate to return a dict keyed by groupkey instead of a list, to avoid duplicate results for the same group key.
    evaluate now returns a dict keyed by DetectorGroupKey, deduplicating results within a single evaluation. However, the cross-worker Redis dedupe guard is still a check-then-act race that can produce duplicate occurrences for the same group key, so the stated dedupe goal is only partially satisfied.