Browse Source

feat(suspect-commits): Remove suspect commit recalculation period (when all-frames is enabled) (#58415)

Closes https://github.com/getsentry/sentry/issues/57438

We are no longer going to run the `process_commit_context` task 
when new events come in. To increase accuracy, we want to calculate 
the suspect commit only at the time the issue was created. 
Otherwise we might select commits that were made _after_ the issue started.
Malachi Willey 1 year ago
parent
commit
f421feefb9
2 changed files with 76 additions and 2 deletions
  1. 8 0
      src/sentry/tasks/post_process.py
  2. 68 2
      tests/sentry/tasks/test_post_process.py

+ 8 - 0
src/sentry/tasks/post_process.py

@@ -1026,6 +1026,14 @@ def process_commits(job: PostProcessJob) -> None:
                     features.has("organizations:commit-context", event.project.organization)
                     features.has("organizations:commit-context", event.project.organization)
                     and has_integrations
                     and has_integrations
                 ):
                 ):
+                    if (
+                        features.has(
+                            "organizations:suspect-commits-all-frames", event.project.organization
+                        )
+                        and not job["group_state"]["is_new"]
+                    ):
+                        return
+
                     cache_key = DEBOUNCE_CACHE_KEY(event.group_id)
                     cache_key = DEBOUNCE_CACHE_KEY(event.group_id)
                     if cache.get(cache_key):
                     if cache.get(cache_key):
                         metrics.incr("sentry.tasks.process_commit_context.debounce")
                         metrics.incr("sentry.tasks.process_commit_context.debounce")

+ 68 - 2
tests/sentry/tasks/test_post_process.py

@@ -19,6 +19,7 @@ from sentry.buffer.redis import RedisBuffer
 from sentry.eventstore.models import Event
 from sentry.eventstore.models import Event
 from sentry.eventstore.processing import event_processing_store
 from sentry.eventstore.processing import event_processing_store
 from sentry.ingest.transaction_clusterer import ClustererNamespace
 from sentry.ingest.transaction_clusterer import ClustererNamespace
+from sentry.integrations.mixins.commit_context import CommitInfo, FileBlameInfo
 from sentry.issues.escalating import manage_issue_states
 from sentry.issues.escalating import manage_issue_states
 from sentry.issues.grouptype import PerformanceNPlusOneGroupType, ProfileFileIOGroupType
 from sentry.issues.grouptype import PerformanceNPlusOneGroupType, ProfileFileIOGroupType
 from sentry.issues.ingest import save_issue_occurrence
 from sentry.issues.ingest import save_issue_occurrence
@@ -1379,8 +1380,7 @@ class ProcessCommitsTestMixin(BasePostProgressGroupMixin):
         )
         )
         self.cache_key = write_event_to_cache(self.created_event)
         self.cache_key = write_event_to_cache(self.created_event)
         self.repo = self.create_repo(
         self.repo = self.create_repo(
-            name="example",
-            integration_id=self.integration.id,
+            name="example", integration_id=self.integration.id, provider="github"
         )
         )
         self.code_mapping = self.create_code_mapping(
         self.code_mapping = self.create_code_mapping(
             repo=self.repo, project=self.project, stack_root="src/"
             repo=self.repo, project=self.project, stack_root="src/"
@@ -1394,6 +1394,23 @@ class ProcessCommitsTestMixin(BasePostProgressGroupMixin):
             message="placeholder commit message",
             message="placeholder commit message",
         )
         )
 
 
+        self.github_blame_all_files_return_value = [
+            FileBlameInfo(
+                code_mapping=self.code_mapping,
+                lineno=39,
+                path="sentry/models/release.py",
+                ref="master",
+                repo=self.repo,
+                commit=CommitInfo(
+                    commitId="asdfwreqr",
+                    committedDate=(datetime.now(timezone.utc) - timedelta(days=2)),
+                    commitMessage="placeholder commit message",
+                    commitAuthorName="",
+                    commitAuthorEmail="admin@localhost",
+                ),
+            )
+        ]
+
     @with_feature("organizations:commit-context")
     @with_feature("organizations:commit-context")
     @patch(
     @patch(
         "sentry.integrations.github.GitHubIntegration.get_commit_context",
         "sentry.integrations.github.GitHubIntegration.get_commit_context",
@@ -1434,6 +1451,55 @@ class ProcessCommitsTestMixin(BasePostProgressGroupMixin):
             )
             )
         assert not cache.has_key(f"process-commit-context-{self.created_event.group_id}")
         assert not cache.has_key(f"process-commit-context-{self.created_event.group_id}")
 
 
+    @with_feature("organizations:commit-context")
+    @with_feature("organizations:suspect-commits-all-frames")
+    @patch("sentry.integrations.github.GitHubIntegration.get_commit_context_all_frames")
+    def test_skip_when_not_is_new(self, mock_get_commit_context):
+        """
+        Tests that when the organizations:suspect-commits-all-frames feature is enabled,
+        and the group is not new, that we do not process commit context.
+        """
+        with self.tasks():
+            self.call_post_process_group(
+                is_new=False,
+                is_regression=False,
+                is_new_group_environment=True,
+                event=self.created_event,
+            )
+        assert not mock_get_commit_context.called
+        assert not GroupOwner.objects.filter(
+            group=self.created_event.group,
+            project=self.created_event.project,
+            organization=self.created_event.project.organization,
+            type=GroupOwnerType.SUSPECT_COMMIT.value,
+        ).exists()
+
+    @with_feature("organizations:commit-context")
+    @with_feature("organizations:suspect-commits-all-frames")
+    @patch(
+        "sentry.integrations.github.GitHubIntegration.get_commit_context_all_frames",
+    )
+    def test_does_not_skip_when_is_new(self, mock_get_commit_context):
+        """
+        Tests that when the organizations:suspect-commits-all-frames feature is enabled,
+        and the group is new, the commit context should be processed.
+        """
+        mock_get_commit_context.return_value = self.github_blame_all_files_return_value
+        with self.tasks():
+            self.call_post_process_group(
+                is_new=True,
+                is_regression=False,
+                is_new_group_environment=True,
+                event=self.created_event,
+            )
+        assert mock_get_commit_context.called
+        assert GroupOwner.objects.get(
+            group=self.created_event.group,
+            project=self.created_event.project,
+            organization=self.created_event.project.organization,
+            type=GroupOwnerType.SUSPECT_COMMIT.value,
+        )
+
 
 
 class SnoozeTestMixin(BasePostProgressGroupMixin):
 class SnoozeTestMixin(BasePostProgressGroupMixin):
     @with_feature("organizations:escalating-issues")
     @with_feature("organizations:escalating-issues")