Skip to content

chore(sentryapps): better logging and errors for select requester #79377

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.

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

Merged
merged 7 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import logging

from jsonschema import ValidationError
from rest_framework.request import Request
from rest_framework.response import Response

from sentry.api.api_owners import ApiOwner
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import region_silo_endpoint
from sentry.coreapi import APIError
from sentry.models.project import Project
from sentry.sentry_apps.api.bases.sentryapps import SentryAppInstallationBaseEndpoint
from sentry.sentry_apps.external_requests.select_requester import SelectRequester

logger = logging.getLogger("sentry.sentry-apps")


@region_silo_endpoint
class SentryAppInstallationExternalRequestsEndpoint(SentryAppInstallationBaseEndpoint):
Expand Down Expand Up @@ -36,7 +42,16 @@ def get(self, request: Request, installation) -> Response:

try:
choices = SelectRequester(**kwargs).run()
except Exception:
return Response({"error": "Error communicating with Sentry App service"}, status=400)
except ValidationError as e:
return Response(
{"error": e.message},
status=400,
)
except APIError:
message = f'Error retrieving select field options from {request.GET.get("uri")}'
return Response(
{"error": message},
status=400,
)

return Response(choices)
30 changes: 25 additions & 5 deletions src/sentry/sentry_apps/external_requests/select_requester.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from uuid import uuid4

from django.utils.functional import cached_property
from jsonschema import ValidationError

from sentry.coreapi import APIError
from sentry.http import safe_urlread
Expand Down Expand Up @@ -36,9 +37,10 @@ class SelectRequester:
def run(self) -> dict[str, Any]:
response: list[dict[str, str]] = []
try:
url = self._build_url()
body = safe_urlread(
send_and_save_sentry_app_request(
self._build_url(),
url,
self.sentry_app,
self.install.organization_id,
"select_options.requested",
Expand All @@ -54,13 +56,24 @@ def run(self) -> dict[str, Any]:
"sentry_app_slug": self.sentry_app.slug,
"install_uuid": self.install.uuid,
"project_slug": self.project_slug,
"uri": self.uri,
"error_message": str(e),
"url": url,
},
)
raise APIError from e

if not self._validate_response(response) or not response:
raise APIError(
if not self._validate_response(response):
logger.info(
"select-requester.invalid-response",
extra={
"response": response,
"sentry_app_slug": self.sentry_app.slug,
"install_uuid": self.install.uuid,
"project_slug": self.project_slug,
"url": url,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't uri enough for debugging here? what added value does url have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the final url would come in handy in checking who we are contacting, and what the final query stirng is (since for this request we tend to have a bunch of query params).
It is somewhat redundant with the uri though, so I'd probably prefer to keep the url to debug with

},
)
raise ValidationError(
f"Invalid response format for SelectField in {self.sentry_app} from uri: {self.uri}"
)
return self._format_response(response)
Expand Down Expand Up @@ -95,7 +108,14 @@ def _format_response(self, resp: list[dict[str, Any]]) -> dict[str, Any]:

for option in resp:
if not ("value" in option and "label" in option):
raise APIError("Missing `value` or `label` in option data for SelectField")
logger.info(
"select-requester.invalid-response",
extra={
"resposnse": resp,
"error_msg": "Missing `value` or `label` in option data for SelectField",
},
)
raise ValidationError("Missing `value` or `label` in option data for SelectField")

choices.append([option["value"], option["label"]])

Expand Down
3 changes: 2 additions & 1 deletion src/sentry/sentry_apps/models/sentry_app_installation.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from django.db import models
from django.db.models import QuerySet
from django.utils import timezone
from jsonschema import ValidationError

from sentry.auth.services.auth import AuthenticatedToken
from sentry.backup.scopes import RelocationScope
Expand Down Expand Up @@ -241,6 +242,6 @@ def prepare_ui_component(
component=component, install=installation, project_slug=project_slug, values=values
).run()
return component
except APIError:
except (APIError, ValidationError):
# TODO(nisanthan): For now, skip showing the UI Component if the API requests fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol! This TODO seems very old and important. Basically what to do here is to return the error and surface it to the user. Future PRs though.

return None
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest
import responses
from jsonschema import ValidationError

from sentry.coreapi import APIError
from sentry.sentry_apps.external_requests.select_requester import SelectRequester
Expand Down Expand Up @@ -69,7 +70,7 @@ def test_invalid_response_missing_label(self):
content_type="application/json",
)

with pytest.raises(APIError):
with pytest.raises(ValidationError):
SelectRequester(
install=self.install,
project_slug=self.project.slug,
Expand All @@ -90,7 +91,7 @@ def test_invalid_response_missing_value(self):
content_type="application/json",
)

with pytest.raises(APIError):
with pytest.raises(ValidationError):
SelectRequester(
install=self.install,
project_slug=self.project.slug,
Expand Down
Loading