ref(crons): Reorganize incident creation / issue occurrence logic - #1
Open
linxia0415 wants to merge 1 commit into
Open
ref(crons): Reorganize incident creation / issue occurrence logic#1linxia0415 wants to merge 1 commit into
linxia0415 wants to merge 1 commit into
Conversation
Since we'll be doing more with issue occurrences split out the concept of incidents into it's own logic module, as well as incident_occurrence into it's own module
📝 WalkthroughWalkthroughThis pull request refactors monitor incident threshold logic by extracting it from ChangesMonitor incident occurrence extraction and orchestration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/monitors/logic/incident_occurrence.py (1)
145-146: 💤 Low valueOptional: simplify single-element access (RUF015).
list(status_counts.keys())[0]builds a throwaway list; prefernext(iter(...)).♻️ Proposed tweak
- return SINGULAR_HUMAN_FAILURE_MAP[list(status_counts.keys())[0]] + return SINGULAR_HUMAN_FAILURE_MAP[next(iter(status_counts.keys()))]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/monitors/logic/incident_occurrence.py` around lines 145 - 146, Replace the throwaway list creation when pulling the single key from status_counts: instead of list(status_counts.keys())[0] use next(iter(status_counts.keys())) (or next(iter(status_counts))) in the same conditional branch where you return SINGULAR_HUMAN_FAILURE_MAP[...] (inside the incident occurrence logic handling the single-element case) so you avoid allocating a temporary list and achieve the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sentry/monitors/logic/incident_occurrence.py`:
- Around line 159-171: get_monitor_environment_context copies
monitor_environment.monitor.config into a local variable config and mutates it,
but then returns monitor_environment.monitor.config instead of the modified
config; update the return value in get_monitor_environment_context so the
"config" field uses the local config variable (i.e., "config": config) so the
human-readable schedule_type substitution
(monitor_environment.monitor.get_schedule_type_display()) is propagated to the
emitted context.
---
Nitpick comments:
In `@src/sentry/monitors/logic/incident_occurrence.py`:
- Around line 145-146: Replace the throwaway list creation when pulling the
single key from status_counts: instead of list(status_counts.keys())[0] use
next(iter(status_counts.keys())) (or next(iter(status_counts))) in the same
conditional branch where you return SINGULAR_HUMAN_FAILURE_MAP[...] (inside the
incident occurrence logic handling the single-element case) so you avoid
allocating a temporary list and achieve the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cceeadb9-9d67-45cd-a8ff-ac211401a86f
📒 Files selected for processing (4)
src/sentry/monitors/logic/incident_occurrence.pysrc/sentry/monitors/logic/incidents.pysrc/sentry/monitors/logic/mark_failed.pysrc/sentry/monitors/types.py
Comment on lines
+159
to
+171
| def get_monitor_environment_context(monitor_environment: MonitorEnvironment): | ||
| config = monitor_environment.monitor.config.copy() | ||
| if "schedule_type" in config: | ||
| config["schedule_type"] = monitor_environment.monitor.get_schedule_type_display() | ||
|
|
||
| return { | ||
| "id": str(monitor_environment.monitor.guid), | ||
| "slug": str(monitor_environment.monitor.slug), | ||
| "name": monitor_environment.monitor.name, | ||
| "config": monitor_environment.monitor.config, | ||
| "status": monitor_environment.get_status_display(), | ||
| "type": monitor_environment.monitor.get_type_display(), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
git log -p -S 'get_monitor_environment_context' -- src/sentry/monitors/logic/mark_failed.py | grep -nA20 'def get_monitor_environment_context' | head -60Repository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 86
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the full current implementation of get_monitor_environment_context
rg -n "def get_monitor_environment_context" -S .
FILE="$(rg -n "def get_monitor_environment_context" -S . | head -n1 | cut -d: -f1)"
echo "== file: $FILE =="
sed -n '1,260p' "$FILE" | nl -ba | sed -n '120,220p'
# 2) Find where schedule_type is stored/used
echo "== schedule_type references (limited) =="
rg -n "schedule_type" src/sentry/monitors -S | head -n 80
# 3) Inspect get_schedule_type_display implementation
echo "== get_schedule_type_display implementation =="
rg -n "def get_schedule_type_display" -S src/sentry | head -n 50
# print surrounding code for the first match (if any)
DISP_FILE="$(rg -n "def get_schedule_type_display" -S src/sentry | head -n1 | cut -d: -f1)"
if [ -n "${DISP_FILE:-}" ]; then
sed -n '1,260p' "$DISP_FILE" | nl -ba | sed -n '1,260p' | head -n 220
fi
# 4) Inspect MonitorEnvironment / monitor.config types if available
echo "== monitor_environment.monitor.config usage =="
rg -n "monitor_environment\.monitor\.config" -S src/sentry | head -n 50
# 5) History: try locating the function in git history anywhere (not just mark_failed.py)
echo "== git history for get_monitor_environment_context across repo =="
git log -p -S 'get_monitor_environment_context' -- src/sentry/monitors/logic 2>/dev/null | head -n 200Repository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(rg -n "def get_monitor_environment_context" -S src/sentry | head -n1 | cut -d: -f1)"
echo "== get_monitor_environment_context file: $FILE =="
sed -n '130,220p' "$FILE" | cat -n
echo "== schedule_type references (limited) =="
rg -n "schedule_type" src/sentry/monitors -S | head -n 80
echo "== get_schedule_type_display implementation =="
DISP_FILE="$(rg -n "def get_schedule_type_display" -S src/sentry | head -n1 | cut -d: -f1 || true)"
if [ -n "${DISP_FILE:-}" ]; then
echo "== file: $DISP_FILE =="
sed -n '1,260p' "$DISP_FILE" | cat -n | head -n 220
else
echo "No def get_schedule_type_display found under src/sentry"
fi
echo "== monitor_environment.monitor.config usage (first 50 matches) =="
rg -n "monitor_environment\.monitor\.config" -S src/sentry | head -n 50 || true
echo "== git history for get_monitor_environment_context (any path) =="
git log -p -S 'get_monitor_environment_context' -- src/sentry/monitors/logic 2>/dev/null | head -n 200 || trueRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 14210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect monitors/serializers.py around where schedule_type display substitution happens
SER_FILE="src/sentry/monitors/serializers.py"
if [ -f "$SER_FILE" ]; then
sed -n '200,280p' "$SER_FILE" | cat -n
fi
# 2) Find usages of get_monitor_environment_context
rg -n "get_monitor_environment_context\(" -S src/sentry | head -n 50
# 3) If there is an event schema/consumer expecting config.schedule_type as string, find it
rg -n "\"config\".*schedule_type|schedule_type.*config" -S src/sentry/monitors src/sentry/sentry* 2>/dev/null | head -n 80 || true
# 4) Search for Kafka event payload builder/contract area around incident occurrence
rg -n "incident_occurrence|get_monitor_environment_context|IssueOccurrence|event_data|kafka" -S src/sentry/monitors/logic/incident_occurrence.py | cat -n
# 5) Check ruff warning RUF015 definition (best-effort): find ruff config mentioning RUF015
if [ -f ".ruff.toml" ]; then
rg -n "RUF015" .ruff.toml || true
fi
if [ -f "pyproject.toml" ]; then
rg -n "RUF015" pyproject.toml || true
fiRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 4731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate mark_failed.py (if present) and any other similar history targets
echo "== mark_failed.py files =="
git ls-files | rg -n "mark_failed\.py" || true
# 2) Show current incident_occurrence.py around create_incident_occurrence and context usage
sed -n '1,220p' src/sentry/monitors/logic/incident_occurrence.py | cat -n
# 3) Find any code that reads event contexts["monitor"] / "contexts"."monitor"
echo "== reads of contexts monitor in codebase (best-effort) =="
rg -n "\"contexts\"\\s*\\[\"monitor\"\\]|contexts\"\\]\\[\"monitor\"\\]|contexts\\]\\[\\\"monitor\\\"\\]|contexts\\.monitor" -S src/sentry | head -n 80 || true
echo "== reads of get_monitor_environment_context result shape (contexts monitor) =="
rg -n "get_status_display\\(\\)|get_type_display\\(\\)|schedule_type\"\\s*:\\s*|schedule_type" -S src/sentry/sentry* src/sentry | head -n 80 || true
# 4) History search: look for schedule_type substitution inside get_monitor_environment_context in git history
echo "== git history for incident_occurrence context function (best-effort, limited) =="
git log -p -S 'def get_monitor_environment_context' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || true
git log -p -S '"config": monitor_environment.monitor.config' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || true
git log -p -S '"schedule_type" = monitor_environment.monitor.get_schedule_type_display' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || trueRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 13484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect mark_failed.py implementation (likely previous/parallel code path)
MARK_FILE="src/sentry/monitors/logic/mark_failed.py"
if [ -f "$MARK_FILE" ]; then
echo "== mark_failed.py get_monitor_environment_context =="
sed -n '1,260p' "$MARK_FILE" | cat -n | rg -n "def get_monitor_environment_context|get_monitor_environment_context" || true
# Show neighborhood around the function if present
FN_LINE="$(rg -n "def get_monitor_environment_context" "$MARK_FILE" | head -n1 | cut -d: -f1 || true)"
if [ -n "${FN_LINE:-}" ]; then
START=$((FN_LINE-30))
END=$((FN_LINE+60))
sed -n "${START},${END}p" "$MARK_FILE" | cat -n
fi
echo "== mark_failed.py schedule_type display usage =="
rg -n "schedule_type.*get_schedule_type_display|get_schedule_type_display\(" "$MARK_FILE" || true
fi
# 2) Find consumers that read contexts.monitor.config or specifically schedule_type from the event contexts
echo "== Reads of contexts.monitor.config (best-effort) =="
rg -n "contexts\.(monitor\.config|monitor\['config'\]|monitor\]\['config'\]|monitor\"\s*:\s*\{[^}]*config|contexts.*monitor.*config)" -S src/sentry tests | head -n 80 || true
echo "== Reads of schedule_type from monitor context (best-effort) =="
rg -n "contexts.*monitor.*schedule_type|schedule_type.*contexts.*monitor" -S src/sentry tests | head -n 80 || true
# 3) git history search for the specific return config key/value
echo "== git history: config returned from get_monitor_environment_context =="
git log -p -S '"config": monitor_environment.monitor.config' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || true
git log -p -S '"config": config' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || true
git log -p -S 'get_schedule_type_display' -- src/sentry/monitors/logic/incident_occurrence.py 2>/dev/null | head -n 200 || trueRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for incident/monitor failure event payload usage of schedule_type in contexts =="
rg -n "contexts.*monitor.*schedule_type|schedule_type.*contexts.*monitor|monitor.*config.*schedule_type|config.*schedule_type" -S src/sentry tests | head -n 120
echo "== Search for get_monitor_environment_context in older/other files =="
rg -n "get_monitor_environment_context" -S src/sentry tests
echo "== Inspect analytics monitor_mark_failed.py for monitor context shape =="
FILE="src/sentry/analytics/events/monitor_mark_failed.py"
if [ -f "$FILE" ]; then
rg -n "contexts|monitor" "$FILE" | head -n 120
sed -n '1,260p' "$FILE" | cat -n
fi
echo "== Inspect monitors/logic/mark_failed.py for any monitor context building =="
FILE="src/sentry/monitors/logic/mark_failed.py"
if [ -f "$FILE" ]; then
rg -n "contexts|monitor.*context|get_monitor_environment_context" "$FILE" | head -n 120
sed -n '1,340p' "$FILE" | cat -n
fiRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 6195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for literal 'contexts': {'monitor' usages =="
rg -n "\"contexts\"\\s*:\\s*\\{\\s*\"monitor\"|\"contexts\"\\s*:\\s*{\\s*\"monitor\"" -S src tests | head -n 80 || true
echo "== Search for contexts.monitor.config reads =="
rg -n "contexts\\.monitor\\.config|contexts\\]\\[\"monitor\"\\].*config|contexts\\]\\['monitor'\\].*config|contexts\\.monitor.*config" -S src tests | head -n 120 || true
echo "== Search for schedule_type inside contexts.monitor.config reads =="
rg -n "monitor.*config.*schedule_type|contexts\\.monitor\\.config.*schedule_type|contexts\\]\\[\"monitor\"\\].*schedule_type|contexts\\]\\['monitor'\\].*schedule_type" -S src tests | head -n 120 || true
echo "== Search for consumers of IssueOccurrence event contexts =="
rg -n "IssueOccurrence|produce_occurrence_to_kafka|event_data\\[\"contexts\"\\]" -S src | head -n 80
echo "== Search for 'monitor' context key usage in issue/occurrence handling =="
rg -n "\"monitor\"\\s*:\\s*get_monitor_environment_context|contexts\"\\]\\[\"monitor\"\\]|contexts\\\"\\]\\[\\\"monitor\\\"\\]" -S src | head -n 80 || trueRepository: test-code-tool/sentry__sentry__coderabbit__PR80528__20260407
Length of output: 9627
Fix dead code in get_monitor_environment_context: return the modified config
The function copies monitor_environment.monitor.config into config and replaces schedule_type with monitor_environment.monitor.get_schedule_type_display(), but the returned context still uses monitor_environment.monitor.config, so the human-readable substitution never reaches the emitted contexts.monitor.config.
🐛 Proposed fix
return {
"id": str(monitor_environment.monitor.guid),
"slug": str(monitor_environment.monitor.slug),
"name": monitor_environment.monitor.name,
- "config": monitor_environment.monitor.config,
+ "config": config,
"status": monitor_environment.get_status_display(),
"type": monitor_environment.monitor.get_type_display(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def get_monitor_environment_context(monitor_environment: MonitorEnvironment): | |
| config = monitor_environment.monitor.config.copy() | |
| if "schedule_type" in config: | |
| config["schedule_type"] = monitor_environment.monitor.get_schedule_type_display() | |
| return { | |
| "id": str(monitor_environment.monitor.guid), | |
| "slug": str(monitor_environment.monitor.slug), | |
| "name": monitor_environment.monitor.name, | |
| "config": monitor_environment.monitor.config, | |
| "status": monitor_environment.get_status_display(), | |
| "type": monitor_environment.monitor.get_type_display(), | |
| } | |
| def get_monitor_environment_context(monitor_environment: MonitorEnvironment): | |
| config = monitor_environment.monitor.config.copy() | |
| if "schedule_type" in config: | |
| config["schedule_type"] = monitor_environment.monitor.get_schedule_type_display() | |
| return { | |
| "id": str(monitor_environment.monitor.guid), | |
| "slug": str(monitor_environment.monitor.slug), | |
| "name": monitor_environment.monitor.name, | |
| "config": config, | |
| "status": monitor_environment.get_status_display(), | |
| "type": monitor_environment.monitor.get_type_display(), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/monitors/logic/incident_occurrence.py` around lines 159 - 171,
get_monitor_environment_context copies monitor_environment.monitor.config into a
local variable config and mutates it, but then returns
monitor_environment.monitor.config instead of the modified config; update the
return value in get_monitor_environment_context so the "config" field uses the
local config variable (i.e., "config": config) so the human-readable
schedule_type substitution
(monitor_environment.monitor.get_schedule_type_display()) is propagated to the
emitted context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since we'll be doing more with issue occurrences split out the concept
of incidents into it's own logic module, as well as incident_occurrence
into it's own module
Part of GH-80527
Summary by CodeRabbit
New Features
Refactor