Skip to content

fix(sentry apps): Self-heal/regenereate broken ServiceHooks #96712

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix(sentry apps): Self-heal/regenereate broken ServiceHooks #96712

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0b195f
initial commit for backfilling servicehook logic
Christinarlong Jul 29, 2025
d1bbd6d
add task to regenerate broken servicehooks
Christinarlong Jul 29, 2025
950e8f0
nit
Christinarlong Jul 29, 2025
6469652
fix typing
Christinarlong Jul 29, 2025
2dbbc0a
add logger context for installation failure and remove extra checks
Christinarlong Jul 30, 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
54 changes: 43 additions & 11 deletions src/sentry/sentry_apps/tasks/sentry_apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,18 +751,19 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs:
servicehook: ServiceHook | None = _load_service_hook(
installation.organization_id, installation.id
)
lifecycle.add_extra("events", installation.sentry_app.events)
lifecycle.add_extras(
{
"installation_uuid": installation.uuid,
"installation_id": installation.id,
"organization": installation.organization_id,
"sentry_app": installation.sentry_app.id,
"events": installation.sentry_app.events,
"webhook_url": installation.sentry_app.webhook_url or "",
}
)

if not servicehook:
lifecycle.add_extra("events", installation.sentry_app.events)
lifecycle.add_extras(
{
"installation_uuid": installation.uuid,
"installation_id": installation.id,
"organization": installation.organization_id,
"sentry_app": installation.sentry_app.id,
"events": installation.sentry_app.events,
"webhook_url": installation.sentry_app.webhook_url or "",
}
)
raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK)
if event not in servicehook.events:
raise SentryAppSentryError(
Expand Down Expand Up @@ -799,6 +800,37 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs:
)


@instrumented_task(
"sentry.sentry_apps.tasks.sentry_apps.regenerate_service_hook_for_installation",
taskworker_config=TaskworkerConfig(
namespace=sentryapp_control_tasks, retry=Retry(times=3), processing_deadline_duration=60
),
**TASK_OPTIONS,
)
def regenerate_service_hook_for_installation(
installation_id: int, servicehook_events: list[str] | None
) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function would be used whenever we encounter the SentryAppWebhookFailureReason.MISSING_SERVICEHOOK or SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK failure cases.

In both cases we usually should have an installation with a working ServiceHook a la we use the Sentry app's event list as the source of truth for all installations

This logic was put into another task because I didn't want another logically separate operation to potentially pollute the results/info of another task. And similarly allows us to get better data on when this operation is ran.

installation = app_service.installation_by_id(id=installation_id)
app_events = installation.sentry_app.events

if servicehook_events is None or set(servicehook_events) != set(app_events):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking about this more idk if we should even have these checks, like I think we can assume the caller will have done the validation/check that the ServiceHook is in a bad state

Copy link
Member

Choose a reason for hiding this comment

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

i agree with your intuition: i think this task should only be called if this if statement is true.

create_or_update_service_hooks_for_installation(
installation=installation,
events=app_events,
webhook_url=installation.sentry_app.webhook_url,
)

logger.info(
"regenerate_service_hook_for_installation",
extra={
"installation_id": installation_id,
"servicehook_events": servicehook_events,
"app_events": app_events,
"sentry_app": installation.sentry_app.id,
},
)


@instrumented_task(
"sentry.sentry_apps.tasks.sentry_apps.create_or_update_service_hooks_for_sentry_app",
taskworker_config=TaskworkerConfig(
Expand Down
63 changes: 62 additions & 1 deletion tests/sentry/sentry_apps/tasks/test_sentry_apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
installation_webhook,
notify_sentry_app,
process_resource_change_bound,
regenerate_service_hook_for_installation,
send_alert_webhook_v2,
send_webhooks,
workflow_notification,
)
from sentry.sentry_apps.utils.errors import SentryAppSentryError
from sentry.shared_integrations.exceptions import ClientError
from sentry.silo.base import SiloMode
from sentry.tasks.post_process import post_process_group
from sentry.testutils.asserts import (
assert_count_of_metric,
Expand All @@ -44,7 +46,7 @@
from sentry.testutils.helpers import with_feature
from sentry.testutils.helpers.datetime import before_now
from sentry.testutils.helpers.eventprocessing import write_event_to_cache
from sentry.testutils.silo import assume_test_silo_mode_of, control_silo_test
from sentry.testutils.silo import assume_test_silo_mode, assume_test_silo_mode_of, control_silo_test
from sentry.testutils.skips import requires_snuba
from sentry.types.activity import ActivityType
from sentry.types.rules import RuleFuture
Expand Down Expand Up @@ -1659,3 +1661,62 @@ def test_saves_error_event_id_if_in_header(self, safe_urlopen):
assert first_request["organization_id"] == self.install.organization_id
assert first_request["error_id"] == "d5111da2c28645c5889d072017e3445d"
assert first_request["project_id"] == "1"


class TestBackfillServiceHooksEvents(TestCase):
def setUp(self) -> None:
self.sentry_app = self.create_sentry_app(
name="Test App",
webhook_url="https://example.com",
organization=self.organization,
events=["issue.created", "issue.resolved", "error.created"],
)
self.install = self.create_sentry_app_installation(
organization=self.organization, slug=self.sentry_app.slug
)

def test_regenerate_service_hook_for_installation_success(self):
with assume_test_silo_mode(SiloMode.REGION):
hook = ServiceHook.objects.get(installation_id=self.install.id)
hook.events = ["issue.resolved", "error.created"]
hook.save()

servicehook_events = ["issue.created"]
with self.tasks(), assume_test_silo_mode(SiloMode.REGION):
regenerate_service_hook_for_installation(
installation_id=self.install.id, servicehook_events=servicehook_events
)

with assume_test_silo_mode(SiloMode.REGION):
hook.refresh_from_db()
assert set(hook.events) == {"issue.created", "issue.resolved", "error.created"}

def test_regenerate_service_hook_for_installation_event_not_in_app_events(self):
servicehook_events = ["comment.created"]
with self.tasks(), assume_test_silo_mode(SiloMode.REGION):
regenerate_service_hook_for_installation(
installation_id=self.install.id, servicehook_events=servicehook_events
)

with assume_test_silo_mode(SiloMode.REGION):
hook = ServiceHook.objects.get(installation_id=self.install.id)
assert set(hook.events) == {"issue.created", "issue.resolved", "error.created"}

def test_regenerate_service_hook_for_installation_with_empty_app_events(self):
with assume_test_silo_mode(SiloMode.CONTROL):
self.sentry_app.update(events=[])
assert self.sentry_app.events == []

with assume_test_silo_mode(SiloMode.REGION):
hook = ServiceHook.objects.get(installation_id=self.install.id)
assert hook.events != []

servicehook_events = ["comment.created"]
with self.tasks(), assume_test_silo_mode(SiloMode.REGION):
regenerate_service_hook_for_installation(
installation_id=self.install.id, servicehook_events=servicehook_events
)

with assume_test_silo_mode(SiloMode.REGION):
hook.refresh_from_db()
assert hook.events == []
Loading