Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup(group): Remove post-save option #79381

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
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
10 changes: 1 addition & 9 deletions src/sentry/api/helpers/group_index/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@
import rest_framework
from django.db import IntegrityError, router, transaction
from django.db.models import Q
from django.db.models.signals import post_save
from django.utils import timezone as django_timezone
from rest_framework import serializers
from rest_framework.request import Request
from rest_framework.response import Response

from sentry import analytics, features, options
from sentry import analytics, features
from sentry.api.serializers import serialize
from sentry.api.serializers.models.actor import ActorSerializer, ActorSerializerResponse
from sentry.db.models.query import create_or_update
Expand Down Expand Up @@ -531,13 +530,6 @@ def update_groups(
group.status = GroupStatus.RESOLVED
group.substatus = None
group.resolved_at = now
if affected and not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["resolved_at", "status", "substatus"],
)
remove_group_from_inbox(
group, action=GroupInboxRemoveAction.RESOLVED, user=acting_user
)
Expand Down
8 changes: 0 additions & 8 deletions src/sentry/event_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
from django.core.exceptions import ValidationError
from django.db import IntegrityError, OperationalError, connection, router, transaction
from django.db.models import Func, Max
from django.db.models.signals import post_save
from django.utils.encoding import force_str
from urllib3.exceptions import MaxRetryError, TimeoutError
from usageaccountant import UsageUnit
Expand Down Expand Up @@ -1717,13 +1716,6 @@ def _handle_regression(group: Group, event: BaseEvent, release: Release | None)
transition_type="automatic",
sender="handle_regression",
)
if not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["last_seen", "active_at", "status", "substatus"],
)

follows_semver = False
resolved_in_activity = None
Expand Down
24 changes: 1 addition & 23 deletions src/sentry/issues/escalating.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
from typing import Any, TypedDict

import jsonschema
from django.db.models.signals import post_save
from snuba_sdk import (
Column,
Condition,
Expand All @@ -28,7 +27,7 @@
)
from snuba_sdk.expressions import Granularity

from sentry import features, options
from sentry import features
from sentry.eventstore.models import GroupEvent
from sentry.issues.escalating_group_forecast import EscalatingGroupForecast
from sentry.issues.escalating_issues_alg import GroupCount
Expand Down Expand Up @@ -507,13 +506,6 @@ def manage_issue_states(
if updated:
group.status = GroupStatus.UNRESOLVED
group.substatus = GroupSubStatus.ESCALATING
if not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)
add_group_to_inbox(group, GroupInboxReason.ESCALATING, snooze_details)
record_group_history(group, GroupHistoryStatus.ESCALATING)

Expand Down Expand Up @@ -553,13 +545,6 @@ def manage_issue_states(
if updated:
group.status = GroupStatus.UNRESOLVED
group.substatus = GroupSubStatus.ONGOING
if not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)
add_group_to_inbox(group, GroupInboxReason.ONGOING, snooze_details)
record_group_history(group, GroupHistoryStatus.ONGOING)

Expand All @@ -574,13 +559,6 @@ def manage_issue_states(
if updated:
group.status = GroupStatus.UNRESOLVED
group.substatus = GroupSubStatus.ONGOING
if not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)
add_group_to_inbox(group, GroupInboxReason.UNIGNORED, snooze_details)
record_group_history(group, GroupHistoryStatus.UNIGNORED)
Activity.objects.create_group_activity(
Expand Down
12 changes: 0 additions & 12 deletions src/sentry/issues/ongoing.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
from typing import Any

import sentry_sdk
from django.db.models.signals import post_save

from sentry import options
from sentry.models.group import Group, GroupStatus
from sentry.models.groupinbox import bulk_remove_groups_from_inbox
from sentry.signals import issue_unresolved
Expand Down Expand Up @@ -53,13 +51,3 @@ def bulk_transition_group_to_ongoing(

with sentry_sdk.start_span(name="bulk_remove_groups_from_inbox"):
bulk_remove_groups_from_inbox(groups_to_transistion)

with sentry_sdk.start_span(name="post_save_send_robust"):
if not options.get("groups.enable-post-update-signal"):
for group in groups_to_transistion:
post_save.send_robust(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)
12 changes: 0 additions & 12 deletions src/sentry/issues/status_change.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
from datetime import datetime, timedelta, timezone
from typing import Any

from django.db.models.signals import post_save

from sentry import options
from sentry.integrations.tasks.kick_off_status_syncs import kick_off_status_syncs
from sentry.issues.ignored import IGNORED_CONDITION_FIELDS
from sentry.issues.ongoing import TRANSITION_AFTER_DAYS
Expand Down Expand Up @@ -165,13 +162,4 @@ def handle_status_update(
kick_off_status_syncs.apply_async(
kwargs={"project_id": group.project_id, "group_id": group.id}
)

if not options.get("groups.enable-post-update-signal"):
post_save.send(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)

return ActivityInfo(activity_type, activity_data)
14 changes: 0 additions & 14 deletions src/sentry/models/activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@
from django.conf import settings
from django.db import models
from django.db.models import F
from django.db.models.signals import post_save
from django.utils import timezone

from sentry import options
from sentry.backup.scopes import RelocationScope
from sentry.db.models import (
BoundedPositiveIntegerField,
Expand Down Expand Up @@ -169,26 +167,14 @@ def save(self, *args, **kwargs):

# HACK: support Group.num_comments
if self.type == ActivityType.NOTE.value and self.group is not None:
from sentry.models.group import Group

self.group.update(num_comments=F("num_comments") + 1)
if not options.get("groups.enable-post-update-signal"):
post_save.send_robust(
sender=Group, instance=self.group, created=True, update_fields=["num_comments"]
)

def delete(self, *args, **kwargs):
super().delete(*args, **kwargs)

# HACK: support Group.num_comments
if self.type == ActivityType.NOTE.value and self.group is not None:
from sentry.models.group import Group

self.group.update(num_comments=F("num_comments") - 1)
if not options.get("groups.enable-post-update-signal"):
post_save.send_robust(
sender=Group, instance=self.group, created=True, update_fields=["num_comments"]
)

def send_notification(self):
activity.send_activity_notifications.delay(self.id)
Expand Down
8 changes: 2 additions & 6 deletions src/sentry/models/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from django.utils.translation import gettext_lazy as _
from snuba_sdk import Column, Condition, Op

from sentry import eventstore, eventtypes, options, tagstore
from sentry import eventstore, eventtypes, tagstore
from sentry.backup.scopes import RelocationScope
from sentry.constants import DEFAULT_LOGGER_NAME, LOG_LEVELS, MAX_CULPRIT_LENGTH
from sentry.db.models import (
Expand Down Expand Up @@ -314,11 +314,7 @@ class GroupManager(BaseManager["Group"]):
use_for_related_fields = True

def get_queryset(self):
return (
super()
.get_queryset()
.with_post_update_signal(options.get("groups.enable-post-update-signal"))
)
return super().get_queryset().with_post_update_signal(True)

def by_qualified_short_id(self, organization_id: int, short_id: str):
return self.by_qualified_short_id_bulk(organization_id, [short_id])[0]
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/options/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -2370,7 +2370,7 @@
# Enable sending a post update signal after we update groups using a queryset update
register(
"groups.enable-post-update-signal",
default=False,
default=True,
flags=FLAG_BOOL | FLAG_AUTOMATOR_MODIFIABLE,
)

Expand Down
11 changes: 2 additions & 9 deletions tests/sentry/api/helpers/test_group_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from sentry.api.helpers.group_index.validators import ValidationError
from sentry.api.issue_search import parse_search_query
from sentry.models.activity import Activity
from sentry.models.group import Group, GroupStatus
from sentry.models.group import GroupStatus
from sentry.models.groupassignee import GroupAssignee
from sentry.models.groupbookmark import GroupBookmark
from sentry.models.grouphash import GroupHash
Expand Down Expand Up @@ -137,8 +137,7 @@ def test_resolving_unresolved_group(self, send_robust: Mock) -> None:
assert send_robust.called

@patch("sentry.signals.issue_ignored.send_robust")
@patch("sentry.issues.status_change.post_save")
def test_ignoring_group_archived_forever(self, post_save: Mock, send_robust: Mock) -> None:
def test_ignoring_group_archived_forever(self, send_robust: Mock) -> None:
group = self.create_group()
add_group_to_inbox(group, GroupInboxReason.NEW)

Expand All @@ -157,12 +156,6 @@ def test_ignoring_group_archived_forever(self, post_save: Mock, send_robust: Moc
assert group.status == GroupStatus.IGNORED
assert group.substatus == GroupSubStatus.FOREVER
assert send_robust.called
post_save.send.assert_called_with(
sender=Group,
instance=group,
created=False,
update_fields=["status", "substatus"],
)
assert not GroupInbox.objects.filter(group=group).exists()

@patch("sentry.signals.issue_ignored.send_robust")
Expand Down
30 changes: 6 additions & 24 deletions tests/sentry/db/models/manager/test_base_query_set.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from sentry.models.group import Group
from sentry.signals import post_update
from sentry.testutils.cases import TestCase
from sentry.testutils.helpers import override_options


@contextmanager
Expand Down Expand Up @@ -40,31 +39,18 @@ def test_empty_query(self):

class TestSendPostUpdateSignal(TestCase):
def test_not_triggered(self):
with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": True}
):
with catch_signal(post_update) as handler:
self.group.message = "hi"
self.group.save()

assert not handler.called

with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": True}
):
with catch_signal(post_update) as handler:
self.group.update(message="hi")

assert not handler.called

with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": False}
):
assert Group.objects.filter(id=self.group.id).update(message="hi") == 1

assert not handler.called

with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": True}
):
with catch_signal(post_update) as handler:
assert (
Group.objects.filter(id=self.group.id)
.with_post_update_signal(False)
Expand All @@ -75,9 +61,7 @@ def test_not_triggered(self):
assert not handler.called

# Test signal not fired when Django detects the query will return no results
with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": True}
):
with catch_signal(post_update) as handler:
assert (
Group.objects.filter(id__in=[]).with_post_update_signal(True).update(message="hi")
== 0
Expand All @@ -86,7 +70,7 @@ def test_not_triggered(self):
assert not handler.called

def test_enable(self):
qs = Group.objects.all()
qs = Group.objects.with_post_update_signal(False)
assert not qs._with_post_update_signal
new_qs = qs.with_post_update_signal(True)
# Make sure we don't modify the previous queryset
Expand All @@ -95,9 +79,7 @@ def test_enable(self):

def test_triggered(self):
message = "hi"
with catch_signal(post_update) as handler, override_options(
{"groups.enable-post-update-signal": True}
):
with catch_signal(post_update) as handler:
assert Group.objects.filter(id=self.group.id).update(message=message) == 1

self.group.refresh_from_db()
Expand Down
7 changes: 1 addition & 6 deletions tests/sentry/issues/test_attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,7 @@ def run_attr_test(
with patch(
"sentry.issues.attributes._log_group_attributes_changed"
) as _log_group_attributes_changed:
with override_options(
{
"groups.enable-post-update-signal": True,
"issues.group_attributes.send_kafka": True,
}
):
with override_options({"issues.group_attributes.send_kafka": True}):
Group.objects.filter(id__in=[g.id for g in groups]).update(**update_fields)
_log_group_attributes_changed.assert_called_with(
Operation.UPDATED, "group", expected_str
Expand Down
Loading