Skip to content

ref(integrations): Remove duplicate search lifecycle #129471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
giovanni-guidini wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ref(integrations): Remove duplicate search lifecycle #129471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/sentry/integrations/source_code_management/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ class SCMIntegrationInteractionType(StrEnum):
# SourceCodeSearchEndpoint
HANDLE_SEARCH_ISSUES = "handle_search_issues"
HANDLE_SEARCH_REPOSITORIES = "handle_search_repositories"
GET = "get"

# CommitContextIntegration
GET_BLAME_FOR_FILES = "get_blame_for_files"
Expand Down Expand Up @@ -208,13 +207,9 @@ class LinkAllReposHaltReason(StrEnum):

class SourceCodeSearchEndpointHaltReason(StrEnum):
"""
Reasons why a SourceCodeSearchEndpoint method (handle_search_issues,
handle_search_repositories, or get) may halt without success/failure.
Reasons why a SourceCodeSearchEndpoint search handler may halt without success/failure.
"""

NO_ISSUE_TRACKER = "no_issue_tracker"
RATE_LIMITED = "rate_limited"
MISSING_REPOSITORY_OR_NO_ACCESS = "missing_repository_or_no_access"
MISSING_INTEGRATION = "missing_integration"
SERIALIZER_ERRORS = "serializer_errors"
MISSING_REPOSITORY_FIELD = "missing_repository_field"
101 changes: 44 additions & 57 deletions src/sentry/integrations/source_code_management/search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
from sentry.integrations.source_code_management.metrics import (
SCMIntegrationInteractionEvent,
SCMIntegrationInteractionType,
SourceCodeSearchEndpointHaltReason,
)
from sentry.integrations.types import IntegrationProviderSlug
from sentry.organizations.services.organization import RpcOrganization
Expand Down Expand Up @@ -95,59 +94,47 @@ def handle_search_field(
def get(
self, request: Request, organization: RpcOrganization, integration_id: int, **kwds: Any
) -> Response:
with self.record_event(
SCMIntegrationInteractionType.GET,
organization_id=organization.id,
integration_id=integration_id,
).capture() as lifecycle:
integration_query = Q(
organizationintegration__organization_id=organization.id, id=integration_id
)

if self.integration_provider:
integration_query &= Q(provider=self.integration_provider)
try:
integration: Integration = Integration.objects.get(integration_query)
except Integration.DoesNotExist:
lifecycle.record_halt(str(SourceCodeSearchEndpointHaltReason.MISSING_INTEGRATION))
return Response(status=404)

serializer = SourceCodeSearchSerializer(data=request.query_params)
if not serializer.is_valid():
lifecycle.record_halt(str(SourceCodeSearchEndpointHaltReason.SERIALIZER_ERRORS))
return self.respond(serializer.errors, status=400)

field = serializer.validated_data["field"]
query = serializer.validated_data["query"]

installation = integration.get_installation(organization.id)
if not isinstance(installation, self.installation_class):
raise NotFound(
f"Integration by that id is not of type {self.integration_provider}."
)

if field == self.issue_field:
repo = None

if self.repository_field: # only fetch repository
repo = request.GET.get(self.repository_field)
if repo is None:
lifecycle.record_halt(
str(SourceCodeSearchEndpointHaltReason.MISSING_REPOSITORY_FIELD)
)
return Response(
{"detail": f"{self.repository_field} is a required parameter"},
status=400,
)

return self.handle_search_issues(installation, query, repo)

if self.repository_field and field == self.repository_field:
return self.handle_search_repositories(integration, installation, query)

repo = request.GET.get(self.repository_field) if self.repository_field else None
response = self.handle_search_field(installation, field, query, repo)
if response is not None:
return response

return Response({"detail": "Invalid field"}, status=400)
integration_query = Q(
organizationintegration__organization_id=organization.id, id=integration_id
)

if self.integration_provider:
integration_query &= Q(provider=self.integration_provider)
try:
integration: Integration = Integration.objects.get(integration_query)
except Integration.DoesNotExist:
return Response(status=404)

serializer = SourceCodeSearchSerializer(data=request.query_params)
if not serializer.is_valid():
return self.respond(serializer.errors, status=400)

field = serializer.validated_data["field"]
query = serializer.validated_data["query"]

installation = integration.get_installation(organization.id)
if not isinstance(installation, self.installation_class):
raise NotFound(f"Integration by that id is not of type {self.integration_provider}.")

if field == self.issue_field:
repo = None

if self.repository_field: # only fetch repository
repo = request.GET.get(self.repository_field)
if repo is None:
return Response(
{"detail": f"{self.repository_field} is a required parameter"},
status=400,
)

return self.handle_search_issues(installation, query, repo)

if self.repository_field and field == self.repository_field:
return self.handle_search_repositories(integration, installation, query)

repo = request.GET.get(self.repository_field) if self.repository_field else None
response = self.handle_search_field(installation, field, query, repo)
if response is not None:
return response

return Response({"detail": "Invalid field"}, status=400)
34 changes: 10 additions & 24 deletions tests/sentry/integrations/bitbucket/test_search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,15 @@ def test_search_issues(self, mock_record: MagicMock) -> None:
{"label": "#456 Issue Title 456", "value": "456"},
]

assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6

# first 2 are middleware calls to ensure control silo, then the next one and the last one are also middleware calls for get_response_from_control_silo
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = product_calls
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.SUCCESS
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
start, success = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert success.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand All @@ -87,14 +84,9 @@ def test_search_repositories(self, mock_record: MagicMock) -> None:
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = (
product_calls # calls get, which calls handle_search_repositories
)
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.SUCCESS
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
start, success = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert success.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand All @@ -115,13 +107,7 @@ def test_search_repositories_no_issue_tracker(self, mock_record: MagicMock) -> N
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]

start1, start2, halt1, halt2 = product_calls # calls get, which calls handle_search_issues
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.HALTED
start, halt = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert halt.args[0] == EventLifecycleOutcome.HALTED
assert_halt_metric(mock_record, SourceCodeSearchEndpointHaltReason.NO_ISSUE_TRACKER.value)
# NOTE: handle_search_issues returns without raising an API error, so for the
# purposes of logging the GET request completes successfully
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
106 changes: 28 additions & 78 deletions tests/sentry/integrations/github/test_search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from sentry.testutils.asserts import (
assert_halt_metric,
assert_middleware_metrics,
assert_slo_metric_calls,
)
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import control_silo_test
Expand Down Expand Up @@ -98,16 +97,12 @@ def test_finds_external_issue_results(self, mock_record: MagicMock) -> None:
{"value": 25, "label": "#25 AEIOU Error"},
{"value": 45, "label": "#45 AEIOU Error"},
]
assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)
product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = product_calls
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.SUCCESS
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
start, success = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert success.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
def test_finds_external_issue_results_with_id(self) -> None:
Expand Down Expand Up @@ -143,17 +138,12 @@ def test_finds_repo_results(self, mock_record: MagicMock) -> None:
{"value": "test/example", "label": "example"},
{"value": "test/exhaust", "label": "exhaust"},
]
assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)
product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = (
product_calls # calls get, which calls handle_search_repositories
)
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.SUCCESS
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
start, success = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert success.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
def test_prefetches_repo_results_with_empty_query(self) -> None:
Expand Down Expand Up @@ -380,20 +370,15 @@ def test_repo_search_validation_error(self, mock_record: MagicMock) -> None:
resp = self.client.get(self.url, data={"field": "repo", "query": "nope", "repo": "example"})
assert resp.status_code == 404
assert "detail" in resp.data
assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)
product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = product_calls
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.HALTED
start, halt = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert halt.args[0] == EventLifecycleOutcome.HALTED
assert_halt_metric(
mock_record, SourceCodeSearchEndpointHaltReason.MISSING_REPOSITORY_OR_NO_ACCESS.value
)
# NOTE: handle_search_repositories returns without raising an API error, so for the
# purposes of logging the GET request completes successfully
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
def test_finds_no_external_issues_results(self) -> None:
Expand Down Expand Up @@ -433,18 +418,13 @@ def test_search_issues_rate_limit(self, mock_record: MagicMock) -> None:
self.url, data={"field": "externalIssue", "query": "ex", "repo": "example"}
)
assert resp.status_code == 429
assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)
product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = product_calls
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.HALTED
start, halt = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert halt.args[0] == EventLifecycleOutcome.HALTED
assert_halt_metric(mock_record, SourceCodeSearchEndpointHaltReason.RATE_LIMITED.value)
# NOTE: handle_search_issues returns without raising an API error, so for the
# purposes of logging the GET request completes successfully
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS

@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand All @@ -460,61 +440,38 @@ def test_search_project_rate_limit(self, mock_record: MagicMock) -> None:
)
resp = self.client.get(self.url, data={"field": "repo", "query": "ex"})
assert resp.status_code == 429
assert len(mock_record.mock_calls) == 8
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)
product_calls = mock_record.mock_calls[3:-1]
start1, start2, halt1, halt2 = product_calls
assert start1.args[0] == EventLifecycleOutcome.STARTED
assert start2.args[0] == EventLifecycleOutcome.STARTED
assert halt1.args[0] == EventLifecycleOutcome.HALTED
start, halt = mock_record.mock_calls[3:-1]
assert start.args[0] == EventLifecycleOutcome.STARTED
assert halt.args[0] == EventLifecycleOutcome.HALTED
assert_halt_metric(mock_record, SourceCodeSearchEndpointHaltReason.RATE_LIMITED.value)
# NOTE: handle_search_repositories returns without raising an API error, so for the
# purposes of logging the GET request completes successfully
assert halt2.args[0] == EventLifecycleOutcome.SUCCESS

# Request Validations
# Test observability requests for GET requests failures here
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_missing_field(self, mock_record: MagicMock) -> None:
resp = self.client.get(self.url, data={"query": "XYZ"})
assert resp.status_code == 400
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]

assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
assert_slo_metric_calls(product_calls, EventLifecycleOutcome.HALTED)
assert_halt_metric(mock_record, SourceCodeSearchEndpointHaltReason.SERIALIZER_ERRORS.value)
assert len(mock_record.mock_calls) == 4
assert_middleware_metrics(mock_record.mock_calls)

@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_missing_query(self, mock_record: MagicMock) -> None:
resp = self.client.get(self.url, data={"field": "externalIssue"})

assert resp.status_code == 400
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
assert_slo_metric_calls(product_calls, EventLifecycleOutcome.HALTED)
assert_halt_metric(mock_record, SourceCodeSearchEndpointHaltReason.SERIALIZER_ERRORS.value)
assert len(mock_record.mock_calls) == 4
assert_middleware_metrics(mock_record.mock_calls)

@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
def test_missing_repository(self, mock_record: MagicMock) -> None:
resp = self.client.get(self.url, data={"field": "externalIssue", "query": "XYZ"})

assert resp.status_code == 400
assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
assert_slo_metric_calls(product_calls, EventLifecycleOutcome.HALTED)
assert_halt_metric(
mock_record, SourceCodeSearchEndpointHaltReason.MISSING_REPOSITORY_FIELD.value
)
assert len(mock_record.mock_calls) == 4
assert_middleware_metrics(mock_record.mock_calls)

def test_invalid_field(self) -> None:
resp = self.client.get(self.url, data={"field": "invalid-field", "query": "nope"})
Expand All @@ -538,15 +495,8 @@ def test_missing_integration(self, mock_record: MagicMock) -> None:

assert resp.status_code == 404

assert len(mock_record.mock_calls) == 6
middleware_calls = mock_record.mock_calls[:3] + mock_record.mock_calls[-1:]
assert_middleware_metrics(middleware_calls)

product_calls = mock_record.mock_calls[3:-1]
assert_slo_metric_calls(product_calls, EventLifecycleOutcome.HALTED)
assert_halt_metric(
mock_record, SourceCodeSearchEndpointHaltReason.MISSING_INTEGRATION.value
)
assert len(mock_record.mock_calls) == 4
assert_middleware_metrics(mock_record.mock_calls)

def test_missing_installation(self) -> None:
# remove organization integration aka "uninstalling" installation
Expand Down
Loading
Loading