Refactor monitor failure handling into dedicated modules - #2
Open
smb060606 wants to merge 1 commit into
Open
Conversation
…0528) 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
WalkthroughThe changes refactor monitor failure handling by extracting incident creation and occurrence generation logic into dedicated modules. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sentry/monitors/logic/incidents.py (1)
52-53: Minor: Prefer generator expression insideany().Using a generator expression allows
any()to short-circuit without building the full list:- if any([checkin["status"] == CheckInStatus.OK for checkin in previous_checkins]): + if any(checkin["status"] == CheckInStatus.OK for checkin in previous_checkins):src/sentry/monitors/logic/incident_occurrence.py (1)
130-156: Address static analysis hint and minor typo.
- Line 132: Typo "humam" → "human" in docstring
- Line 146: Per Ruff RUF015, prefer
next(iter(...))over single-element slicedef get_failure_reason(failed_checkins: Sequence[SimpleCheckIn]): """ - Builds a humam readible string from a list of failed check-ins. + Builds a human readable string from a list of failed check-ins. "3 missed check-ins detected" "2 missed check-ins, 1 timeout check-in and 1 error check-in were detected" "A failed check-in was detected" """ status_counts = Counter( checkin["status"] for checkin in failed_checkins - if checkin["status"] in HUMAN_FAILURE_STATUS_MAP.keys() + if checkin["status"] in HUMAN_FAILURE_STATUS_MAP ) if sum(status_counts.values()) == 1: - return SINGULAR_HUMAN_FAILURE_MAP[list(status_counts.keys())[0]] + return SINGULAR_HUMAN_FAILURE_MAP[next(iter(status_counts.keys()))]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/sentry/monitors/logic/incident_occurrence.py(1 hunks)src/sentry/monitors/logic/incidents.py(1 hunks)src/sentry/monitors/logic/mark_failed.py(2 hunks)src/sentry/monitors/types.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/sentry/monitors/types.py (2)
src/sentry/sentry_apps/services/app/model.py (1)
id(112-112)src/sentry/eventstore/models.py (1)
datetime(112-120)
src/sentry/monitors/logic/incidents.py (2)
src/sentry/monitors/models.py (4)
CheckInStatus(124-176)MonitorCheckIn(472-582)MonitorIncident(747-795)MonitorStatus(80-121)src/sentry/monitors/types.py (1)
SimpleCheckIn(105-112)
src/sentry/monitors/logic/mark_failed.py (2)
src/sentry/monitors/logic/incidents.py (1)
try_incident_threshold(14-104)src/sentry/monitors/models.py (3)
CheckInStatus(124-176)MonitorCheckIn(472-582)MonitorEnvironment(644-724)
src/sentry/monitors/logic/incident_occurrence.py (4)
src/sentry/monitors/models.py (7)
timezone(340-341)CheckInStatus(124-176)MonitorCheckIn(472-582)MonitorEnvironment(644-724)MonitorIncident(747-795)owner_actor(324-325)get_schedule_type_display(343-344)src/sentry/issues/grouptype.py (1)
MonitorIncidentType(528-536)src/sentry/monitors/types.py (1)
SimpleCheckIn(105-112)src/sentry/issues/producer.py (2)
PayloadType(25-34)produce_occurrence_to_kafka(50-90)
🪛 Ruff (0.14.8)
src/sentry/monitors/logic/incident_occurrence.py
146-146: Prefer next(iter(status_counts.keys())) over single element slice
Replace with next(iter(status_counts.keys()))
(RUF015)
🔇 Additional comments (4)
src/sentry/monitors/types.py (1)
105-113: LGTM!Clean, well-documented TypedDict that provides a minimal representation for check-in data. The fields align correctly with the usage in
incidents.pywhere it's populated from both direct model access and.values()queries.src/sentry/monitors/logic/mark_failed.py (1)
14-79: LGTM - Clean separation of concerns.The refactoring properly separates responsibilities:
mark_failedhandles timing updates and MonitorEnvironment statetry_incident_thresholdhandles incident creation logicThe conditional update pattern at lines 52-63 correctly prevents race conditions with newer check-ins.
src/sentry/monitors/logic/incidents.py (1)
89-104: LGTM - Occurrence creation and signal emission.The logic correctly:
- Respects mute settings on both monitor and environment
- Re-fetches full
MonitorCheckInobjects needed bycreate_incident_occurrence- Sends the signal regardless of mute status (preserving original behavior)
src/sentry/monitors/logic/incident_occurrence.py (1)
28-113: LGTM - Well-structured occurrence generation.The function properly:
- Guards against missing
monitor_env- Constructs comprehensive evidence display
- Handles optional
trace_idandreceivedtimestamp- Uses lazy imports to avoid circular dependencies
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.
Bug: Modified config copy is not used in the returned dictionary.
The function creates a modified copy of config with a human-readable schedule_type, but line 168 returns the original monitor_environment.monitor.config instead of the modified config variable.
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(),
}🤖 Prompt for AI Agents
In src/sentry/monitors/logic/incident_occurrence.py around lines 159 to 171, the
function builds a modified copy named `config` (with a human-readable
`schedule_type`) but then returns the original
`monitor_environment.monitor.config`; change the returned dictionary to use the
modified `config` variable instead of `monitor_environment.monitor.config` so
the human-readable schedule_type is included in the output.
Comment on lines
+36
to
+59
| else: | ||
| previous_checkins = cast( | ||
| list[SimpleCheckIn], | ||
| # Using .values for performance reasons | ||
| MonitorCheckIn.objects.filter( | ||
| monitor_environment=monitor_env, date_added__lte=failed_checkin.date_added | ||
| ) | ||
| .order_by("-date_added") | ||
| .values("id", "date_added", "status"), | ||
| ) | ||
|
|
||
| # reverse the list after slicing in order to start with oldest check-in | ||
| previous_checkins = list(reversed(previous_checkins[:failure_issue_threshold])) | ||
|
|
||
| # If we have any successful check-ins within the threshold of | ||
| # commits we have NOT reached an incident state | ||
| if any([checkin["status"] == CheckInStatus.OK for checkin in previous_checkins]): | ||
| return False | ||
|
|
||
| # change monitor status + update fingerprint timestamp | ||
| monitor_env.status = MonitorStatus.ERROR | ||
| monitor_env.save(update_fields=("status",)) | ||
|
|
||
| starting_checkin = previous_checkins[0] |
There was a problem hiding this comment.
Potential IndexError when threshold exceeds available check-ins.
When failure_issue_threshold > 1 and the monitor environment has no prior check-ins (e.g., a brand new monitor that immediately fails), previous_checkins will be an empty list after slicing. The any() check on line 52 returns False for an empty list, so the function won't return early, and line 59 will raise an IndexError.
Consider adding a guard for this edge case:
# reverse the list after slicing in order to start with oldest check-in
previous_checkins = list(reversed(previous_checkins[:failure_issue_threshold]))
+ # Not enough check-ins to evaluate threshold
+ if not previous_checkins:
+ return False
+
# If we have any successful check-ins within the threshold of
# commits we have NOT reached an incident state
if any([checkin["status"] == CheckInStatus.OK for checkin in previous_checkins]):
return False📝 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
| else: | |
| previous_checkins = cast( | |
| list[SimpleCheckIn], | |
| # Using .values for performance reasons | |
| MonitorCheckIn.objects.filter( | |
| monitor_environment=monitor_env, date_added__lte=failed_checkin.date_added | |
| ) | |
| .order_by("-date_added") | |
| .values("id", "date_added", "status"), | |
| ) | |
| # reverse the list after slicing in order to start with oldest check-in | |
| previous_checkins = list(reversed(previous_checkins[:failure_issue_threshold])) | |
| # If we have any successful check-ins within the threshold of | |
| # commits we have NOT reached an incident state | |
| if any([checkin["status"] == CheckInStatus.OK for checkin in previous_checkins]): | |
| return False | |
| # change monitor status + update fingerprint timestamp | |
| monitor_env.status = MonitorStatus.ERROR | |
| monitor_env.save(update_fields=("status",)) | |
| starting_checkin = previous_checkins[0] | |
| else: | |
| previous_checkins = cast( | |
| list[SimpleCheckIn], | |
| # Using .values for performance reasons | |
| MonitorCheckIn.objects.filter( | |
| monitor_environment=monitor_env, date_added__lte=failed_checkin.date_added | |
| ) | |
| .order_by("-date_added") | |
| .values("id", "date_added", "status"), | |
| ) | |
| # reverse the list after slicing in order to start with oldest check-in | |
| previous_checkins = list(reversed(previous_checkins[:failure_issue_threshold])) | |
| # Not enough check-ins to evaluate threshold | |
| if not previous_checkins: | |
| return False | |
| # If we have any successful check-ins within the threshold of | |
| # commits we have NOT reached an incident state | |
| if any([checkin["status"] == CheckInStatus.OK for checkin in previous_checkins]): | |
| return False | |
| # change monitor status + update fingerprint timestamp | |
| monitor_env.status = MonitorStatus.ERROR | |
| monitor_env.save(update_fields=("status",)) | |
| starting_checkin = previous_checkins[0] |
🤖 Prompt for AI Agents
In src/sentry/monitors/logic/incidents.py around lines 36 to 59, guard against
previous_checkins being empty before accessing previous_checkins[0]; when the
sliced/reversed previous_checkins is empty (new monitor with immediate failure)
the any() check will be False but accessing index 0 will raise IndexError. Fix
by checking if previous_checkins is non-empty and using previous_checkins[0]
when present, otherwise set starting_checkin to the current failed_checkin (or
otherwise appropriate fallback), then continue updating monitor_env.status and
saving.
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.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.