diff --git a/src/sentry/integrations/source_code_management/metrics.py b/src/sentry/integrations/source_code_management/metrics.py index a2059ad06e80..58a5732b0fad 100644 --- a/src/sentry/integrations/source_code_management/metrics.py +++ b/src/sentry/integrations/source_code_management/metrics.py @@ -50,6 +50,8 @@ class SCMIntegrationInteractionType(StrEnum): # Releases COMPARE_COMMITS = "compare_commits" + RESOLVE_ISSUE_VIA_COMMIT = "resolve_issue_via_commit" + RESOLVE_ISSUE_VIA_PR = "resolve_issue_via_pr" # Status Checks CREATE_STATUS_CHECK = "create_status_check" @@ -62,6 +64,10 @@ class SCMIntegrationInteractionType(StrEnum): GET_REPO_TREE = "get_repo_tree" +class SCMResolutionHaltReason(StrEnum): + ISSUE_ALREADY_RESOLVED = "issue_already_resolved" + + @dataclass class SCMIntegrationInteractionEvent(IntegrationEventLifecycleMetric): """ diff --git a/src/sentry/models/releases/set_commits.py b/src/sentry/models/releases/set_commits.py index 02b2959c9c90..651bfe74fbeb 100644 --- a/src/sentry/models/releases/set_commits.py +++ b/src/sentry/models/releases/set_commits.py @@ -3,12 +3,18 @@ import itertools import logging import re +from contextlib import nullcontext from typing import Any, TypedDict from django.db import IntegrityError, router from sentry.constants import ObjectStatus from sentry.db.postgres.transactions import in_test_hide_transaction_boundary +from sentry.integrations.source_code_management.metrics import ( + SCMIntegrationInteractionEvent, + SCMIntegrationInteractionType, + SCMResolutionHaltReason, +) from sentry.issues.action_log import SYSTEM_ACTOR, ActionSource, action_context_scope from sentry.locks import locks from sentry.models.activity import Activity @@ -200,11 +206,6 @@ def update_group_resolutions(release, commit_author_by_commit): ).values_list("group_id", "linked_id") ) - commit_group_authors = [ - (cr[0], commit_author_by_commit.get(cr[1])) - for cr in commit_resolutions # group_id - ] - # repository_id is fetched here so we don't need a second PullRequest query # below to resolve providers (pull_request_resolutions is always a subset). pr_repo_ids = dict( @@ -232,20 +233,20 @@ def update_group_resolutions(release, commit_author_by_commit): pr_authors_dict = {pra.id: pra.author for pra in pr_authors} - pull_request_group_authors = [ - (prr[0], pr_authors_dict.get(prr[1])) for prr in pull_request_resolutions - ] - # Map each resolved group to the (normalized) provider of the commit/PR that # resolved it, so the issue_resolved analytics event records e.g. "github". # commit -> repository_id comes for free from the select_related above; # pr -> repository_id was already collected in pr_repo_ids above. commit_repo_ids = {rc["commit_id"]: rc["commit__repository_id"] for rc in release_commits} - repo_providers = dict( - Repository.objects.filter( + repo_details = { + repo_id: (provider, integration_id) + for repo_id, provider, integration_id in Repository.objects.filter( id__in=set(commit_repo_ids.values()) | set(pr_repo_ids.values()) - ).values_list("id", "provider") - ) + ).values_list("id", "provider", "integration_id") + } + repo_providers = { + repo_id: provider for repo_id, (provider, _integration_id) in repo_details.items() + } # Iterate PRs first, commits second, so a commit-derived provider wins when a # group is resolved by both. The repo_id guards prevent a repo-less link from # overwriting a valid provider with None. @@ -276,14 +277,37 @@ def update_group_resolutions(release, commit_author_by_commit): user_by_author: dict[CommitAuthor | None, RpcUser | None] = {None: None} - commits_and_prs = list(itertools.chain(commit_group_authors, pull_request_group_authors)) - group_project_lookup = dict( - Group.objects.filter(id__in=[group_id for group_id, _ in commits_and_prs]).values_list( - "id", "project_id" + resolution_attempts = list( + itertools.chain( + ( + ( + group_id, + commit_author_by_commit.get(commit_id), + SCMIntegrationInteractionType.RESOLVE_ISSUE_VIA_COMMIT, + commit_repo_ids.get(commit_id), + ) + for group_id, commit_id in commit_resolutions + ), + ( + ( + group_id, + pr_authors_dict.get(pr_id), + SCMIntegrationInteractionType.RESOLVE_ISSUE_VIA_PR, + pr_repo_ids.get(pr_id), + ) + for group_id, pr_id in pull_request_resolutions + ), ) ) + group_project_lookup = dict( + Group.objects.filter( + id__in=[ + group_id for group_id, _author, _interaction_type, _repo_id in resolution_attempts + ] + ).values_list("id", "project_id") + ) - for group_id, author in commits_and_prs: + for group_id, author, interaction_type, repo_id in resolution_attempts: if author is not None and author not in user_by_author: try: user_by_author[author] = author.find_users()[0] @@ -291,44 +315,68 @@ def update_group_resolutions(release, commit_author_by_commit): user_by_author[author] = None actor = user_by_author[author] - with atomic_transaction( - using=( - router.db_for_write(GroupResolution), - router.db_for_write(Group), - # inside the remove_group_from_inbox - router.db_for_write(GroupInbox), - router.db_for_write(Activity), - ) - ): - resolution, _ = GroupResolution.objects.update_or_create( - group_id=group_id, - defaults={ - "release": release, - "type": GroupResolution.Type.in_release, - "status": GroupResolution.Status.resolved, - "actor_id": actor.id if actor is not None else None, - }, + provider, integration_id = ( + repo_details.get(repo_id, (None, None)) if repo_id is not None else (None, None) + ) + normalized_provider = normalize_provider_key(provider) + event = ( + SCMIntegrationInteractionEvent( + interaction_type=interaction_type, + provider_key=normalized_provider, + organization_id=release.organization_id, + integration_id=integration_id, ) - group = Group.objects.get(id=group_id) - should_create_resolution_activity = group.status != GroupStatus.RESOLVED - group.update(status=GroupStatus.RESOLVED, substatus=None) - remove_group_from_inbox(group, action=GroupInboxRemoveAction.RESOLVED, user=actor) - if should_create_resolution_activity: - activity_data: dict[str, Any] = {"version": release.version} - resolution_commit_id = commit_id_by_group.get(group_id) - if resolution_commit_id is not None: - activity_data["commit"] = resolution_commit_id - Activity.objects.create( - project_id=group.project_id, - group=group, - type=ActivityType.SET_RESOLVED_IN_RELEASE.value, - user_id=actor.id if actor is not None else None, - ident=resolution.id, - data=activity_data, + if interaction_type is not None and normalized_provider is not None + else None + ) + lifecycle_context = ( + event.capture(assume_success=False) if event is not None else nullcontext() + ) + with lifecycle_context as lifecycle: + with atomic_transaction( + using=( + router.db_for_write(GroupResolution), + router.db_for_write(Group), + # inside the remove_group_from_inbox + router.db_for_write(GroupInbox), + router.db_for_write(Activity), ) - record_group_history(group, GroupHistoryStatus.RESOLVED, actor=actor) - - metrics.incr("group.resolved", instance="in_commit", skip_internal=True) + ): + resolution, _ = GroupResolution.objects.update_or_create( + group_id=group_id, + defaults={ + "release": release, + "type": GroupResolution.Type.in_release, + "status": GroupResolution.Status.resolved, + "actor_id": actor.id if actor is not None else None, + }, + ) + group = Group.objects.get(id=group_id) + should_create_resolution_activity = group.status != GroupStatus.RESOLVED + group.update(status=GroupStatus.RESOLVED, substatus=None) + remove_group_from_inbox(group, action=GroupInboxRemoveAction.RESOLVED, user=actor) + if should_create_resolution_activity: + activity_data: dict[str, Any] = {"version": release.version} + resolution_commit_id = commit_id_by_group.get(group_id) + if resolution_commit_id is not None: + activity_data["commit"] = resolution_commit_id + Activity.objects.create( + project_id=group.project_id, + group=group, + type=ActivityType.SET_RESOLVED_IN_RELEASE.value, + user_id=actor.id if actor is not None else None, + ident=resolution.id, + data=activity_data, + ) + record_group_history(group, GroupHistoryStatus.RESOLVED, actor=actor) + + metrics.incr("group.resolved", instance="in_commit", skip_internal=True) + + if lifecycle is not None: + if should_create_resolution_activity: + lifecycle.record_success() + else: + lifecycle.record_halt(SCMResolutionHaltReason.ISSUE_ALREADY_RESOLVED) issue_resolved.send_robust( organization_id=release.organization_id, diff --git a/tests/sentry/models/test_release.py b/tests/sentry/models/test_release.py index 4d592a540210..4735080f82c4 100644 --- a/tests/sentry/models/test_release.py +++ b/tests/sentry/models/test_release.py @@ -1,5 +1,5 @@ from datetime import timedelta -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest from django.core.exceptions import ValidationError @@ -10,6 +10,7 @@ from sentry.api.release_search import INVALID_SEMVER_MESSAGE from sentry.exceptions import InvalidSearchQuery from sentry.integrations.models.external_issue import ExternalIssue +from sentry.integrations.source_code_management.metrics import SCMIntegrationInteractionType from sentry.issues.action_log import SYSTEM_ACTOR, ActionSource from sentry.issues.action_log.types import SetResolvedInReleaseAction from sentry.models.commit import Commit @@ -695,15 +696,21 @@ def test_commit_resolves_only_when_released_to_issue_project(self) -> None: assert resolution.status == GroupResolution.Status.resolved assert Group.objects.get(id=frontend_group.id).status == GroupStatus.RESOLVED + @patch("sentry.models.releases.set_commits.SCMIntegrationInteractionEvent") @patch("sentry.analytics.record") @receivers_raise_on_send() - def test_resolution_records_commit_provider(self, mock_record: MagicMock) -> None: + def test_resolution_records_commit_provider( + self, mock_record: MagicMock, mock_lifecycle_event: MagicMock + ) -> None: org = self.create_organization(owner=Factories.create_user()) project = self.create_project(organization=org, name="foo") group = self.create_group(project=project) add_group_to_inbox(group, GroupInboxReason.MANUAL) repo = Repository.objects.create( - organization_id=org.id, name="test/repo", provider="integrations:github" + organization_id=org.id, + name="test/repo", + provider="integrations:github", + integration_id=123, ) commit = Commit.objects.create( organization_id=org.id, @@ -731,6 +738,19 @@ def test_resolution_records_commit_provider(self, mock_record: MagicMock) -> Non issue_category=group.issue_category.name.lower(), ), ) + mock_lifecycle_event.assert_called_once_with( + interaction_type=SCMIntegrationInteractionType.RESOLVE_ISSUE_VIA_COMMIT, + provider_key="github", + organization_id=org.id, + integration_id=repo.integration_id, + ) + mock_lifecycle_event.return_value.capture.assert_called_once_with(assume_success=False) + capture_context = mock_lifecycle_event.return_value.capture.return_value + lifecycle = capture_context.__enter__.return_value + lifecycle.record_success.assert_called_once_with() + assert capture_context.mock_calls.index( + call.__enter__().record_success() + ) < capture_context.mock_calls.index(call.__exit__(None, None, None)) @patch("sentry.analytics.record") @receivers_raise_on_send() @@ -785,9 +805,12 @@ def test_resolution_attributes_to_system(self) -> None: actor=SYSTEM_ACTOR, ) + @patch("sentry.models.releases.set_commits.SCMIntegrationInteractionEvent") @patch("sentry.analytics.record") @receivers_raise_on_send() - def test_resolution_via_pull_request_records_merge_commit(self, mock_record: MagicMock) -> None: + def test_resolution_via_pull_request_records_merge_commit( + self, mock_record: MagicMock, mock_lifecycle_event: MagicMock + ) -> None: """A group resolved by a pull request records the PR's merge commit id on the IssueResolvedEvent, resolved through the release's matching commit.""" org = self.create_organization(owner=Factories.create_user()) @@ -795,7 +818,10 @@ def test_resolution_via_pull_request_records_merge_commit(self, mock_record: Mag group = self.create_group(project=project) add_group_to_inbox(group, GroupInboxReason.MANUAL) repo = Repository.objects.create( - organization_id=org.id, name="test/repo", provider="integrations:github" + organization_id=org.id, + name="test/repo", + provider="integrations:github", + integration_id=123, ) commit = Commit.objects.create(organization_id=org.id, repository_id=repo.id, key="b" * 40) pull_request = self.create_pull_request(repository_id=repo.id, organization_id=org.id) @@ -827,6 +853,15 @@ def test_resolution_via_pull_request_records_merge_commit(self, mock_record: Mag issue_category=group.issue_category.name.lower(), ), ) + mock_lifecycle_event.assert_called_once_with( + interaction_type=SCMIntegrationInteractionType.RESOLVE_ISSUE_VIA_PR, + provider_key="github", + organization_id=org.id, + integration_id=repo.integration_id, + ) + mock_lifecycle_event.return_value.capture.assert_called_once_with(assume_success=False) + lifecycle = mock_lifecycle_event.return_value.capture.return_value.__enter__.return_value + lifecycle.record_success.assert_called_once_with() @patch("sentry.integrations.example.integration.ExampleIntegration.sync_status_outbound") @receivers_raise_on_send()