Skip to content
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

ref(sentry apps): Use new error design for sentry app errors #83355

Merged
merged 8 commits into from
Jan 17, 2025

Conversation

Christinarlong
Copy link
Contributor

@Christinarlong Christinarlong commented Jan 14, 2025

See this notion doc for the design changes
tl:dr No wrapping errors, sentry app errors take in extras dictionary for additional context

Also a callout: If possible please give feedback if any data is sensitive and shouldn't be sent to one party or the other

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 14, 2025
@Christinarlong Christinarlong marked this pull request as ready for review January 14, 2025 17:53
@Christinarlong Christinarlong requested review from a team as code owners January 14, 2025 17:53
src/sentry/sentry_apps/api/bases/sentryapps.py Outdated Show resolved Hide resolved
src/sentry/sentry_apps/api/bases/sentryapps.py Outdated Show resolved Hide resolved

# Represents a user/client error that occured during a Sentry App process
class SentryAppError(SentryAppBaseError):
error_type = SentryAppErrorType.CLIENT
Copy link
Member

Choose a reason for hiding this comment

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

are you using error_type anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need error_type in the future for Alert Rule Action UI process/ creation because it's an RPC method and we need to send error context in the RPC response.

src/sentry/sentry_apps/api/bases/sentryapps.py Outdated Show resolved Hide resolved
src/sentry/sentry_apps/api/bases/sentryapps.py Outdated Show resolved Hide resolved
@@ -131,7 +128,13 @@ def handle_exception_with_details(self, request, exc, handler_context=None, scop

def _handle_sentry_app_exception(self, exception: Exception):
if isinstance(exception, SentryAppIntegratorError) or isinstance(exception, SentryAppError):
Copy link
Member

Choose a reason for hiding this comment

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

are you planning on sending public context and webhook context separately in here?

Copy link
Contributor Author

@Christinarlong Christinarlong Jan 14, 2025

Choose a reason for hiding this comment

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

yes the idea is to make some send_webhook_context_to_integrators(exception) and call that here. webhook_context & public_context will both go to integrators via that send function.

Comment on lines 132 to 133
public_context = exception.public_context
if public_context:
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can use the walrus operator

Suggested change
public_context = exception.public_context
if public_context:
if public_context := exception.public_context:

@Christinarlong Christinarlong merged commit ad6ad5f into master Jan 17, 2025
49 checks passed
@Christinarlong Christinarlong deleted the crl/new-sa-error-w-fixes branch January 17, 2025 21:40
Copy link

sentry-io bot commented Jan 18, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ SentryAppSentryError: Failed to create external issue obj /api/0/sentry-app-installations/{uuid}/external... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants