chore(sentry apps): add SLO context manager for send alert event (issue alerts) #86356
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #86356 +/- ##
==========================================
+ Coverage 87.79% 87.95% +0.15%
==========================================
Files 9782 9753 -29
Lines 554157 553677 -480
Branches 21730 21292 -438
==========================================
+ Hits 486528 486972 +444
+ Misses 67237 66323 -914
+ Partials 392 382 -10 |
…o instead of wrapping make new contxt manager
| @@ -210,7 +203,7 @@ def send_alert_webhook( | |||
| "alert_rule_ui_component_webhook.sent", | |||
| organization_id=organization.id, | |||
| sentry_app_id=sentry_app_id, | |||
| event=f"{request_data.resource}.{request_data.action}", | |||
| event=str(SentryAppEventType.EVENT_ALERT_TRIGGERED), | |||
There was a problem hiding this comment.
nit: don't need to cast a StrEnum to str
| @@ -498,6 +491,7 @@ def send_resource_change_webhook( | |||
| def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): | |||
| for f in futures: | |||
| if not f.kwargs.get("sentry_app"): | |||
| logger.info("notify_sentry_app.future_missing_sentry_app", extra={"future": f}) | |||
There was a problem hiding this comment.
should we raise an exception? calling action_inst.after (which calls notify_sentry_app for sentry app actions) is wrapped in safe_execute so i believe raising an error will log an exception
sentry/src/sentry/rules/processing/processor.py
Lines 164 to 168 in be3950b
i suppose this means the action is broken anyway, so hopefully we wouldn't migrate it cc @iamrajjoshi
There was a problem hiding this comment.
if one future is missing a sentry app does that mean the rest are broken? If we raise we'll stop prcessing all the other futures. I can capture here though if that works ?
There was a problem hiding this comment.
you could call logger.error which would capture a sentry error and log, but not raise an exception
There was a problem hiding this comment.
i would be curious if we are ever missing a future in the first place, i have a suspicion that check exists because of mypy
| @@ -498,6 +491,7 @@ def send_resource_change_webhook( | |||
| def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): | |||
| for f in futures: | |||
| if not f.kwargs.get("sentry_app"): | |||
| logger.info("notify_sentry_app.future_missing_sentry_app", extra={"future": f}) | |||
There was a problem hiding this comment.
i would be curious if we are ever missing a future in the first place, i have a suspicion that check exists because of mypy
Christinarlong
added
the
Trigger: Revert
|
PR reverted: fa6491f |
…ent (issue alerts) (#86356)" This reverts commit 7736c94. Co-authored-by: Christinarlong <[email protected]>
After I looked further into the what notify_sentry_app was being was doing and who was calling it, it didn't feel super useful to add a context manager there. So currently the bounds are the
send_alert_eventtask forPREPARE_WEBHOOKandsend_and_save_webhook_requestfor theSEND_WEBHOOKbound