Skip to content

chore(sentry apps): add SLO context manager for send alert event (issue alerts) #86356

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 40 commits into from
Mar 14, 2025
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
8299673
inital commit
Christinarlong Feb 28, 2025
b5e833b
add context manager for event webhooks
Christinarlong Feb 28, 2025
f3dde2d
add context manager for event webhooks
Christinarlong Feb 28, 2025
757890f
typing
Christinarlong Feb 28, 2025
e9dacdf
record halts in sending webhook
Christinarlong Feb 28, 2025
a26919d
update tests
Christinarlong Feb 28, 2025
fe4de7f
update tests
Christinarlong Mar 3, 2025
3804a47
create helper for asserting count
Christinarlong Mar 3, 2025
48a67f5
add testing for send webhook
Christinarlong Mar 4, 2025
56d5e90
fix and add tests for event webhook SLOs
Christinarlong Mar 4, 2025
2614eb5
add test for lifecycle halt for published apps
Christinarlong Mar 4, 2025
5188300
Merge branch 'master' into crl/sa-slos-context-manager
Christinarlong Mar 4, 2025
c518499
add context manager to alert prep task
Christinarlong Mar 4, 2025
b68ade6
add context manager send alert webhook
Christinarlong Mar 4, 2025
72419db
fix typing
Christinarlong Mar 5, 2025
c261e25
pr fixes for tasks
Christinarlong Mar 5, 2025
ce6b02f
fix tests via pr comments
Christinarlong Mar 5, 2025
887b1a3
add assertionerror
Christinarlong Mar 5, 2025
91693e7
typing
Christinarlong Mar 5, 2025
0e2c40a
Merge branch 'crl/sa-slos-context-manager' into crl/slo-send-alert-we…
Christinarlong Mar 5, 2025
46f0cee
pr schtuff
Christinarlong Mar 5, 2025
997e954
pr schtuff
Christinarlong Mar 5, 2025
85f7132
shift up event naming to before lifecycle
Christinarlong Mar 6, 2025
1ea4fab
Update src/sentry/sentry_apps/metrics.py
Christinarlong Mar 6, 2025
1d0988a
Merge branch 'crl/sa-slos-context-manager' into crl/slo-send-alert-we…
Christinarlong Mar 6, 2025
cafe800
update tests and remove extra params
Christinarlong Mar 6, 2025
0a6c67b
remove extra context since sentry error will automagically catch
Christinarlong Mar 6, 2025
d6dbd01
add ignore and capture
Christinarlong Mar 6, 2025
33d5da0
add tests for retry decorator
Christinarlong Mar 6, 2025
eb8ced0
add testing that we call retry
Christinarlong Mar 6, 2025
f1ebf57
Merge branch 'crl/add-ignore-and-retry' into crl/sa-slos-context-manager
Christinarlong Mar 7, 2025
c23e962
remove try catches since the retry decorator willl handle for us, als…
Christinarlong Mar 7, 2025
b68a033
Merge branch 'master' into crl/sa-slos-context-manager
Christinarlong Mar 7, 2025
83e94bd
do event checking with cast
Christinarlong Mar 7, 2025
3229f2a
Merge branch 'crl/sa-slos-context-manager' into crl/slo-send-alert-we…
Christinarlong Mar 7, 2025
9dca10e
cast the event alert trifggerdd
Christinarlong Mar 7, 2025
3c16f86
Merge branch 'master' into crl/installation-webhook-slo
Christinarlong Mar 10, 2025
ad51113
Merge branch 'master' into crl/slo-send-alert-webhook
Christinarlong Mar 10, 2025
416e7ca
pr fixes
Christinarlong Mar 11, 2025
f439a44
fix tests
Christinarlong Mar 14, 2025
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
1 change: 1 addition & 0 deletions src/sentry/sentry_apps/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class SentryAppWebhookFailureReason(StrEnum):
INVALID_EVENT = "invalid_event"
MISSING_SERVICEHOOK = "missing_servicehook"
EVENT_NOT_IN_SERVCEHOOK = "event_not_in_servicehook"
MISSING_ISSUE_OCCURRENCE = "missing_issue_occurrence"
MISSING_USER = "missing_user"


Expand Down
124 changes: 59 additions & 65 deletions src/sentry/sentry_apps/tasks/sentry_apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ def _webhook_event_data(
@instrumented_task(name="sentry.sentry_apps.tasks.sentry_apps.send_alert_webhook", **TASK_OPTIONS)
@retry_decorator
def send_alert_webhook(
rule: str,
rule_label: str,
sentry_app_id: int,
instance_id: str,
group_id: int,
Expand All @@ -128,79 +128,72 @@ def send_alert_webhook(
additional_payload: Mapping[str, Any] | None = None,
**kwargs: Any,
):
group = Group.objects.get_from_cache(id=group_id)
assert group, "Group must exist to get related attributes"
project = Project.objects.get_from_cache(id=group.project_id)
organization = Organization.objects.get_from_cache(id=project.organization_id)
extra = {
"sentry_app_id": sentry_app_id,
"project_slug": project.slug,
"organization_slug": organization.slug,
"rule": rule,
}
with SentryAppInteractionEvent(
operation_type=SentryAppInteractionType.PREPARE_WEBHOOK,
event_type=SentryAppEventType.EVENT_ALERT_TRIGGERED,
).capture() as lifecycle:
group = Group.objects.get_from_cache(id=group_id)
assert group, "Group must exist to get related attributes"
project = Project.objects.get_from_cache(id=group.project_id)
organization = Organization.objects.get_from_cache(id=project.organization_id)
extra: dict[str, int | str] = {
"sentry_app_id": sentry_app_id,
"project_id": project.id,
"organization_slug": organization.slug,
"rule": rule_label,
}
lifecycle.add_extras(extra)

sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id)
if sentry_app is None:
logger.info("event_alert_webhook.missing_sentry_app", extra=extra)
return
sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id)
if sentry_app is None:
raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_SENTRY_APP)

installations = app_service.get_many(
filter=dict(
organization_id=organization.id,
app_ids=[sentry_app.id],
status=SentryAppInstallationStatus.INSTALLED,
installations = app_service.get_many(
filter=dict(
organization_id=organization.id,
app_ids=[sentry_app.id],
status=SentryAppInstallationStatus.INSTALLED,
)
)
)
if not installations:
logger.info("event_alert_webhook.missing_installation", extra=extra)
return
(install,) = installations
if not installations:
raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_INSTALLATION)
(install,) = installations

nodedata = nodestore.backend.get(
BaseEvent.generate_node_id(project_id=project.id, event_id=instance_id)
)
nodedata = nodestore.backend.get(
BaseEvent.generate_node_id(project_id=project.id, event_id=instance_id)
)

if not nodedata:
extra = {
"event_id": instance_id,
"occurrence_id": occurrence_id,
"rule": rule,
"sentry_app": sentry_app.slug,
"group_id": group_id,
}
logger.info("send_alert_event.missing_event", extra=extra)
return
if not nodedata:
raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_EVENT)

occurrence = None
if occurrence_id:
occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id)
occurrence = None
if occurrence_id:
occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id)

if not occurrence:
logger.info(
"send_alert_event.missing_occurrence",
extra={"occurrence_id": occurrence_id, "project_id": project.id},
)
return

group_event = GroupEvent(
project_id=project.id,
event_id=instance_id,
group=group,
data=nodedata,
occurrence=occurrence,
)
if not occurrence:
raise SentryAppSentryError(
message=SentryAppWebhookFailureReason.MISSING_ISSUE_OCCURRENCE
)

event_context = _webhook_event_data(group_event, group.id, project.id)
group_event = GroupEvent(
project_id=project.id,
event_id=instance_id,
group=group,
data=nodedata,
occurrence=occurrence,
)

data = {"event": event_context, "triggered_rule": rule}
event_context = _webhook_event_data(group_event, group.id, project.id)

# Attach extra payload to the webhook
if additional_payload_key and additional_payload:
data[additional_payload_key] = additional_payload
data = {"event": event_context, "triggered_rule": rule_label}

request_data = AppPlatformEvent(
resource="event_alert", action="triggered", install=install, data=data
)
# Attach extra payload to the webhook
if additional_payload_key and additional_payload:
data[additional_payload_key] = additional_payload

request_data = AppPlatformEvent(
resource="event_alert", action="triggered", install=install, data=data
)

send_and_save_webhook_request(sentry_app, request_data)

Expand All @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't need to cast a StrEnum to str

)


Expand Down Expand Up @@ -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})
Copy link
Member

Choose a reason for hiding this comment

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

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

results = safe_execute(
action_inst.after,
event=event,
notification_uuid=notification_uuid,
)

i suppose this means the action is broken anyway, so hopefully we wouldn't migrate it cc @iamrajjoshi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

you could call logger.error which would capture a sentry error and log, but not raise an exception

Copy link
Member

Choose a reason for hiding this comment

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

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

continue

extra_kwargs: dict[str, Any] = {
Expand All @@ -519,7 +513,7 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]):
instance_id=event.event_id,
group_id=event.group_id,
occurrence_id=event.occurrence_id if hasattr(event, "occurrence_id") else None,
rule=f.rule.label,
rule_label=f.rule.label,
sentry_app_id=f.kwargs["sentry_app"].id,
**extra_kwargs,
)
Expand Down
Loading
Loading