Skip to content

Commit 980ead9

Browse files
authored
fix(issues): Start writing new_group_first_seen in merge case (#97682)
This is part of an effort to fix the issue-search's sort-by-age feature. As part of that feature, we are adding the "first_seen" time of a group to each error. When we merge groups, we want to update the errors to have the new group's "group_first_seen". We've decided to do that by explicitly passing the primary group's "first_seen" time along with the Kafka message. This PR starts writing that field properly; it must wait until [snuba#7351](getsentry/snuba#7351) has fully deployed.
1 parent d45ce1a commit 980ead9

File tree

6 files changed

+31
-9
lines changed

6 files changed

+31
-9
lines changed

src/sentry/eventstream/base.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,11 @@ def end_delete_groups(self, state: Mapping[str, Any]) -> None:
147147
pass
148148

149149
def start_merge(
150-
self, project_id: int, previous_group_ids: Sequence[int], new_group_id: int
150+
self,
151+
project_id: int,
152+
previous_group_ids: Sequence[int],
153+
new_group_id: int,
154+
new_group_first_seen: datetime | None = None,
151155
) -> dict[str, Any]:
152156
raise NotImplementedError
153157

src/sentry/eventstream/snuba.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
# 'project_id': id,
5555
# 'previous_group_ids': [id2, id2],
5656
# 'new_group_id': id,
57+
# 'group_first_seen': timestamp,
5758
# 'datetime': timestamp,
5859
# })
5960
# Unmerge: (2, '(start_unmerge|end_unmerge)', {
@@ -229,7 +230,11 @@ def end_delete_groups(self, state: Mapping[str, Any]) -> None:
229230
)
230231

231232
def start_merge(
232-
self, project_id: int, previous_group_ids: Sequence[int], new_group_id: int
233+
self,
234+
project_id: int,
235+
previous_group_ids: Sequence[int],
236+
new_group_id: int,
237+
new_group_first_seen: datetime | None = None,
233238
) -> dict[str, Any]:
234239
if not previous_group_ids:
235240
raise ValueError("expected groups to merge!")
@@ -242,13 +247,16 @@ def start_merge(
242247
"datetime": json.datetime_to_str(datetime.now(tz=timezone.utc)),
243248
}
244249

250+
if new_group_first_seen is not None:
251+
state["new_group_first_seen"] = json.datetime_to_str(new_group_first_seen)
252+
245253
self._send(project_id, "start_merge", extra_data=(state,), asynchronous=False)
246254

247255
return state
248256

249257
def end_merge(self, state: Mapping[str, Any]) -> None:
250258
state_copy: MutableMapping[str, Any] = {**state}
251-
state_copy["datetime"] = datetime.now(tz=timezone.utc)
259+
state_copy["datetime"] = json.datetime_to_str(datetime.now(tz=timezone.utc))
252260
self._send(
253261
state_copy["project_id"], "end_merge", extra_data=(state_copy,), asynchronous=False
254262
)

src/sentry/issues/merge.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ def handle_merge(
5656

5757
group_ids_to_merge = [g.id for g in groups_to_merge]
5858
eventstream_state = eventstream.backend.start_merge(
59-
primary_group.project_id, group_ids_to_merge, primary_group.id
59+
primary_group.project_id, group_ids_to_merge, primary_group.id, primary_group.first_seen
6060
)
6161

6262
Group.objects.filter(id__in=group_ids_to_merge).update(

tests/sentry/issues/endpoints/test_organization_group_index.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4034,7 +4034,10 @@ def test_merge(
40344034
)
40354035

40364036
mock_eventstream.start_merge.assert_called_once_with(
4037-
group1.project_id, [group3.id, group1.id], group2.id
4037+
group1.project_id,
4038+
[group3.id, group1.id],
4039+
group2.id,
4040+
group2.first_seen,
40384041
)
40394042

40404043
assert len(merge_groups.mock_calls) == 1

tests/sentry/issues/test_merge.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
from typing import Any
2-
from unittest.mock import patch
1+
from unittest.mock import MagicMock, patch
32

43
import pytest
54
import rest_framework
@@ -13,6 +12,7 @@
1312
from sentry.testutils.skips import requires_snuba
1413
from sentry.types.activity import ActivityType
1514
from sentry.types.group import GroupSubStatus
15+
from sentry.utils import json
1616

1717
pytestmark = [requires_snuba]
1818

@@ -29,7 +29,7 @@ def setUp(self) -> None:
2929
self.groups.append(group)
3030

3131
@patch("sentry.tasks.merge.merge_groups.delay")
32-
def test_handle_merge(self, merge_groups: Any) -> None:
32+
def test_handle_merge(self, merge_groups: MagicMock) -> None:
3333
Activity.objects.all().delete()
3434
merge = handle_merge(self.groups, self.project_lookup, self.user)
3535

@@ -39,6 +39,10 @@ def test_handle_merge(self, merge_groups: Any) -> None:
3939
assert len(groups.filter(substatus__isnull=True)) == 4
4040
assert merge_groups.called
4141

42+
assert merge_groups.call_args[1]["eventstream_state"][
43+
"new_group_first_seen"
44+
] == json.datetime_to_str(min([g.first_seen for g in groups]))
45+
4246
primary_group = self.groups[0]
4347
assert Activity.objects.filter(type=ActivityType.MERGE.value, group=primary_group)
4448
assert merge["parent"] == str(primary_group.id)

tests/snuba/api/endpoints/test_project_group_index.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1342,7 +1342,10 @@ def test_merge(
13421342
)
13431343

13441344
mock_eventstream.start_merge.assert_called_once_with(
1345-
group1.project_id, [group3.id, group1.id], group2.id
1345+
group1.project_id,
1346+
[group3.id, group1.id],
1347+
group2.id,
1348+
group2.first_seen,
13461349
)
13471350

13481351
assert len(merge_groups.mock_calls) == 1

0 commit comments

Comments
 (0)