Skip to content

Commit 09abcc3

Browse files
lobsterkatiegeorge-sentry
authored andcommitted
ref(grouping): Increase config transition and move config tests (#112508)
When a project's grouping config is updated, we put the project in a transition period, during which we calculate the old hash if we don't find the new hash, so we can attach the new hash to the old hash's group (assuming the old hash exists). This prevents grouping configs from creating new groups even though event data hasn't changed. It used to be that during the transition period, we _always_ calculated the old hash in addition to the new hash, even if we'd already linked the new hash to an existing group (if any). Since the vast majority of events come in to existing groups, this meant a ton of wasted calculation, and an unacceptable slow-down during ingest if too many projects were in transition at once. To limit the number of projects transitioning at any one time, we set the transition period to only last a week. Now, however, we limit the double calculation to the first time we see an event from a given group, rather than every time we see an event from that group. Because this theoretically reduced (by multiple orders of magnitude) the extra work a transition would entail, once we made that change, we increased the transition period to a month. We still hadn't seen it in action, though. That changed a few months ago, when we introduced a new grouping config. Sure enough, though there was an initial spike of double calculations (at about 7% of events), it dropped extremely quickly, and within a few hours it was down under 1% of events (and has only continued to fall since). Even more encouragingly, there was no noticeable change to average ingest time. Now that we have that assurance, we can increase the transition period to the full 90 days it takes for groups to age out, thus preventing even the few groups which go more than a month between events from being recreated. This PR does just that, by changing the `SENTRY_GROUPING_CONFIG_TRANSITION_DURATION` constant. While we're dealing with grouping config stuff, it also moves the relevant tests to their own file, continuing the effort to have `test_event_manager_grouping.py` not be literally a thousand lines long.
1 parent 0851330 commit 09abcc3

File tree

3 files changed

+199
-189
lines changed

3 files changed

+199
-189
lines changed

src/sentry/conf/server.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2817,7 +2817,7 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]:
28172817
BETA_GROUPING_CONFIG = ""
28182818

28192819
# How long the migration phase for grouping lasts
2820-
SENTRY_GROUPING_CONFIG_TRANSITION_DURATION = 30 * 24 * 3600 # 30 days
2820+
SENTRY_GROUPING_CONFIG_TRANSITION_DURATION = 90 * 24 * 3600 # 90 days, until groups age out
28212821

28222822
SENTRY_USE_GRANIAN = True
28232823

tests/sentry/event_manager/test_event_manager_grouping.py

Lines changed: 2 additions & 188 deletions
Original file line numberDiff line numberDiff line change
@@ -6,40 +6,34 @@
66
from time import time
77
from typing import Any
88
from unittest import mock
9-
from unittest.mock import ANY, MagicMock, patch
9+
from unittest.mock import MagicMock, patch
1010

1111
import pytest
1212
from django.core.cache import cache
1313

14-
from sentry import audit_log
15-
from sentry.conf.server import DEFAULT_GROUPING_CONFIG, SENTRY_GROUPING_CONFIG_TRANSITION_DURATION
14+
from sentry.conf.server import DEFAULT_GROUPING_CONFIG
1615
from sentry.event_manager import _get_updated_group_title
1716
from sentry.eventtypes.base import DefaultEvent
1817
from sentry.exceptions import HashDiscarded
19-
from sentry.grouping.api import get_grouping_config_dict_for_project
2018
from sentry.grouping.ingest.caching import (
2119
get_grouphash_existence_cache_key,
2220
get_grouphash_object_cache_key,
2321
)
24-
from sentry.grouping.ingest.config import update_or_set_grouping_config_if_needed
2522
from sentry.grouping.ingest.hashing import (
2623
GROUPHASH_CACHE_EXPIRY_SECONDS,
2724
find_grouphash_with_group,
2825
get_or_create_grouphashes,
2926
)
30-
from sentry.models.auditlogentry import AuditLogEntry
3127
from sentry.models.group import Group
3228
from sentry.models.grouphash import GroupHash
3329
from sentry.models.grouptombstone import GroupTombstone
34-
from sentry.models.options.project_option import ProjectOption
3530
from sentry.models.project import Project
3631
from sentry.services.eventstore.models import Event
3732
from sentry.testutils.cases import TestCase
3833
from sentry.testutils.helpers.eventprocessing import save_new_event
3934
from sentry.testutils.helpers.options import override_options
4035
from sentry.testutils.pytest.fixtures import django_db_all
4136
from sentry.testutils.pytest.mocking import count_matching_calls
42-
from sentry.testutils.silo import assume_test_silo_mode_of
4337
from sentry.testutils.skips import requires_snuba
4438
from tests.sentry.grouping import NO_MSG_PARAM_CONFIG
4539

@@ -186,186 +180,6 @@ def test_updates_group_metadata(self) -> None:
186180
assert group.message == event2.message
187181
assert group.data["metadata"]["title"] == event2.title
188182

189-
def test_loads_default_config_if_stored_config_option_is_invalid(self) -> None:
190-
self.project.update_option("sentry:grouping_config", "dogs.are.great")
191-
config_dict = get_grouping_config_dict_for_project(self.project)
192-
assert config_dict["id"] == DEFAULT_GROUPING_CONFIG
193-
194-
self.project.update_option("sentry:grouping_config", {"not": "a string"})
195-
config_dict = get_grouping_config_dict_for_project(self.project)
196-
assert config_dict["id"] == DEFAULT_GROUPING_CONFIG
197-
198-
def test_auto_updates_grouping_config_even_if_config_is_gone(self) -> None:
199-
"""This tests that setups with deprecated configs will auto-upgrade."""
200-
self.project.update_option("sentry:grouping_config", "non_existing_config")
201-
save_new_event({"message": "foo"}, self.project)
202-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
203-
assert self.project.get_option("sentry:secondary_grouping_config") is None
204-
205-
def test_auto_updates_grouping_config(self) -> None:
206-
self.project.update_option("sentry:grouping_config", NO_MSG_PARAM_CONFIG)
207-
# Set platform to prevent additional audit log entry from platform inference
208-
self.project.platform = "python"
209-
self.project.save()
210-
211-
save_new_event({"message": "Adopt don't shop"}, self.project)
212-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
213-
214-
with assume_test_silo_mode_of(AuditLogEntry):
215-
audit_log_entry = AuditLogEntry.objects.get()
216-
217-
assert audit_log_entry.event == audit_log.get_event_id("PROJECT_EDIT")
218-
assert audit_log_entry.actor_label == "Sentry"
219-
220-
assert audit_log_entry.data == {
221-
"sentry:grouping_config": DEFAULT_GROUPING_CONFIG,
222-
"sentry:secondary_grouping_config": NO_MSG_PARAM_CONFIG,
223-
"sentry:secondary_grouping_expiry": ANY, # tested separately below
224-
"id": self.project.id,
225-
"slug": self.project.slug,
226-
"name": self.project.name,
227-
"status": 0,
228-
"public": False,
229-
}
230-
231-
# When the config upgrade is actually happening, the expiry value is set before the
232-
# audit log entry is created, which means the expiry is based on a timestamp
233-
# ever-so-slightly before the audit log entry's timestamp, making a one-second tolerance
234-
# necessary.
235-
actual_expiry = audit_log_entry.data["sentry:secondary_grouping_expiry"]
236-
expected_expiry = (
237-
int(audit_log_entry.datetime.timestamp()) + SENTRY_GROUPING_CONFIG_TRANSITION_DURATION
238-
)
239-
assert actual_expiry == expected_expiry or actual_expiry == expected_expiry - 1
240-
241-
@patch(
242-
"sentry.event_manager.update_or_set_grouping_config_if_needed",
243-
wraps=update_or_set_grouping_config_if_needed,
244-
)
245-
def test_sets_default_grouping_config_project_option_if_missing(
246-
self, update_config_spy: MagicMock
247-
):
248-
# To start, the project defaults to the current config but doesn't have its own config
249-
# option set in the DB
250-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
251-
assert (
252-
ProjectOption.objects.filter(
253-
project_id=self.project.id, key="sentry:grouping_config"
254-
).first()
255-
is None
256-
)
257-
258-
save_new_event({"message": "Dogs are great!"}, self.project)
259-
260-
update_config_spy.assert_called_with(self.project, "ingest")
261-
262-
# After the function has been called, the config still defaults to the current one (and no
263-
# transition has started), but the project now has its own config record in the DB
264-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
265-
assert self.project.get_option("sentry:secondary_grouping_config") is None
266-
assert self.project.get_option("sentry:secondary_grouping_expiry") == 0
267-
assert ProjectOption.objects.filter(
268-
project_id=self.project.id, key="sentry:grouping_config"
269-
).exists()
270-
271-
@patch(
272-
"sentry.event_manager.update_or_set_grouping_config_if_needed",
273-
wraps=update_or_set_grouping_config_if_needed,
274-
)
275-
def test_no_ops_if_grouping_config_project_option_exists_and_is_current(
276-
self, update_config_spy: MagicMock
277-
):
278-
self.project.update_option("sentry:grouping_config", DEFAULT_GROUPING_CONFIG)
279-
280-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
281-
assert ProjectOption.objects.filter(
282-
project_id=self.project.id, key="sentry:grouping_config"
283-
).exists()
284-
285-
save_new_event({"message": "Dogs are great!"}, self.project)
286-
287-
update_config_spy.assert_called_with(self.project, "ingest")
288-
289-
# After the function has been called, the config still defaults to the current one and no
290-
# transition has started
291-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
292-
assert self.project.get_option("sentry:secondary_grouping_config") is None
293-
assert self.project.get_option("sentry:secondary_grouping_expiry") == 0
294-
295-
@patch(
296-
"sentry.event_manager.update_or_set_grouping_config_if_needed",
297-
wraps=update_or_set_grouping_config_if_needed,
298-
)
299-
def test_no_ops_if_sample_rate_test_fails(self, update_config_spy: MagicMock):
300-
with (
301-
# Ensure our die roll will fall outside the sample rate
302-
patch("sentry.grouping.ingest.config.random", return_value=0.1121),
303-
override_options({"grouping.config_transition.config_upgrade_sample_rate": 0.0908}),
304-
):
305-
self.project.update_option("sentry:grouping_config", NO_MSG_PARAM_CONFIG)
306-
assert self.project.get_option("sentry:grouping_config") == NO_MSG_PARAM_CONFIG
307-
308-
save_new_event({"message": "Dogs are great!"}, self.project)
309-
310-
update_config_spy.assert_called_with(self.project, "ingest")
311-
312-
# After the function has been called, the config hasn't changed and no transition has
313-
# started
314-
assert self.project.get_option("sentry:grouping_config") == NO_MSG_PARAM_CONFIG
315-
assert self.project.get_option("sentry:secondary_grouping_config") is None
316-
assert self.project.get_option("sentry:secondary_grouping_expiry") == 0
317-
318-
@patch(
319-
"sentry.event_manager.update_or_set_grouping_config_if_needed",
320-
wraps=update_or_set_grouping_config_if_needed,
321-
)
322-
def test_ignores_sample_rate_if_current_config_is_invalid(self, update_config_spy: MagicMock):
323-
with (
324-
# Ensure our die roll will fall outside the sample rate
325-
patch("sentry.grouping.ingest.config.random", return_value=0.1121),
326-
override_options({"grouping.config_transition.config_upgrade_sample_rate": 0.0908}),
327-
):
328-
self.project.update_option("sentry:grouping_config", "not_a_real_config")
329-
assert self.project.get_option("sentry:grouping_config") == "not_a_real_config"
330-
331-
save_new_event({"message": "Dogs are great!"}, self.project)
332-
333-
update_config_spy.assert_called_with(self.project, "ingest")
334-
335-
# The config has been updated, but no transition has started because we can't calculate
336-
# a secondary hash using a config that doesn't exist
337-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
338-
assert self.project.get_option("sentry:secondary_grouping_config") is None
339-
assert self.project.get_option("sentry:secondary_grouping_expiry") == 0
340-
341-
@patch(
342-
"sentry.event_manager.update_or_set_grouping_config_if_needed",
343-
wraps=update_or_set_grouping_config_if_needed,
344-
)
345-
def test_ignores_sample_rate_if_no_record_exists(self, update_config_spy: MagicMock):
346-
with (
347-
# Ensure our die roll will fall outside the sample rate
348-
patch("sentry.grouping.ingest.config.random", return_value=0.1121),
349-
override_options({"grouping.config_transition.config_upgrade_sample_rate": 0.0908}),
350-
):
351-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
352-
assert not ProjectOption.objects.filter(
353-
project_id=self.project.id, key="sentry:grouping_config"
354-
).exists()
355-
356-
save_new_event({"message": "Dogs are great!"}, self.project)
357-
358-
update_config_spy.assert_called_with(self.project, "ingest")
359-
360-
# The config hasn't been updated, but now the project has its own record. No transition
361-
# has started because the config was already up to date.
362-
assert self.project.get_option("sentry:grouping_config") == DEFAULT_GROUPING_CONFIG
363-
assert ProjectOption.objects.filter(
364-
project_id=self.project.id, key="sentry:grouping_config"
365-
).exists()
366-
assert self.project.get_option("sentry:secondary_grouping_config") is None
367-
assert self.project.get_option("sentry:secondary_grouping_expiry") == 0
368-
369183

370184
class GroupHashCachingTest(TestCase):
371185
@contextmanager

0 commit comments

Comments
 (0)