Skip to content

Commit 8e863ac

Browse files
ref(crons): Fix N+1 query in MonitorSerializer by removing is_muted property (#103693)
Removes the computed property Monitor.is_muted and replaces it with a standalone is_monitor_muted(monitor) function for better clarity and to fix an N+1 query issue in MonitorSerializer. The serializer now efficiently computes mute status for multiple monitors in a single pass by checking if any environment is unmuted, rather than calling the property on each monitor individually which would trigger separate queries.
1 parent 5a5345b commit 8e863ac

File tree

9 files changed

+126
-50
lines changed

9 files changed

+126
-50
lines changed

src/sentry/monitors/models.py

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ def get_audit_log_data(self):
325325
"name": self.name,
326326
"status": self.status,
327327
"config": self.config,
328-
"is_muted": self.is_muted,
328+
"is_muted": is_monitor_muted(self),
329329
"slug": self.slug,
330330
"owner_user_id": self.owner_user_id,
331331
"owner_team_id": self.owner_team_id,
@@ -433,22 +433,22 @@ def normalize_before_relocation_import(
433433
self.guid = uuid4()
434434
return old_pk
435435

436-
@property
437-
def is_muted(self) -> bool:
438-
"""
439-
A monitor is considered muted if ALL of its environments are muted.
440-
If a monitor has no environments, it is considered unmuted.
441-
"""
442-
env_counts = MonitorEnvironment.objects.filter(monitor_id=self.id).aggregate(
443-
total=models.Count("id"), muted=models.Count("id", filter=Q(is_muted=True))
444-
)
445436

446-
# If no environments exist, monitor is not muted
447-
if env_counts["total"] == 0:
448-
return False
437+
def is_monitor_muted(monitor: Monitor) -> bool:
438+
"""
439+
A monitor is considered muted if ALL of its environments are muted.
440+
If a monitor has no environments, it is considered unmuted.
441+
"""
442+
env_counts = MonitorEnvironment.objects.filter(monitor_id=monitor.id).aggregate(
443+
total=models.Count("id"), muted=models.Count("id", filter=Q(is_muted=True))
444+
)
445+
446+
# If no environments exist, monitor is not muted
447+
if env_counts["total"] == 0:
448+
return False
449449

450-
# Monitor is muted only if ALL environments are muted
451-
return env_counts["total"] == env_counts["muted"]
450+
# Monitor is muted only if ALL environments are muted
451+
return env_counts["total"] == env_counts["muted"]
452452

453453

454454
def check_organization_monitor_limit(organization_id: int) -> None:
@@ -637,7 +637,7 @@ def ensure_environment(
637637
monitor_env, created = MonitorEnvironment.objects.get_or_create(
638638
monitor=monitor,
639639
environment_id=environment.id,
640-
defaults={"status": MonitorStatus.ACTIVE, "is_muted": monitor.is_muted},
640+
defaults={"status": MonitorStatus.ACTIVE, "is_muted": is_monitor_muted(monitor)},
641641
)
642642

643643
# recompute per-project monitor check-in rate limit quota

src/sentry/monitors/serializers.py

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
from collections import defaultdict
22
from collections.abc import MutableMapping, Sequence
33
from datetime import datetime
4+
from itertools import groupby
5+
from operator import attrgetter
46
from typing import Any, Literal, TypedDict, cast
57

68
from django.db.models import prefetch_related_objects
@@ -198,6 +200,30 @@ def get_attrs(self, item_list, user, **kwargs):
198200
for actor, serialized_actor in zip(filtered_actors, actors_serialized)
199201
}
200202

203+
# Query ALL environments (unfiltered) to determine muted status
204+
# A monitor is muted only if ALL its environments are muted
205+
all_monitor_environments = (
206+
MonitorEnvironment.objects.filter(monitor__in=item_list)
207+
.exclude(
208+
status__in=[MonitorStatus.PENDING_DELETION, MonitorStatus.DELETION_IN_PROGRESS]
209+
)
210+
.order_by("monitor_id")
211+
)
212+
213+
# Group environments by monitor
214+
monitor_envs_by_id = {
215+
monitor_id: list(envs)
216+
for monitor_id, envs in groupby(all_monitor_environments, key=attrgetter("monitor_id"))
217+
}
218+
219+
# A monitor is muted only if it has environments AND all of them are muted
220+
is_muted_data = {
221+
item.id: bool(monitor_envs_by_id.get(item.id, []))
222+
and not any(not env.is_muted for env in monitor_envs_by_id.get(item.id, []))
223+
for item in item_list
224+
}
225+
226+
# Now query the filtered environments for serialization
201227
monitor_environments_qs = (
202228
MonitorEnvironment.objects.filter(monitor__in=item_list)
203229
.annotate(status_ordering=MONITOR_ENVIRONMENT_ORDERING)
@@ -213,6 +239,7 @@ def get_attrs(self, item_list, user, **kwargs):
213239

214240
monitor_environments = list(monitor_environments_qs)
215241
serialized_monitor_environments = defaultdict(list)
242+
216243
for monitor_env, serialized in zip(
217244
monitor_environments, serialize(monitor_environments, user)
218245
):
@@ -227,6 +254,7 @@ def get_attrs(self, item_list, user, **kwargs):
227254
"project": projects_data[item.project_id] if item.project_id else None,
228255
"environments": environment_data[item.id],
229256
"owner": actor_data.get(item.owner_actor),
257+
"is_muted": is_muted_data[item.id],
230258
}
231259
for item in item_list
232260
}
@@ -245,7 +273,7 @@ def serialize(self, obj, attrs, user, **kwargs) -> MonitorSerializerResponse:
245273
result: MonitorSerializerResponse = {
246274
"id": str(obj.guid),
247275
"status": obj.get_status_display(),
248-
"isMuted": obj.is_muted,
276+
"isMuted": attrs["is_muted"],
249277
"isUpserting": obj.is_upserting,
250278
"name": obj.name,
251279
"slug": obj.slug,

tests/sentry/monitors/endpoints/test_base_monitor_details.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
MonitorEnvironment,
1919
MonitorIncident,
2020
ScheduleType,
21+
is_monitor_muted,
2122
)
2223
from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR
2324
from sentry.monitors.utils import ensure_cron_detector, get_timeout_at
@@ -354,7 +355,7 @@ def test_can_mute(self) -> None:
354355
assert resp.data["slug"] == monitor.slug
355356

356357
monitor = Monitor.objects.get(id=monitor.id)
357-
assert monitor.is_muted
358+
assert is_monitor_muted(monitor)
358359

359360
def test_can_unmute(self) -> None:
360361
monitor = self._create_monitor()
@@ -367,7 +368,7 @@ def test_can_unmute(self) -> None:
367368
assert resp.data["slug"] == monitor.slug
368369

369370
monitor = Monitor.objects.get(id=monitor.id)
370-
assert not monitor.is_muted
371+
assert not is_monitor_muted(monitor)
371372

372373
def test_timezone(self) -> None:
373374
monitor = self._create_monitor()

tests/sentry/monitors/endpoints/test_base_monitor_environment_details.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
from sentry.constants import ObjectStatus
22
from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion
3-
from sentry.monitors.models import Monitor, MonitorEnvironment, MonitorStatus
3+
from sentry.monitors.models import Monitor, MonitorEnvironment, MonitorStatus, is_monitor_muted
44
from sentry.testutils.cases import MonitorTestCase
55
from sentry.testutils.helpers.datetime import freeze_time
66

@@ -72,7 +72,7 @@ def test_muting_all_environments_mutes_monitor(self) -> None:
7272

7373
# Initially monitor should be unmuted
7474
monitor.refresh_from_db()
75-
assert monitor.is_muted is False
75+
assert is_monitor_muted(monitor) is False
7676

7777
# Mute first environment
7878
self.get_success_response(
@@ -85,7 +85,7 @@ def test_muting_all_environments_mutes_monitor(self) -> None:
8585

8686
# Monitor should still be unmuted (one environment is unmuted)
8787
monitor.refresh_from_db()
88-
assert monitor.is_muted is False
88+
assert is_monitor_muted(monitor) is False
8989

9090
# Mute second environment
9191
self.get_success_response(
@@ -98,7 +98,7 @@ def test_muting_all_environments_mutes_monitor(self) -> None:
9898

9999
# Now monitor should be muted (all environments are muted)
100100
monitor.refresh_from_db()
101-
assert monitor.is_muted is True
101+
assert is_monitor_muted(monitor) is True
102102

103103
def test_unmuting_one_environment_unmutes_monitor(self) -> None:
104104
"""Test that unmuting one environment when all were muted also unmutes the monitor."""
@@ -111,7 +111,7 @@ def test_unmuting_one_environment_unmutes_monitor(self) -> None:
111111

112112
# Verify initial state
113113
monitor.refresh_from_db()
114-
assert monitor.is_muted is True
114+
assert is_monitor_muted(monitor) is True
115115

116116
# Unmute one environment via the endpoint
117117
self.get_success_response(
@@ -124,7 +124,7 @@ def test_unmuting_one_environment_unmutes_monitor(self) -> None:
124124

125125
# Monitor should now be unmuted
126126
monitor = Monitor.objects.get(id=monitor.id)
127-
assert monitor.is_muted is False
127+
assert is_monitor_muted(monitor) is False
128128
env1.refresh_from_db()
129129
assert env1.is_muted is False
130130
env2.refresh_from_db()

tests/sentry/monitors/endpoints/test_organization_detector_index.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from sentry.api.serializers import serialize
44
from sentry.constants import ObjectStatus
55
from sentry.monitors.grouptype import MonitorIncidentType
6-
from sentry.monitors.models import Monitor, ScheduleType
6+
from sentry.monitors.models import Monitor, ScheduleType, is_monitor_muted
77
from sentry.monitors.serializers import MonitorSerializer
88
from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR
99
from sentry.testutils.cases import APITestCase
@@ -205,7 +205,7 @@ def test_create_monitor_with_optional_fields(self):
205205
)
206206
assert monitor.name == "Full Config Monitor"
207207
assert monitor.status == ObjectStatus.DISABLED
208-
assert monitor.is_muted is False
208+
assert is_monitor_muted(monitor) is False
209209
assert monitor.config["schedule"] == "*/30 * * * *"
210210
assert monitor.config["checkin_margin"] == 15
211211
assert monitor.config["max_runtime"] == 120

tests/sentry/monitors/endpoints/test_organization_monitor_index.py

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
from sentry.analytics.events.cron_monitor_created import CronMonitorCreated, FirstCronMonitorCreated
1313
from sentry.constants import DataCategory, ObjectStatus
1414
from sentry.models.rule import Rule, RuleSource
15-
from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType
15+
from sentry.monitors.models import Monitor, MonitorStatus, ScheduleType, is_monitor_muted
1616
from sentry.monitors.utils import get_detector_for_monitor
1717
from sentry.quotas.base import SeatAssignmentResult
1818
from sentry.testutils.asserts import assert_org_audit_log_exists
@@ -150,7 +150,7 @@ def test_sort_muted(self) -> None:
150150
self._create_monitor(name="A monitor"),
151151
self._create_monitor(name="ZA monitor"),
152152
]
153-
monitors.sort(key=lambda m: (m.is_muted, m.name))
153+
monitors.sort(key=lambda m: (is_monitor_muted(m), m.name))
154154

155155
response = self.get_success_response(self.organization.slug, sort="muted")
156156
self.check_valid_response(response, monitors)
@@ -389,6 +389,45 @@ def test_ignore_pending_deletion_environments(self) -> None:
389389
assert len(response.data[0]["environments"]) == 1
390390
assert response.data[0]["environments"][0]["status"] == "ok"
391391

392+
def test_is_muted_calculated_from_all_envs_not_filtered(self) -> None:
393+
"""Test that isMuted field is calculated from ALL environments, not just filtered ones"""
394+
# Monitor with prod unmuted, dev muted
395+
monitor = self._create_monitor(name="Test Monitor")
396+
self._create_monitor_environment(monitor, name="prod", is_muted=False)
397+
self._create_monitor_environment(monitor, name="dev", is_muted=True)
398+
399+
# When viewing all environments, monitor should NOT be muted
400+
# (because not ALL environments are muted)
401+
response = self.get_success_response(self.organization.slug)
402+
assert response.data[0]["isMuted"] is False
403+
404+
# When filtering to only "prod" environment, monitor should STILL not be muted
405+
# even though we're only displaying prod in the environments array
406+
response = self.get_success_response(self.organization.slug, environment=["prod"])
407+
assert response.data[0]["isMuted"] is False
408+
assert len(response.data[0]["environments"]) == 1
409+
assert response.data[0]["environments"][0]["name"] == "prod"
410+
411+
# When filtering to only "dev" environment (which is muted),
412+
# monitor should STILL not be muted because prod is unmuted
413+
response = self.get_success_response(self.organization.slug, environment=["dev"])
414+
assert response.data[0]["isMuted"] is False
415+
assert len(response.data[0]["environments"]) == 1
416+
assert response.data[0]["environments"][0]["name"] == "dev"
417+
418+
# Now mute ALL environments
419+
monitor.monitorenvironment_set.update(is_muted=True)
420+
421+
# Monitor should now be muted regardless of environment filter
422+
response = self.get_success_response(self.organization.slug)
423+
assert response.data[0]["isMuted"] is True
424+
425+
response = self.get_success_response(self.organization.slug, environment=["prod"])
426+
assert response.data[0]["isMuted"] is True
427+
428+
response = self.get_success_response(self.organization.slug, environment=["dev"])
429+
assert response.data[0]["isMuted"] is True
430+
392431

393432
class CreateOrganizationMonitorTest(MonitorTestCase):
394433
endpoint = "sentry-api-0-organization-monitor-index"
@@ -655,8 +694,8 @@ def test_bulk_mute_unmute(self) -> None:
655694
monitor_two.refresh_from_db()
656695
env_one.refresh_from_db()
657696
env_two.refresh_from_db()
658-
assert monitor_one.is_muted
659-
assert monitor_two.is_muted
697+
assert is_monitor_muted(monitor_one)
698+
assert is_monitor_muted(monitor_two)
660699
assert env_one.is_muted
661700
assert env_two.is_muted
662701
assert_org_audit_log_exists(
@@ -681,8 +720,8 @@ def test_bulk_mute_unmute(self) -> None:
681720
monitor_two.refresh_from_db()
682721
env_one.refresh_from_db()
683722
env_two.refresh_from_db()
684-
assert not monitor_one.is_muted
685-
assert not monitor_two.is_muted
723+
assert not is_monitor_muted(monitor_one)
724+
assert not is_monitor_muted(monitor_two)
686725
assert not env_one.is_muted
687726
assert not env_two.is_muted
688727

@@ -763,5 +802,5 @@ def test_disallow_when_no_open_membership(self) -> None:
763802

764803
monitor.refresh_from_db()
765804
env.refresh_from_db()
766-
assert not monitor.is_muted
805+
assert not is_monitor_muted(monitor)
767806
assert not env.is_muted

tests/sentry/monitors/logic/test_mark_failed.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
MonitorIncident,
1717
MonitorStatus,
1818
ScheduleType,
19+
is_monitor_muted,
1920
)
2021
from sentry.testutils.cases import TestCase
2122

@@ -100,7 +101,7 @@ def test_mark_failed_muted(self, mock_dispatch_incident_occurrence: mock.MagicMo
100101

101102
monitor.refresh_from_db()
102103
monitor_environment.refresh_from_db()
103-
assert monitor.is_muted
104+
assert is_monitor_muted(monitor)
104105
assert monitor_environment.status == MonitorStatus.ERROR
105106

106107
assert mock_dispatch_incident_occurrence.call_count == 0
@@ -342,7 +343,7 @@ def test_mark_failed_issue_threshold_disabled(
342343

343344
monitor.refresh_from_db()
344345
monitor_environment.refresh_from_db()
345-
assert monitor.is_muted
346+
assert is_monitor_muted(monitor)
346347
assert monitor_environment.status == MonitorStatus.ERROR
347348

348349
assert mock_dispatch_incident_occurrence.call_count == 0

tests/sentry/monitors/test_models.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
MonitorEnvironmentLimitsExceeded,
1414
MonitorLimitsExceeded,
1515
ScheduleType,
16+
is_monitor_muted,
1617
)
1718
from sentry.monitors.types import DATA_SOURCE_CRON_MONITOR
1819
from sentry.monitors.validators import ConfigValidator
@@ -454,7 +455,7 @@ def test_is_muted_all_environments_muted(self):
454455
)
455456

456457
# Verify monitor.is_muted is True
457-
assert monitor.is_muted is True
458+
assert is_monitor_muted(monitor) is True
458459

459460
def test_is_muted_some_environments_unmuted(self):
460461
"""Test that monitor.is_muted returns False when any environment is unmuted."""
@@ -475,7 +476,7 @@ def test_is_muted_some_environments_unmuted(self):
475476
)
476477

477478
# Verify monitor.is_muted is False
478-
assert monitor.is_muted is False
479+
assert is_monitor_muted(monitor) is False
479480

480481
def test_is_muted_all_environments_unmuted(self):
481482
"""Test that monitor.is_muted returns False when all environments are unmuted."""
@@ -496,12 +497,12 @@ def test_is_muted_all_environments_unmuted(self):
496497
)
497498

498499
# Verify monitor.is_muted is False
499-
assert monitor.is_muted is False
500+
assert is_monitor_muted(monitor) is False
500501

501502
def test_is_muted_no_environments(self):
502503
"""Test that monitor.is_muted returns False when there are no environments."""
503504
monitor = self.create_monitor()
504-
assert monitor.is_muted is False
505+
assert is_monitor_muted(monitor) is False
505506

506507
def test_is_muted_single_environment(self):
507508
"""Test is_muted works correctly with a single environment."""
@@ -514,7 +515,7 @@ def test_is_muted_single_environment(self):
514515
is_muted=True,
515516
)
516517

517-
assert monitor.is_muted is True
518+
assert is_monitor_muted(monitor) is True
518519

519520
# Test with unmuted environment
520521
monitor2 = self.create_monitor()
@@ -525,4 +526,4 @@ def test_is_muted_single_environment(self):
525526
is_muted=False,
526527
)
527528

528-
assert monitor2.is_muted is False
529+
assert is_monitor_muted(monitor2) is False

0 commit comments

Comments
 (0)