Sentry / PR #8

ref(crons): Reorganize incident creation / issue occurrence logic

getsentry/sentry#80528
Golden recall: 50% Extra findings: 2

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The function modifies the config variable to include display values but then returns the original monitor.config instead of the modified version HIGH ✓ Found "In get_monitor_environment_context, the config copy and schedule_type rewrite is dead code: the returned dict uses the original monitor_environment.monitor.config instead of the modified config local, so consumers always receive the raw schedule_type integer. Fix: return "config": config" (incident_occurrence.py L168)
#2 The code fetches MonitorCheckIn objects by ID when the required data already exists in previous_checkins. This creates an unnecessary database query LOW ✗ Not found The only low findings are unused logger declarations (incidents.py L13, incident_occurrence.py L20); no candidate mentions the redundant DB query at incidents.py:93 (checkins = MonitorCheckIn.objects.filter(id__in=[c["id"] for c in previous_checkins])). The partial review likely contributed to this miss.

Supplementary Findings (2 findings)

Finding File Severity Legitimate?
Unused module-level logger = logging.getLogger(__name__) incidents.py:13 LOW ✓ Accurate dead code, minor
Unused module-level logger = logging.getLogger(__name__) incident_occurrence.py:20 LOW ✓ Accurate dead code, minor

Honest Analysis

Recall is 1/2 (50%), and the review body itself flags the cause: it opens with a "partial review" disclaimer, noting one sub-review was delivered from prose without a formal submit_review call. Three findings total is a thin pass for a refactor that moved incident and occurrence logic across modules.

The one hit is the right one. The dead config copy in get_monitor_environment_context (incident_occurrence.py L168) matches golden #1 exactly — the modified config local is never returned, so consumers always receive the raw schedule_type integer — with the same fix (return "config": config). FR rated it MEDIUM; the dataset rates it HIGH. The miss (golden #2, the redundant MonitorCheckIn DB query at incidents.py:93) is a Low-severity performance nit; no finding mentioned it, and the partial review plausibly contributed to the miss.

The two supplementary findings — unused logger declarations in both modules — are accurate but noise-level: dead code of the mildest kind. They confirm the review's shallowness on this PR. Honest bottom line: the 1/2 recall scores the review about right. The dataset is small (2 goldens), the HIGH-severity golden was found, and the refactor itself was correctly judged behavior-preserving. But this page is a demonstration of the partial-review failure mode more than of reviewer quality.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

⛅ Tech ☀️ Feat
1 medium, 2 low 1 ok

This is a partial review: one of the sub-reviews was delivered from prose without a formal submit_review call, so its findings may be less complete than a fully completed review. The refactor cleanly splits incident logic into incidents.py and incident_occurrence.py, moving SimpleCheckIn and related symbols to types.py without breaking imports or introducing cycles. The moved implementations preserve behavior. A small number of issues were flagged, all carried over from the original code or introduced as minor cleanliness items. The most notable is a dead-code defect in get_monitor_environment_context where the transformed config copy is never returned.

Worth checking

  • 🟡 src/sentry/monitors/logic/incident_occurrence.py (L168)
    In get_monitor_environment_context, the config copy and schedule_type rewrite is dead code: the returned dict uses the original monitor_environment.monitor.config instead of the modified config local, so consumers always receive the raw schedule_type integer. Fix: return "config": config.
Small things (take or leave)
  • 🔵 src/sentry/monitors/logic/incidents.py (L13)
    logger = logging.getLogger(__name__) is defined module-level but never used. Remove it or use it (e.g., log incident creation/threshold path) to match the pattern in mark_ok.py.
  • 🔵 src/sentry/monitors/logic/incident_occurrence.py (L20)
    logger = logging.getLogger(__name__) is defined module-level but never used. Remove it or log the produced occurrence for lint cleanliness.

✅ Feature-level checklist

Looks good

  • ✅ Split the concept of incidents into its own logic module, and incident_occurrence into its own module.
    The refactor successfully extracts incident and incident_occurrence logic into dedicated modules (incidents.py and incident_occurrence.py) and moves SimpleCheckIn to types.py. No dangling imports or cycles were found, and the moved logic preserves original behavior.