ref(crons): Remove Monitor.is_muted field, make it computed (stage 3) #103567
Conversation
[NEW-564: There needs to be some way to mute the entire cron detector](https://linear.app/getsentry/issue/NEW-564/there-needs-to-be-some-way-to-mute-the-entire-cron-detector) Stage 3 of the migration completes the transition to environment-based muting: - Removes the `Monitor.is_muted` database field - Converts `Monitor.is_muted` to a computed property that returns True only when ALL environments are muted - Removes dual-write synchronization logic from endpoints and tasks - Updates muting logic to propagate from monitor to environments - Updates sorting and filtering queries to use environment muting state - Updates all tests to use environment-based muting Migration 0012 removes the is_muted field from the Monitor model. The monitor is now considered muted if and only if all of its environments are muted. If a monitor has no environments, it is considered unmuted.
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| ) | ||
|
|
||
| # Only update if there are environments | ||
| if env_counts["total"] > 0: | ||
| all_muted = env_counts["total"] == env_counts["muted"] | ||
| if self.is_muted != all_muted: | ||
| self.update(is_muted=all_muted) | ||
| # If no environments exist, monitor is not muted | ||
| if env_counts["total"] == 0: | ||
| return False | ||
|
|
||
| # Monitor is muted only if ALL environments are muted | ||
| return env_counts["total"] == env_counts["muted"] | ||
|
|
||
|
|
||
| def check_organization_monitor_limit(organization_id: int) -> None: |
There was a problem hiding this comment.
Bug: N+1 query occurs when serializing a list of monitors because Monitor.is_muted property executes a database query for each monitor.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
When serializing a list of monitors, accessing the Monitor.is_muted property for each monitor triggers a separate database query. The is_muted property, defined in src/sentry/monitors/models.py lines 436-451, executes MonitorEnvironment.objects.filter(...).aggregate(...). The MonitorSerializer.serialize() method, called for each monitor in a list (e.g., by the GET /organizations/{org}/monitors/ endpoint), directly accesses obj.is_muted. This results in an N+1 query pattern, where N additional database queries are performed for the is_muted state, significantly degrading performance.
💡 Suggested Fix
To resolve the N+1 query, either compute is_muted from pre-fetched MonitorEnvironment data within MonitorSerializer.get_attrs() and pass it via attrs, or use database-level annotations to compute the muted state before serialization.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/monitors/models.py#L433-L454
Potential issue: When serializing a list of monitors, accessing the `Monitor.is_muted`
property for each monitor triggers a separate database query. The `is_muted` property,
defined in `src/sentry/monitors/models.py` lines 436-451, executes
`MonitorEnvironment.objects.filter(...).aggregate(...)`. The
`MonitorSerializer.serialize()` method, called for each monitor in a list (e.g., by the
`GET /organizations/{org}/monitors/` endpoint), directly accesses `obj.is_muted`. This
results in an N+1 query pattern, where N additional database queries are performed for
the `is_muted` state, significantly degrading performance.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2785913
|
This PR has a migration; here is the generated SQL for for --
-- Moved monitor.is_muted field to pending deletion state
--
-- (no-op) |
armenzg
left a comment
armenzg
left a comment
There was a problem hiding this comment.
I don't know enough about monitors to fully comprehend this, however, it seems a straightforward changes with lots reasonable coverage.
| SafeRemoveField( | ||
| model_name="monitor", | ||
| name="is_muted", | ||
| deletion_action=DeletionAction.MOVE_TO_PENDING, |
There was a problem hiding this comment.
Good 👍🏻
Remove the column and in the generated migration use SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING) to replace RemoveField(...). This only marks the state for the column as removed.
| @property | ||
| def is_muted(self) -> bool: |
There was a problem hiding this comment.
Do you want a cached property? Currently this will execute queries each time the property is accessed.
There was a problem hiding this comment.
I think for now I am OK with a brief N+1, but I'm going to get rid of this property after I think.
evanpurkhiser
deleted the
evanpurkhiser/ref-crons-remove-monitor-is-muted-field-make-it-computed-stage-3-
branch
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
NEW-564: There needs to be some way to mute the entire cron detector
Stage 3 of the migration completes the transition to environment-based muting:
Monitor.is_muteddatabase fieldMonitor.is_mutedto a computed property that returns True only when ALL environments are mutedMigration 0012 removes the is_muted field from the Monitor model.
The monitor is now considered muted if and only if all of its environments are muted. If a monitor has no environments, it is considered unmuted.