Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion migrations_lockfile.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ hybridcloud: 0024_add_project_distribution_scope

insights: 0002_backfill_team_starred

monitors: 0011_backfill_monitor_environment_is_muted
monitors: 0012_remove_monitor_is_muted_field

nodestore: 0001_squashed_0002_nodestore_no_dictfield

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ def update_monitor_environment(
is_muted = request.data.get("isMuted")
if type(is_muted) is bool:
monitor_environment.update(is_muted=is_muted)
# Dual-write: Sync is_muted back to monitor if all environments are muted
monitor.ensure_is_muted()

self.create_audit_entry(
request=request,
Expand Down
37 changes: 32 additions & 5 deletions src/sentry/monitors/endpoints/organization_monitor_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,17 @@ def get(self, request: AuthenticatedHttpRequest, organization: Organization) ->
sort_fields = []

if sort == "status":
# Check if all environments are muted by seeing if any unmuted environments exist
has_unmuted_env = monitor_environments_query.filter(is_muted=False)

queryset = queryset.annotate(
environment_status_ordering=Case(
# Sort DISABLED and is_muted monitors to the bottom of the list
# Sort DISABLED and fully muted monitors to the bottom of the list
When(status=ObjectStatus.DISABLED, then=Value(len(DEFAULT_STATUS_ORDER) + 1)),
When(is_muted=True, then=Value(len(DEFAULT_STATUS_ORDER))),
When(
Exists(monitor_environments_query) & ~Exists(has_unmuted_env),
then=Value(len(DEFAULT_STATUS_ORDER)),
),
default=Subquery(
monitor_environments_query.annotate(
status_ordering=MONITOR_ENVIRONMENT_ORDERING
Expand All @@ -169,10 +175,21 @@ def get(self, request: AuthenticatedHttpRequest, organization: Organization) ->
elif sort == "name":
sort_fields = ["name"]
elif sort == "muted":
# Check if any environments are muted
has_muted_env = monitor_environments_query.filter(is_muted=True)
# Check if all environments are muted
has_unmuted_env = monitor_environments_query.filter(is_muted=False)

queryset = queryset.annotate(
muted_ordering=Case(
When(is_muted=True, then=Value(2)),
When(Exists(monitor_environments_query.filter(is_muted=True)), then=Value(1)),
# No environments muted (or no environments at all)
When(~Exists(has_muted_env), then=Value(0)),
# Some environments muted (not all)
When(Exists(has_muted_env) & Exists(has_unmuted_env), then=Value(1)),
# All environments muted (and at least one environment exists)
When(
Exists(monitor_environments_query) & ~Exists(has_unmuted_env), then=Value(2)
),
default=0,
),
)
Expand Down Expand Up @@ -315,6 +332,9 @@ def put(self, request: AuthenticatedHttpRequest, organization) -> Response:
if not assign_result.assignable:
return self.respond(assign_result.reason, status=400)

# Extract is_muted to propagate to environments, don't update Monitor directly
is_muted = result.pop("is_muted", None)

updated = []
errored = []
for monitor in monitors:
Expand All @@ -330,7 +350,14 @@ def put(self, request: AuthenticatedHttpRequest, organization) -> Response:
if status == ObjectStatus.DISABLED:
quotas.backend.disable_seat(DataCategory.MONITOR_SEAT, monitor)

monitor.update(**result)
# Propagate is_muted to all monitor environments
if is_muted is not None:
MonitorEnvironment.objects.filter(monitor_id=monitor.id).update(
is_muted=is_muted
)

if result:
monitor.update(**result)
updated.append(monitor)
self.create_audit_entry(
request=request,
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/monitors/logic/incidents.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ def try_incident_threshold(

# Only create an occurrence if:
# - We have an active incident and fingerprint
# - The monitor and env are not muted
if not monitor_env.monitor.is_muted and not monitor_env.is_muted and incident:
# - The environment is not muted
if not monitor_env.is_muted and incident:
checkins = list(MonitorCheckIn.objects.filter(id__in=[c.id for c in previous_checkins]))
for checkin in checkins:
dispatch_incident_occurrence(checkin, checkins, incident, received, clock_tick)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Generated by Django 5.2.1 on 2025-11-13 19:47

from sentry.new_migrations.migrations import CheckedMigration
from sentry.new_migrations.monkey.fields import SafeRemoveField
from sentry.new_migrations.monkey.state import DeletionAction


class Migration(CheckedMigration):
# This flag is used to mark that a migration shouldn't be automatically run in production.
# This should only be used for operations where it's safe to run the migration after your
# code has deployed. So this should not be used for most operations that alter the schema
# of a table.
# Here are some things that make sense to mark as post deployment:
# - Large data migrations. Typically we want these to be run manually so that they can be
# monitored and not block the deploy for a long period of time while they run.
# - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
# run this outside deployments so that we don't block them. Note that while adding an index
# is a schema change, it's completely safe to run the operation after the code has deployed.
# Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment

is_post_deployment = False

dependencies = [
("monitors", "0011_backfill_monitor_environment_is_muted"),
]

operations = [
SafeRemoveField(
model_name="monitor",
name="is_muted",
deletion_action=DeletionAction.MOVE_TO_PENDING,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
]
27 changes: 10 additions & 17 deletions src/sentry/monitors/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,12 +238,6 @@ class Monitor(Model):
check-in payloads. The slug can be changed.
"""

is_muted = models.BooleanField(default=False, db_default=False)
"""
Monitor is operating normally but will not produce incidents or produce
occurrences into the issues platform.
"""

name = models.CharField(max_length=128)
"""
Human readable name of the monitor. Used for display purposes.
Expand Down Expand Up @@ -439,23 +433,22 @@ def normalize_before_relocation_import(
self.guid = uuid4()
return old_pk

def ensure_is_muted(self) -> None:
@property
def is_muted(self) -> bool:
Comment on lines +436 to +437
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a cached property? Currently this will execute queries each time the property is accessed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Dual-write: Sync is_muted from monitor environments back to the monitor.

Sets Monitor.is_muted based on whether all MonitorEnvironments are muted.
If there are no environments, is_muted remains unchanged.
A monitor is considered muted if ALL of its environments are muted.
If a monitor has no environments, it is considered unmuted.
"""
# Count total environments and muted environments
env_counts = MonitorEnvironment.objects.filter(monitor_id=self.id).aggregate(
total=models.Count("id"), muted=models.Count("id", filter=Q(is_muted=True))
)

# 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:
Comment on lines 444 to 454
Copy link

@sentry sentry bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
3 changes: 0 additions & 3 deletions src/sentry/monitors/tasks/detect_broken_monitor_envs.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ def build_open_incidents_queryset():
resolving_checkin=None,
starting_timestamp__lte=(current_time - timedelta(days=NUM_DAYS_BROKEN_PERIOD)),
monitor__status=ObjectStatus.ACTIVE,
monitor__is_muted=False,
monitor_environment__is_muted=False,
monitorenvbrokendetection__env_muted_timestamp=None,
)
Expand Down Expand Up @@ -243,8 +242,6 @@ def detect_broken_monitor_envs_for_org(org_id: int):
with transaction.atomic(router.db_for_write(MonitorEnvBrokenDetection)):
open_incident.monitor_environment.update(is_muted=True)
detection.update(env_muted_timestamp=django_timezone.now())
# Dual-write: Sync is_muted back to monitor if all environments are muted
open_incident.monitor.ensure_is_muted()

for email in get_user_emails_from_monitor(open_incident.monitor, project):
if not email:
Expand Down
12 changes: 5 additions & 7 deletions src/sentry/monitors/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,6 @@ def create(self, validated_data):
name=validated_data["name"],
slug=validated_data.get("slug"),
status=validated_data["status"],
is_muted=validated_data.get("is_muted", False),
config=validated_data["config"],
)

Expand Down Expand Up @@ -459,6 +458,11 @@ def update(self, instance, validated_data):
):
quotas.backend.disable_seat(DataCategory.MONITOR_SEAT, instance)

# Forward propagate is_muted to all monitor environments when changed
is_muted = params.pop("is_muted", None)
if is_muted is not None:
MonitorEnvironment.objects.filter(monitor_id=instance.id).update(is_muted=is_muted)

if params:
instance.update(**params)
create_audit_entry(
Expand All @@ -469,12 +473,6 @@ def update(self, instance, validated_data):
data=instance.get_audit_log_data(),
)

# Dual-write is_muted to all monitor environments when changed
if "is_muted" in params:
MonitorEnvironment.objects.filter(monitor_id=instance.id).update(
is_muted=params["is_muted"]
)

# Update monitor slug in billing
if "slug" in params:
quotas.backend.update_monitor_slug(existing_slug, params["slug"], instance.project_id)
Expand Down
3 changes: 1 addition & 2 deletions tests/sentry/monitors/clock_tasks/test_check_missed.py
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,6 @@ def assert_state_does_not_change_for_status(
"max_runtime": None,
},
status=state,
is_muted=is_muted,
)
# Expected checkin was a full minute ago, if this monitor wasn't in the
# `state` the monitor would usually end up marked as timed out
Expand All @@ -469,7 +468,7 @@ def assert_state_does_not_change_for_status(
next_checkin=ts - timedelta(minutes=1),
next_checkin_latest=ts,
status=MonitorStatus.ACTIVE,
is_muted=environment_is_muted,
is_muted=is_muted or environment_is_muted,
)

dispatch_check_missing(ts)
Expand Down
13 changes: 10 additions & 3 deletions tests/sentry/monitors/consumers/test_monitor_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,16 +293,23 @@ def test_failing(self) -> None:
)

def test_muted(self) -> None:
monitor = self._create_monitor(is_muted=True)
monitor = self._create_monitor()
# Create a muted environment for this monitor
production_env = self.create_environment(name="production")
MonitorEnvironment.objects.create(
monitor=monitor,
environment_id=production_env.id,
is_muted=True,
)
self.send_checkin(monitor.slug, status="error")

checkin = MonitorCheckIn.objects.get(guid=self.guid)
assert checkin.status == CheckInStatus.ERROR

monitor_environment = MonitorEnvironment.objects.get(id=checkin.monitor_environment.id)

# The created monitor environment is in line with the check-in, but the
# parent monitor is muted
# The monitor environment should still be muted and track the error status
assert monitor_environment.is_muted is True
assert monitor_environment.status == MonitorStatus.ERROR
assert monitor_environment.last_checkin == checkin.date_added
assert monitor_environment.next_checkin == monitor.get_next_expected_checkin(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,9 @@ def test_slug_same(self) -> None:

def test_can_mute(self) -> None:
monitor = self._create_monitor()
# Create an environment so the monitor has an environment to mute
self._create_monitor_environment(monitor)

resp = self.get_success_response(
self.organization.slug, monitor.slug, method="PUT", **{"isMuted": True}
)
Expand All @@ -355,8 +358,8 @@ def test_can_mute(self) -> None:

def test_can_unmute(self) -> None:
monitor = self._create_monitor()

monitor.update(is_muted=True)
# Create a muted environment so the monitor is muted
self._create_monitor_environment(monitor, is_muted=True)

resp = self.get_success_response(
self.organization.slug, monitor.slug, method="PUT", **{"isMuted": False}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,12 @@ def test_muting_all_environments_mutes_monitor(self) -> None:

def test_unmuting_one_environment_unmutes_monitor(self) -> None:
"""Test that unmuting one environment when all were muted also unmutes the monitor."""
# Start with a muted monitor
# Start with a monitor that has all environments muted
monitor = self._create_monitor(status=MonitorStatus.ACTIVE)
monitor.update(is_muted=True)

# Create two muted environments
env1 = self._create_monitor_environment(monitor, name="production")
env1.update(is_muted=True)
env2 = self._create_monitor_environment(monitor, name="staging")
env2.update(is_muted=True)
env1 = self._create_monitor_environment(monitor, name="production", is_muted=True)
env2 = self._create_monitor_environment(monitor, name="staging", is_muted=True)

# Verify initial state
monitor.refresh_from_db()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ def test_create_monitor_with_optional_fields(self):
"name": "Full Config Monitor",
"slug": "full-config-monitor",
"status": "disabled",
"isMuted": True,
"isMuted": False,
"config": {
"schedule": "*/30 * * * *",
"scheduleType": "crontab",
Expand All @@ -205,7 +205,7 @@ def test_create_monitor_with_optional_fields(self):
)
assert monitor.name == "Full Config Monitor"
assert monitor.status == ObjectStatus.DISABLED
assert monitor.is_muted is True
assert monitor.is_muted is False
assert monitor.config["schedule"] == "*/30 * * * *"
assert monitor.config["checkin_margin"] == 15
assert monitor.config["max_runtime"] == 120
Expand Down
Loading
Loading