ref(sentry apps): Improve Errors for Alert Rule Creation for Sentry Apps #82067

Christinarlong · 2024-12-12T23:46:19Z

Introduces two classes to represent Sentry App exceptions:
SentryAppError should be used to represent client side errors like failed validation. For the webhook, these should not be sent to the integrator.
SentryAppIntegratorError is used to represent errors between Sentry & the integrator, i.e connection failed, integrator response is incorrect etc. These should be sent to the integrator for the webhook.

For this PR, we want to start wrapping errors in the SentryApp flows with these distinctions. And then in the endpoints we want to catch these two specific errors and return the error message. More info here in notion

This PR is a split of #81877 since that was turning into 2 PRs into 1

Context for Alert Rule Sentry App creation: Notion doc

src/sentry/sentry_apps/utils/alert_rule_action.py

+        trigger_sentry_app_action_creators_for_incidents(serialized_data)
+    except (SentryAppError, SentryAppIntegratorError) as e:
+        return Response(
+            str(e),


To fix the problem, we need to ensure that detailed exception messages are not exposed to the end user. Instead, we should log the detailed error message on the server and return a generic error message to the user. This can be achieved by modifying the exception handling code to log the exception and return a generic error message.

Modify the exception handling code in the create_sentry_app_alert_rule_component_for_incidents function to log the detailed error message and return a generic error message.

Modify the exception handling code in the create_sentry_app_alert_rule_issues_component function to log the detailed error message and return a generic error message.

src/sentry/sentry_apps/utils/alert_rule_action.py

+
+    except (SentryAppError, SentryAppIntegratorError) as e:
+        return Response(
+            {"actions": [str(e)]},


To fix the problem, we need to ensure that detailed exception messages are not exposed to the user. Instead, we should log the detailed error message on the server and return a generic error message to the user. This can be achieved by modifying the exception handling code to log the exception and return a generic error message.

Modify the exception handling block to log the detailed error message using sentry_sdk.capture_exception(e).

Return a generic error message to the user.

codecov · 2024-12-13T00:17:01Z

Codecov Report

Attention: Patch coverage is 90.67797% with 11 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/sentry_apps/utils/alert_rule_action.py	91.66%	5 Missing ⚠️
...s/external_requests/alert_rule_action_requester.py	72.72%	3 Missing ⚠️
src/sentry/sentry_apps/services/app/impl.py	75.00%	2 Missing ⚠️
...rc/sentry/sentry_apps/alert_rule_action_creator.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #82067       +/-   ##
===========================================
+ Coverage   52.94%   80.37%   +27.42%     
===========================================
  Files        7247     7273       +26     
  Lines      319932   321339     +1407     
  Branches    20944    20944               
===========================================
+ Hits       169397   258264    +88867     
+ Misses     150133    62673    -87460     
  Partials      402      402

sentaur-athena · 2024-12-19T17:32:41Z

src/sentry/sentry_apps/utils/alert_rule_action.py

+        trigger_sentry_app_action_creators_for_incidents(serialized_data)
+    except (SentryAppError, SentryAppIntegratorError) as e:
+        return Response(
+            str(e),


Add a comment here for future engineers about why this is safe.

sentaur-athena · 2024-12-19T17:34:58Z

src/sentry/sentry_apps/utils/alert_rule_action.py

+            raise Exception(result.message)
+
+
+def create_sentry_app_alert_rule_component_for_incidents(


It is confusing to me that one of the create component methods you have here returns a component and one of them return an error and None on success.

[1] I don't see much benefit in this method. How about you directly call trigger_sentry_app_action_creators_for_incidents and let the exception go through.

sentaur-athena · 2024-12-19T17:40:23Z

src/sentry/api/endpoints/project_rule_details.py

@@ -285,7 +285,9 @@ def put(self, request: Request, project, rule) -> Response:
                context = {"uuid": client.uuid}
                return Response(context, status=202)

-            trigger_sentry_app_action_creators_for_issues(actions=kwargs["actions"])
+            result = create_sentry_app_alert_rule_issues_component(actions=kwargs["actions"])
+            if isinstance(result, Response):


It's not good practice to have different typed returned from a method and then decide what to do with it based on type. I have a suggestion below on how to improve this. Please read the comments annotated with [n] in the order on n.

sentaur-athena · 2024-12-19T17:46:28Z

src/sentry/api/endpoints/project_rule_details.py

@@ -285,7 +285,9 @@ def put(self, request: Request, project, rule) -> Response:
                context = {"uuid": client.uuid}
                return Response(context, status=202)

-            trigger_sentry_app_action_creators_for_issues(actions=kwargs["actions"])
+            result = create_sentry_app_alert_rule_issues_component(actions=kwargs["actions"])


[2] Then wrap this in try catch and then in the catch, catch the exception and call a method like return IntegrationResponse.from_exception() where this method is a util that does all the work you're doing to turn exceptions into response.

If from_exception() needs to know the caller for some reason to parse the errors better you can pass that as a param.

sentaur-athena · 2024-12-19T17:46:58Z

src/sentry/incidents/endpoints/organization_alert_rule_details.py

@@ -82,7 +84,12 @@ def update_alert_rule(request: Request, organization, alert_rule):
        partial=True,
    )
    if serializer.is_valid():
-        trigger_sentry_app_action_creators_for_incidents(serializer.validated_data)
+        raised_error = create_sentry_app_alert_rule_component_for_incidents(


All comments applies to this method as well.

Christinarlong added 10 commits December 12, 2024 12:53

dawg idk

aeb7176

dawg idk

3002ef5

switch back to alertruleactionresult

afd18d8

update arc process to return alertruleactionresult

67bc16f

update refs and arar tests

2f0c14f

fix sentry apps tests

223ff0e

fix typing

a0b764a

circluar imports :dies and adding tests for metric alerts

b8331a9

add tests for project rules

9e41d24

revert errors for non alert rule processes

72a683c

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 12, 2024

vercel bot deployed to Preview December 12, 2024 23:47 View deployment

bruh why did this pass

2c946f4

vercel bot deployed to Preview December 12, 2024 23:53 View deployment

github-advanced-security bot found potential problems Dec 12, 2024

View reviewed changes

fix typing

b84d813

vercel bot deployed to Preview December 13, 2024 00:20 View deployment

Christinarlong marked this pull request as ready for review December 13, 2024 00:39

Christinarlong requested review from a team as code owners December 13, 2024 00:39

sentaur-athena reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(sentry apps): Improve Errors for Alert Rule Creation for Sentry Apps #82067

ref(sentry apps): Improve Errors for Alert Rule Creation for Sentry Apps #82067

Christinarlong commented Dec 12, 2024 •

edited

Loading

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

codecov bot commented Dec 13, 2024 •

edited

Loading

sentaur-athena Dec 19, 2024

sentaur-athena Dec 19, 2024

sentaur-athena Dec 19, 2024

sentaur-athena Dec 19, 2024

sentaur-athena Dec 19, 2024

sentaur-athena Dec 19, 2024

@@ -41,4 +41,5 @@
                 except (SentryAppError, SentryAppIntegratorError) as e:
+                    sentry_sdk.capture_exception(e)
                     return Response(
-                        str(e),
+                        "An error occurred while processing your request.",
                         status=status.HTTP_400_BAD_REQUEST,
@@ -61,4 +62,5 @@
                 except (SentryAppError, SentryAppIntegratorError) as e:
+                    sentry_sdk.capture_exception(e)
                     return Response(
-                        {"actions": [str(e)]},
+                        {"actions": ["An error occurred while processing your request."]},
                         status=status.HTTP_400_BAD_REQUEST,

		raise Exception(result.message)


		def create_sentry_app_alert_rule_component_for_incidents(

ref(sentry apps): Improve Errors for Alert Rule Creation for Sentry Apps #82067

Are you sure you want to change the base?

ref(sentry apps): Improve Errors for Alert Rule Creation for Sentry Apps #82067

Conversation

Christinarlong commented Dec 12, 2024 • edited Loading

codecov bot commented Dec 13, 2024 • edited Loading

Codecov Report

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

sentaur-athena Dec 19, 2024

Choose a reason for hiding this comment

Christinarlong commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading