chore(sentry apps): New error design for external request flow #83678
Conversation
github-actions
bot
added
the
Scope: Backend
❌ 11 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| ).run() | ||
|
|
||
| assert ( | ||
| str(exception_info.value) == "Missing `value` or `label` in option data for SelectField" |
There was a problem hiding this comment.
These are duplicate tests that test cases already covered in prev ones
| except (APIError, ValidationError, APIUnauthorized) as e: | ||
| return Response({"error": str(e)}, status=400) | ||
| except Exception as e: | ||
| error_id = capture_exception(e) | ||
| return Response( | ||
| { | ||
| "error": f"Something went wrong while trying to link issue. Sentry error ID: {error_id}" | ||
| }, | ||
| status=500, | ||
| ) |
There was a problem hiding this comment.
is there a reason we're no longer capturing these errors?
There was a problem hiding this comment.
error handler in the base IntegrationPlatformEndpoint (link) will do the catching for us
| 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, | ||
| ) |
There was a problem hiding this comment.
same here, any reason we're not capturing these anymore?
There was a problem hiding this comment.
same as above
| }, | ||
| status_code=503, |
There was a problem hiding this comment.
should a validation error raise 503? i think this should be 500 if we can't extract the json
There was a problem hiding this comment.
503 was mainly chosen to represent some failure that can't be fixed on the client side because of an issue due to the third party. Though I couldn't really find a status code that matched cleanly with the 3p error case, so open to suggestions.
For the 500 code, if we can't extract the JSON, I assumed that meant the 3p was giving back some bad format like HTML or smthn. In that case the error isn't really Sentry's fault, which I think 500 represents something went wrong with Sentry. 🤔
There was a problem hiding this comment.
if i am understanding this correctly, this error would be when the 3p sends us an invalid payload? could we return 400 then?
There was a problem hiding this comment.
400 is a client error code though. My understanding is that for client error codes/4XXs, indicate the "client"/person who submitted the request did something wrong and that if they change their request they can get back a 200. A la they can correct the mistake themselves.
In this case though, if a 3p is not sending us the right format like HTML or smthn for a request, it's unlikely the end user will be able to correct the situation themselves by filling out say the form a different way.
| raise SentryAppIntegratorError( | ||
| message=f"Issue occured while trying to contact {self.sentry_app.slug} to link issue", | ||
| webhook_context={"error_type": error_type, **extras}, | ||
| status_code=503, |
There was a problem hiding this comment.
should this be 500? normal exceptions that get raised and are uncaught by django error handling return 500
There was a problem hiding this comment.
We can catch RequestExceptions here instead of all Exceptions. It's probably better to have the uncaughts go through so we can get alerts and fine tune. Alternatively, we could have other non RequestException exceptions go under SentryAppSentryError which would also return a 500 and be a bit nicer
| raise SentryAppIntegratorError( | ||
| message=f"Something went wrong while getting options for Select FormField from {self.sentry_app.slug}", | ||
| webhook_context={"error_type": message, **extra}, | ||
| status_code=503, |
There was a problem hiding this comment.
We don't really want a 400 here since it's a 3p request failure
| }, | ||
| status_code=503, |
There was a problem hiding this comment.
should this be a 400 if it's during validation?
There was a problem hiding this comment.
Went with 503, since the client can't fix the issue/ the 3p response was incorrect so the 3p has to fix.
| @@ -45,7 +43,9 @@ def post(self, request: Request, installation) -> Response: | |||
| data = request.data.copy() | |||
|
|
|||
| if not {"groupId", "action", "uri"}.issubset(data.keys()): | |||
| return Response(status=400) | |||
| raise SentryAppError( | |||
| message="Missing groupId, action, uri in request body", status_code=400 | |||
There was a problem hiding this comment.
nit: maybe we tell them exactly whats missing 🤔
| }, | ||
| status_code=503, |
There was a problem hiding this comment.
if i am understanding this correctly, this error would be when the 3p sends us an invalid payload? could we return 400 then?
No description provided.