Skip to content

Commit 7be1981

Browse files
nora-shapandrewshie-sentry
authored andcommitted
ref(suspect commits): Simplify event file committers to GroupOwner-only behavior (#98504)
## Summary Removes the legacy release-based fallback logic from the event file committers endpoint, simplifying it to use only GroupOwner-based suspect commit detection. Current behavior: If there are no suspect commits for the event, do a live analysis using the legacy strategy, do not save any of the commits it finds as suspect, return as many as it finds. 💡 : it was doing this because it was the only way to show a suspect commit if the Author was NOT a User of their org. The legacy strategy does not allow GroupOwner creation if there is not an associated User, it ignores those. This does not match the behavior of the non-legacy strategy, which allows GroupOwners with `user=None` and all serializers have fallback logic to display details from the `CommitAuthor` object if there is no `User`. The legacy method needs to be updated to allow GroupOwners with `user=None`. Proposed changes: no live analysis - only GroupOwners created during original analysis will be retuned. While my intention has been to create _fewer_ but _better_ suspect commits, with the logic in this flow meant I was actually displaying a lot more legacy strategy suspect commits, which is not what I want. ## Changes Made ### Core Logic Changes - **Removed legacy fallback**: `get_serialized_event_file_committers` now only calls `_get_serialized_committers_from_group_owners` - **Dynamic suspect commit type**: Uses `suspectCommitStrategy` from GroupOwner context instead of hardcoding `INTEGRATION_COMMIT` - `RELEASE_BASED` → `"via commit in release"` - `SCM_BASED` → `"via SCM integration"` - **Preserved API contract**: Still returns 404 when no committers found (not empty array) ## Behavior Changes ### Before ```python # 1. Check GroupOwners first # 2. If no GroupOwners, fall back to release-based analysis (legacy logic) # 3. Analyze stacktrace frames against recent release commits # 4. Always returned "via SCM integration" regardless of actual strategy # 5. Any number of suspect commits can be returned ``` ### After ```python # 1. Check GroupOwners only - all suspect commit strategies create GroupOwners when the Issue is first processed # 2. If no GroupOwners, return 404 "No committers found" # 3. Use actual commit strategy from GroupOwner context # 4. Only 1 suspect commit can be returned ```
1 parent 8f23299 commit 7be1981

File tree

8 files changed

+384
-894
lines changed

8 files changed

+384
-894
lines changed

src/sentry/api/endpoints/event_file_committers.py

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,6 @@
66
from sentry.api.api_publish_status import ApiPublishStatus
77
from sentry.api.base import region_silo_endpoint
88
from sentry.api.bases.project import ProjectEndpoint
9-
from sentry.models.commit import Commit
10-
from sentry.models.group import Group
11-
from sentry.models.release import Release
129
from sentry.services import eventstore
1310
from sentry.utils.committers import get_serialized_event_file_committers
1411

@@ -39,18 +36,10 @@ def get(self, request: Request, project, event_id) -> Response:
3936
elif event.group_id is None:
4037
raise NotFound(detail="Issue not found")
4138

42-
try:
43-
committers = get_serialized_event_file_committers(
44-
project, event, frame_limit=int(request.GET.get("frameLimit", 25))
45-
)
39+
committers = get_serialized_event_file_committers(project, event)
4640

47-
# TODO(nisanthan): Remove the Group.DoesNotExist and Release.DoesNotExist once Commit Context goes GA
48-
except Group.DoesNotExist:
49-
raise NotFound(detail="Issue not found")
50-
except Release.DoesNotExist:
51-
raise NotFound(detail="Release not found")
52-
except Commit.DoesNotExist:
53-
raise NotFound(detail="No Commits found for Release")
41+
if not committers:
42+
raise NotFound(detail="No committers found")
5443

5544
data = {
5645
"committers": committers,

src/sentry/integrations/utils/commit_context.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ def find_commit_context_for_event_all_frames(
122122
has_too_recent_blames=has_too_recent_blames,
123123
)
124124

125-
return (selected_blame, selected_install)
125+
return selected_blame, selected_install
126126

127127

128128
def get_or_create_commit_from_blame(

src/sentry/utils/committers.py

Lines changed: 12 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,13 @@
1616
from sentry.models.commit import Commit
1717
from sentry.models.commitfilechange import CommitFileChange
1818
from sentry.models.group import Group
19-
from sentry.models.groupowner import GroupOwner, GroupOwnerType
19+
from sentry.models.groupowner import GroupOwner, GroupOwnerType, SuspectCommitStrategy
2020
from sentry.models.project import Project
2121
from sentry.models.release import Release
2222
from sentry.models.releasecommit import ReleaseCommit
2323
from sentry.services.eventstore.models import Event, GroupEvent
2424
from sentry.users.services.user.service import user_service
25-
from sentry.utils import metrics
26-
from sentry.utils.event_frames import find_stack_frames, get_sdk_name, munged_filename_and_frames
25+
from sentry.utils.event_frames import find_stack_frames, munged_filename_and_frames
2726
from sentry.utils.hashlib import hash_values
2827

2928
PATH_SEPARATORS = frozenset(["/", "\\"])
@@ -188,6 +187,14 @@ def _get_serialized_committers_from_group_owners(
188187
if serialized_owners:
189188
author = serialized_owners[0]
190189

190+
# Map the suspect commit strategy to the appropriate type
191+
strategy = owner.context.get("suspectCommitStrategy")
192+
if strategy == SuspectCommitStrategy.RELEASE_BASED:
193+
suspect_commit_type = SuspectCommitType.RELEASE_COMMIT.value
194+
else:
195+
# Default to SCM integration for SCM_BASED or any other/unknown strategy
196+
suspect_commit_type = SuspectCommitType.INTEGRATION_COMMIT.value
197+
191198
return [
192199
{
193200
"author": author,
@@ -196,7 +203,7 @@ def _get_serialized_committers_from_group_owners(
196203
commit,
197204
serializer=CommitSerializer(
198205
exclude=["author"],
199-
type=SuspectCommitType.INTEGRATION_COMMIT.value,
206+
type=suspect_commit_type,
200207
),
201208
)
202209
],
@@ -360,59 +367,14 @@ def get_serialized_committers(project: Project, group_id: int) -> Sequence[Autho
360367
def get_serialized_event_file_committers(
361368
project: Project,
362369
event: Event | GroupEvent,
363-
frame_limit: int = 25,
364370
) -> Sequence[AuthorCommitsSerialized]:
365371
if event.group_id is None:
366372
return []
367373
result = _get_serialized_committers_from_group_owners(project, event.group_id)
368374
if result is not None:
369375
return result
370-
371-
# TODO(nisanthan): We create GroupOwner records for
372-
# legacy Suspect Commits in process_suspect_commits task.
373-
# We should refactor to query GroupOwner rather than recalculate.
374-
# But we need to store the commitId and a way to differentiate
375-
# if the Suspect Commit came from ReleaseCommits or CommitContext.
376376
else:
377-
event_frames = get_frame_paths(event)
378-
sdk_name = get_sdk_name(event.data)
379-
committers = get_event_file_committers(
380-
project,
381-
event.group_id,
382-
event_frames,
383-
event.platform,
384-
frame_limit=frame_limit,
385-
sdk_name=sdk_name,
386-
)
387-
commits = [commit for committer in committers for commit in committer["commits"]]
388-
serialized_commits: Sequence[MutableMapping[str, Any]] = serialize(
389-
[c for (c, score) in commits],
390-
serializer=CommitSerializer(
391-
exclude=["author"],
392-
type=SuspectCommitType.RELEASE_COMMIT.value,
393-
),
394-
)
395-
396-
serialized_commits_by_id = {}
397-
398-
for (commit, score), serialized_commit in zip(commits, serialized_commits):
399-
serialized_commit["score"] = score
400-
serialized_commits_by_id[commit.id] = serialized_commit
401-
402-
serialized_committers: list[AuthorCommitsSerialized] = []
403-
for committer in committers:
404-
commit_ids = [commit.id for (commit, _) in committer["commits"]]
405-
commits_result = [serialized_commits_by_id[commit_id] for commit_id in commit_ids]
406-
serialized_committers.append(
407-
{"author": committer["author"], "commits": dedupe_commits(commits_result)}
408-
)
409-
410-
metrics.incr(
411-
"feature.owners.has-committers",
412-
instance="hit" if committers else "miss",
413-
skip_internal=False,
414-
)
415-
return serialized_committers
377+
return []
416378

417379

418380
def dedupe_commits(

tests/sentry/api/endpoints/test_event_committers.py

Lines changed: 78 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from django.urls import reverse
22

3-
from sentry.models.groupowner import GroupOwner, GroupOwnerType
3+
from sentry.models.commitauthor import CommitAuthor
4+
from sentry.models.groupowner import GroupOwner, GroupOwnerType, SuspectCommitStrategy
45
from sentry.models.pullrequest import PullRequest
56
from sentry.models.repository import Repository
67
from sentry.testutils.cases import APITestCase
@@ -19,18 +20,32 @@ def test_simple(self) -> None:
1920

2021
project = self.create_project()
2122

22-
release = self.create_release(project, self.user)
2323
min_ago = before_now(minutes=1).isoformat()
2424
event = self.store_event(
2525
data={
2626
"fingerprint": ["group1"],
2727
"timestamp": min_ago,
28-
"release": release.version,
2928
},
3029
project_id=project.id,
3130
default_event_type=EventType.DEFAULT,
3231
)
3332

33+
# Create a commit and GroupOwner to simulate SCM-based suspect commit detection
34+
repo = self.create_repo(project=project, name="example/repo")
35+
commit = self.create_commit(project=project, repo=repo)
36+
assert event.group is not None
37+
GroupOwner.objects.create(
38+
group_id=event.group.id,
39+
project=project,
40+
organization_id=project.organization_id,
41+
type=GroupOwnerType.SUSPECT_COMMIT.value,
42+
user_id=self.user.id,
43+
context={
44+
"commitId": commit.id,
45+
"suspectCommitStrategy": SuspectCommitStrategy.RELEASE_BASED,
46+
},
47+
)
48+
3449
url = reverse(
3550
"sentry-api-0-event-file-committers",
3651
kwargs={
@@ -44,10 +59,10 @@ def test_simple(self) -> None:
4459
assert response.status_code == 200, response.content
4560
assert len(response.data["committers"]) == 1
4661
assert response.data["committers"][0]["author"]["username"] == "admin@localhost"
47-
assert len(response.data["committers"][0]["commits"]) == 1
48-
assert (
49-
response.data["committers"][0]["commits"][0]["message"] == "placeholder commit message"
50-
)
62+
commits = response.data["committers"][0]["commits"]
63+
assert len(commits) == 1
64+
assert commits[0]["message"] == commit.message
65+
assert commits[0]["suspectCommitType"] == "via commit in release"
5166

5267
def test_no_group(self) -> None:
5368
self.login_as(user=self.user)
@@ -74,7 +89,8 @@ def test_no_group(self) -> None:
7489
assert response.status_code == 404, response.content
7590
assert response.data["detail"] == "Issue not found"
7691

77-
def test_no_release(self) -> None:
92+
def test_no_committers(self) -> None:
93+
"""Test that events without GroupOwners return 404"""
7894
self.login_as(user=self.user)
7995

8096
project = self.create_project()
@@ -95,51 +111,9 @@ def test_no_release(self) -> None:
95111

96112
response = self.client.get(url, format="json")
97113
assert response.status_code == 404, response.content
98-
assert response.data["detail"] == "Release not found"
99-
100-
def test_null_stacktrace(self) -> None:
101-
self.login_as(user=self.user)
102-
103-
project = self.create_project()
104-
105-
release = self.create_release(project, self.user)
106-
107-
min_ago = before_now(minutes=1).isoformat()
108-
event = self.store_event(
109-
data={
110-
"fingerprint": ["group1"],
111-
"environment": "production",
112-
"type": "default",
113-
"exception": {
114-
"values": [
115-
{
116-
"type": "ValueError",
117-
"value": "My exception value",
118-
"module": "__builtins__",
119-
"stacktrace": None,
120-
}
121-
]
122-
},
123-
"tags": [["environment", "production"], ["sentry:release", release.version]],
124-
"release": release.version,
125-
"timestamp": min_ago,
126-
},
127-
project_id=project.id,
128-
)
129-
130-
url = reverse(
131-
"sentry-api-0-event-file-committers",
132-
kwargs={
133-
"event_id": event.event_id,
134-
"project_id_or_slug": event.project.slug,
135-
"organization_id_or_slug": event.project.organization.slug,
136-
},
137-
)
114+
assert response.data["detail"] == "No committers found"
138115

139-
response = self.client.get(url, format="json")
140-
assert response.status_code == 200, response.content
141-
142-
def test_with_commit_context(self) -> None:
116+
def test_with_committers(self) -> None:
143117
self.login_as(user=self.user)
144118
self.repo = Repository.objects.create(
145119
organization_id=self.organization.id,
@@ -242,9 +216,60 @@ def test_with_commit_context_pull_request(self) -> None:
242216

243217
response = self.client.get(url, format="json")
244218
assert response.status_code == 200, response.content
245-
246219
commits = response.data["committers"][0]["commits"]
247220
assert len(commits) == 1
248221
assert "pullRequest" in commits[0]
249222
assert commits[0]["pullRequest"]["id"] == pull_request.key
250223
assert commits[0]["suspectCommitType"] == "via SCM integration"
224+
225+
def test_endpoint_with_no_user_groupowner(self) -> None:
226+
"""Test API endpoint returns commit author fallback for GroupOwner with user_id=None."""
227+
self.login_as(user=self.user)
228+
project = self.create_project()
229+
230+
min_ago = before_now(minutes=1).isoformat()
231+
event = self.store_event(
232+
data={"fingerprint": ["group1"], "timestamp": min_ago},
233+
project_id=project.id,
234+
default_event_type=EventType.DEFAULT,
235+
)
236+
237+
# Create commit with external author and GroupOwner with user_id=None
238+
repo = self.create_repo(project=project, name="example/repo")
239+
commit_author = CommitAuthor.objects.create(
240+
organization_id=project.organization_id,
241+
name="External Dev",
242+
243+
)
244+
commit = self.create_commit(project=project, repo=repo, author=commit_author)
245+
assert event.group is not None
246+
GroupOwner.objects.create(
247+
group_id=event.group.id,
248+
project=project,
249+
organization_id=project.organization_id,
250+
type=GroupOwnerType.SUSPECT_COMMIT.value,
251+
user_id=None, # No Sentry user mapping
252+
context={
253+
"commitId": commit.id,
254+
"suspectCommitStrategy": SuspectCommitStrategy.RELEASE_BASED,
255+
},
256+
)
257+
258+
url = reverse(
259+
"sentry-api-0-event-file-committers",
260+
kwargs={
261+
"event_id": event.event_id,
262+
"project_id_or_slug": event.project.slug,
263+
"organization_id_or_slug": event.project.organization.slug,
264+
},
265+
)
266+
267+
response = self.client.get(url, format="json")
268+
assert response.status_code == 200, response.content
269+
270+
# Should return commit author fallback
271+
author = response.data["committers"][0]["author"]
272+
assert author["email"] == "[email protected]"
273+
assert author["name"] == "External Dev"
274+
assert "username" not in author # No Sentry user fields
275+
assert "id" not in author # No Sentry user fields

0 commit comments

Comments
 (0)