Skip to content

Commit 8566bef

Browse files
cathtengcathteng
authored
chore(ecosystem): add SLO for fetching PR diffs for open PR comments (#95812)
1 parent cdc72db commit 8566bef

File tree

6 files changed

+55
-166
lines changed

6 files changed

+55
-166
lines changed

‎src/sentry/integrations/github/integration.py

Lines changed: 7 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
)
3030
from sentry.integrations.github.constants import ISSUE_LOCKED_ERROR_MESSAGE, RATE_LIMITED_MESSAGE
3131
from sentry.integrations.github.tasks.link_all_repos import link_all_repos
32-
from sentry.integrations.github.tasks.utils import GithubAPIErrorType
3332
from sentry.integrations.models.integration import Integration
3433
from sentry.integrations.models.organization_integration import OrganizationIntegration
3534
from sentry.integrations.pipeline import IntegrationPipeline
@@ -39,13 +38,11 @@
3938
from sentry.integrations.source_code_management.commit_context import (
4039
OPEN_PR_MAX_FILES_CHANGED,
4140
OPEN_PR_MAX_LINES_CHANGED,
42-
OPEN_PR_METRICS_BASE,
4341
CommitContextIntegration,
4442
OpenPRCommentWorkflow,
4543
PRCommentWorkflow,
4644
PullRequestFile,
4745
PullRequestIssue,
48-
_open_pr_comment_log,
4946
)
5047
from sentry.integrations.source_code_management.language_parsers import (
5148
get_patch_parsers_for_organization,
@@ -477,48 +474,13 @@ class GitHubOpenPRCommentWorkflow(OpenPRCommentWorkflow):
477474

478475
def safe_for_comment(self, repo: Repository, pr: PullRequest) -> list[dict[str, Any]]:
479476
client = self.integration.get_client()
480-
logger.info(
481-
_open_pr_comment_log(
482-
integration_name=self.integration.integration_name, suffix="check_safe_for_comment"
483-
)
484-
)
485477
try:
486478
pr_files = client.get_pullrequest_files(repo=repo.name, pull_number=pr.key)
487479
except ApiError as e:
488-
logger.info(
489-
_open_pr_comment_log(
490-
integration_name=self.integration.integration_name, suffix="api_error"
491-
)
492-
)
493-
if e.json and RATE_LIMITED_MESSAGE in e.json.get("message", ""):
494-
metrics.incr(
495-
OPEN_PR_METRICS_BASE.format(
496-
integration=self.integration.integration_name, key="api_error"
497-
),
498-
tags={"type": GithubAPIErrorType.RATE_LIMITED.value, "code": e.code},
499-
)
500-
elif e.code == 404:
501-
metrics.incr(
502-
OPEN_PR_METRICS_BASE.format(
503-
integration=self.integration.integration_name, key="api_error"
504-
),
505-
tags={"type": GithubAPIErrorType.MISSING_PULL_REQUEST.value, "code": e.code},
506-
)
480+
if (e.json and RATE_LIMITED_MESSAGE in e.json.get("message", "")) or e.code == 404:
481+
return []
507482
else:
508-
metrics.incr(
509-
OPEN_PR_METRICS_BASE.format(
510-
integration=self.integration.integration_name, key="api_error"
511-
),
512-
tags={"type": GithubAPIErrorType.UNKNOWN.value, "code": e.code},
513-
)
514-
logger.exception(
515-
_open_pr_comment_log(
516-
integration_name=self.integration.integration_name,
517-
suffix="unknown_api_error",
518-
),
519-
extra={"error": str(e)},
520-
)
521-
return []
483+
raise
522484

523485
changed_file_count = 0
524486
changed_lines_count = 0
@@ -538,21 +500,10 @@ def safe_for_comment(self, repo: Repository, pr: PullRequest) -> list[dict[str,
538500
changed_lines_count += file["changes"]
539501
filtered_pr_files.append(file)
540502

541-
if changed_file_count > OPEN_PR_MAX_FILES_CHANGED:
542-
metrics.incr(
543-
OPEN_PR_METRICS_BASE.format(
544-
integration=self.integration.integration_name, key="rejected_comment"
545-
),
546-
tags={"reason": "too_many_files"},
547-
)
548-
return []
549-
if changed_lines_count > OPEN_PR_MAX_LINES_CHANGED:
550-
metrics.incr(
551-
OPEN_PR_METRICS_BASE.format(
552-
integration=self.integration.integration_name, key="rejected_comment"
553-
),
554-
tags={"reason": "too_many_lines"},
555-
)
503+
if (
504+
changed_file_count > OPEN_PR_MAX_FILES_CHANGED
505+
or changed_lines_count > OPEN_PR_MAX_LINES_CHANGED
506+
):
556507
return []
557508

558509
return filtered_pr_files
@@ -566,14 +517,6 @@ def get_pr_files(self, pr_files: list[dict[str, Any]]) -> list[PullRequestFile]:
566517
if "patch" in file
567518
]
568519

569-
logger.info(
570-
_open_pr_comment_log(
571-
integration_name=self.integration.integration_name,
572-
suffix="pr_filenames",
573-
),
574-
extra={"count": len(pullrequest_files)},
575-
)
576-
577520
return pullrequest_files
578521

579522
def get_pr_files_safe_for_comment(
@@ -582,19 +525,6 @@ def get_pr_files_safe_for_comment(
582525
pr_files = self.safe_for_comment(repo=repo, pr=pr)
583526

584527
if len(pr_files) == 0:
585-
logger.info(
586-
_open_pr_comment_log(
587-
integration_name=self.integration.integration_name,
588-
suffix="not_safe_for_comment",
589-
),
590-
extra={"file_count": len(pr_files)},
591-
)
592-
metrics.incr(
593-
OPEN_PR_METRICS_BASE.format(
594-
integration=self.integration.integration_name, key="error"
595-
),
596-
tags={"type": "unsafe_for_comment"},
597-
)
598528
return []
599529

600530
return self.get_pr_files(pr_files)

‎src/sentry/integrations/gitlab/integration.py

Lines changed: 2 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -312,33 +312,10 @@ def safe_for_comment(self, repo: Repository, pr: PullRequest) -> list[dict[str,
312312
try:
313313
diffs = client.get_pr_diffs(repo=repo, pr=pr)
314314
except ApiError as e:
315-
logger.info(
316-
_open_pr_comment_log(
317-
integration_name=self.integration.integration_name, suffix="api_error"
318-
)
319-
)
320315
if e.code == 404:
321-
metrics.incr(
322-
OPEN_PR_METRICS_BASE.format(
323-
integration=self.integration.integration_name, key="api_error"
324-
),
325-
tags={"type": "missing_pr", "code": e.code},
326-
)
316+
return []
327317
else:
328-
metrics.incr(
329-
OPEN_PR_METRICS_BASE.format(
330-
integration=self.integration.integration_name, key="api_error"
331-
),
332-
tags={"type": "unknown_api_error", "code": e.code},
333-
)
334-
logger.exception(
335-
_open_pr_comment_log(
336-
integration_name=self.integration.integration_name,
337-
suffix="unknown_api_error",
338-
),
339-
extra={"error": str(e)},
340-
)
341-
return []
318+
raise
342319

343320
changed_file_count = 0
344321
changed_lines_count = 0
@@ -401,33 +378,12 @@ def get_pr_files_safe_for_comment(
401378
pr_diffs = self.safe_for_comment(repo=repo, pr=pr)
402379

403380
if len(pr_diffs) == 0:
404-
logger.info(
405-
_open_pr_comment_log(
406-
integration_name=self.integration.integration_name,
407-
suffix="not_safe_for_comment",
408-
),
409-
extra={"file_count": len(pr_diffs)},
410-
)
411-
metrics.incr(
412-
OPEN_PR_METRICS_BASE.format(
413-
integration=self.integration.integration_name, key="error"
414-
),
415-
tags={"type": "unsafe_for_comment"},
416-
)
417381
return []
418382

419383
pr_files = [
420384
PullRequestFile(filename=diff["new_path"], patch=diff["diff"]) for diff in pr_diffs
421385
]
422386

423-
logger.info(
424-
_open_pr_comment_log(
425-
integration_name=self.integration.integration_name,
426-
suffix="pr_filenames",
427-
),
428-
extra={"count": len(pr_files)},
429-
)
430-
431387
return pr_files
432388

433389
def get_comment_data(self, comment_body: str) -> dict[str, Any]:

‎src/sentry/integrations/source_code_management/metrics.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ class SCMIntegrationInteractionType(StrEnum):
3838
CREATE_COMMENT = "create_comment"
3939
UPDATE_COMMENT = "update_comment"
4040
QUEUE_COMMENT_TASK = "queue_comment_task"
41+
GET_PR_DIFFS = "get_pr_diffs" # open PR comments
4142

4243
# Tasks
4344
LINK_ALL_REPOS = "link_all_repos"

‎src/sentry/integrations/source_code_management/tasks.py

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@
1616
from sentry.integrations.source_code_management.language_parsers import (
1717
get_patch_parsers_for_organization,
1818
)
19+
from sentry.integrations.source_code_management.metrics import (
20+
CommitContextIntegrationInteractionEvent,
21+
SCMIntegrationInteractionType,
22+
)
1923
from sentry.models.options.organization_option import OrganizationOption
2024
from sentry.models.organization import Organization
2125
from sentry.models.project import Project
@@ -236,7 +240,7 @@ def open_pr_comment_workflow(pr_id: int) -> None:
236240
)
237241
return
238242

239-
# check integration exists to hit Github API with client
243+
# check integration exists to hit external API with client
240244
integration = integration_service.get_integration(
241245
integration_id=repo.integration_id, status=ObjectStatus.ACTIVE
242246
)
@@ -269,12 +273,16 @@ def open_pr_comment_workflow(pr_id: int) -> None:
269273
)
270274
return
271275

272-
# CREATING THE COMMENT
273-
274-
# fetch the files in the PR and determine if it is safe to comment
275-
pullrequest_files = open_pr_comment_workflow.get_pr_files_safe_for_comment(
276-
repo=repo, pr=pull_request
277-
)
276+
with CommitContextIntegrationInteractionEvent(
277+
interaction_type=SCMIntegrationInteractionType.GET_PR_DIFFS,
278+
provider_key=integration_name,
279+
repository=repo,
280+
pull_request_id=pull_request.id,
281+
).capture():
282+
# fetch the files in the PR and determine if it is safe to comment
283+
pullrequest_files = open_pr_comment_workflow.get_pr_files_safe_for_comment(
284+
repo=repo, pr=pull_request
285+
)
278286

279287
issue_table_contents = {}
280288
top_issues_per_file = []

‎tests/sentry/integrations/github/tasks/test_open_pr_comment.py

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,6 @@ def test_too_many_files(self):
151151

152152
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
153153
assert pr_files == [] # not safe
154-
self.mock_integration_metrics.incr.assert_called_with(
155-
"github.open_pr_comment.rejected_comment", tags={"reason": "too_many_files"}
156-
)
157154

158155
@responses.activate
159156
def test_too_many_lines(self):
@@ -169,9 +166,6 @@ def test_too_many_lines(self):
169166

170167
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
171168
assert pr_files == [] # not safe
172-
self.mock_integration_metrics.incr.assert_called_with(
173-
"github.open_pr_comment.rejected_comment", tags={"reason": "too_many_lines"}
174-
)
175169

176170
@responses.activate
177171
def test_too_many_files_and_lines(self):
@@ -194,9 +188,6 @@ def test_too_many_files_and_lines(self):
194188

195189
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
196190
assert pr_files == [] # not safe
197-
self.mock_integration_metrics.incr.assert_any_call(
198-
"github.open_pr_comment.rejected_comment", tags={"reason": "too_many_lines"}
199-
)
200191

201192
@responses.activate
202193
def test_error__rate_limited(self):
@@ -212,9 +203,6 @@ def test_error__rate_limited(self):
212203

213204
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
214205
assert pr_files == [] # not safe
215-
self.mock_integration_metrics.incr.assert_called_with(
216-
"github.open_pr_comment.api_error", tags={"type": "gh_rate_limited", "code": 429}
217-
)
218206

219207
@responses.activate
220208
def test_error__missing_pr(self):
@@ -224,22 +212,15 @@ def test_error__missing_pr(self):
224212

225213
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
226214
assert pr_files == [] # not safe
227-
self.mock_integration_metrics.incr.assert_called_with(
228-
"github.open_pr_comment.api_error",
229-
tags={"type": "missing_gh_pull_request", "code": 404},
230-
)
231215

232216
@responses.activate
233217
def test_error__api_error(self):
234218
responses.add(
235219
responses.GET, self.gh_path.format(pull_number=self.pr.key), status=400, json={}
236220
)
237221

238-
pr_files = self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
239-
assert pr_files == [] # not safe
240-
self.mock_integration_metrics.incr.assert_called_with(
241-
"github.open_pr_comment.api_error", tags={"type": "unknown_api_error", "code": 400}
242-
)
222+
with pytest.raises(ApiError):
223+
self.open_pr_comment_workflow.safe_for_comment(repo=self.gh_repo, pr=self.pr)
243224

244225

245226
class TestGetFilenames(GithubCommentTestCase):
@@ -1137,9 +1118,6 @@ def test_comment_workflow_early_return(
11371118

11381119
pull_request_comment_query = PullRequestComment.objects.all()
11391120
assert len(pull_request_comment_query) == 0
1140-
mock_integration_metrics.incr.assert_called_with(
1141-
"github.open_pr_comment.error", tags={"type": "unsafe_for_comment"}
1142-
)
11431121

11441122
mock_safe_for_comment.return_value = [{}]
11451123
mock_pr_filenames.return_value = [
@@ -1355,6 +1333,3 @@ def test_comment_workflow_not_safe_for_comment(
13551333
open_pr_comment_workflow(self.pr.id)
13561334

13571335
assert not mock_pr_filenames.called
1358-
mock_integration_metrics.incr.assert_called_with(
1359-
"github.open_pr_comment.error", tags={"type": "unsafe_for_comment"}
1360-
)

0 commit comments

Comments
 (0)