Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

from sentry import features
from sentry.notifications.notification_action.utils import execute_via_group_type_registry
from sentry.sentry_apps.services.legacy_webhook.service import send_legacy_webhooks_for_invocation
from sentry.sentry_apps.services.legacy_webhook.service import (
send_legacy_webhooks_for_invocation,
send_sentry_app_webhook_for_invocation,
)
from sentry.services.eventstore.models import GroupEvent
from sentry.workflow_engine.models import Action
from sentry.workflow_engine.registry import action_handler_registry
Expand Down Expand Up @@ -56,4 +59,8 @@ def execute(invocation: ActionInvocation) -> None:
)

if new_path and isinstance(invocation.event_data.event, GroupEvent):
send_legacy_webhooks_for_invocation(invocation)
target_identifier = invocation.action.config.get("target_identifier")
if target_identifier == "webhooks":
Comment thread
GabeVillalobos marked this conversation as resolved.
send_legacy_webhooks_for_invocation(invocation)
else:
send_sentry_app_webhook_for_invocation(invocation)
29 changes: 29 additions & 0 deletions src/sentry/sentry_apps/services/legacy_webhook/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
from sentry.eventstore.models import GroupEvent
from sentry.models.options.project_option import ProjectOption
from sentry.models.rule import Rule
from sentry.sentry_apps.services.app import app_service
from sentry.sentry_apps.tasks.sentry_apps import send_alert_webhook_v2
from sentry.workflow_engine.models import AlertRuleWorkflow, Workflow
from sentry.workflow_engine.types import ActionInvocation

Expand Down Expand Up @@ -83,6 +85,33 @@ def build_legacy_webhook_payload(invocation: ActionInvocation) -> LegacyWebhookP
return data


def send_sentry_app_webhook_for_invocation(invocation: ActionInvocation) -> None:
event = invocation.event_data.event
assert isinstance(event, GroupEvent)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this assertion exist in the old code? If not, do we have a strong guarantee that this will always be true? Also would help to add a message in the assertion of why we expect this to be the case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes this would be guarded in the old code by the same NOA utils. We also have the guard in the action code but for typing (since we're giving ActionInvocation) we need to add the check

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it. You may want to consider having the webhook handler execute method break down the ActionInvocation (or have it call into a helper/transformer), that gives you all the properly typed data you need instead of doing multiple instance checks throughout the code.

sentry_app_slug = invocation.action.config.get("target_identifier")
if not sentry_app_slug:
logger.warning("webhook_action_handler.missing_target_identifier")
return

sentry_app = app_service.get_sentry_app_by_slug(slug=sentry_app_slug)
if sentry_app is None:
logger.warning(
"webhook_action_handler.sentry_app_not_found",
extra={"sentry_app_slug": sentry_app_slug},
)
return

rule_label = _get_triggering_rule_name(invocation)

send_alert_webhook_v2.delay(
rule_label=rule_label,
sentry_app_id=sentry_app.id,
instance_id=event.event_id,
group_id=event.group_id,
occurrence_id=getattr(event, "occurrence_id", None),
)


def send_legacy_webhooks_for_invocation(invocation: ActionInvocation) -> None:
# Delayed import to avoid circular dependency (tasks imports LegacyWebhookPayload from here)
from sentry.sentry_apps.services.legacy_webhook.tasks import send_legacy_webhook_task
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,62 @@ def test_non_group_event_skips_new_path_but_old_path_still_runs(
mock_new_path.assert_not_called()
mock_old_path.assert_called_once_with(invocation)

@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_sentry_app_webhook_for_invocation"
)
@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_legacy_webhooks_for_invocation"
)
def test_new_path_sentry_app_action_routes_to_sentry_app_webhook(
self, mock_legacy: mock.MagicMock, mock_sentry_app: mock.MagicMock
) -> None:
action = self.create_action(
type=Action.Type.WEBHOOK,
config={"target_identifier": "my-app"},
)
invocation = ActionInvocation(
event_data=self.event_data,
action=action,
detector=self.detector,
notification_uuid=str(uuid.uuid4()),
workflow_id=self.workflow.id,
)

with (
self.feature(
{
"organizations:legacy-webhook-new-path": True,
"organizations:legacy-webhook-disable-old-path": True,
}
),
):
WebhookActionHandler.execute(invocation)

mock_sentry_app.assert_called_once_with(invocation)
mock_legacy.assert_not_called()

@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_sentry_app_webhook_for_invocation"
)
@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_legacy_webhooks_for_invocation"
)
def test_new_path_webhooks_action_routes_to_legacy_webhook(
self, mock_legacy: mock.MagicMock, mock_sentry_app: mock.MagicMock
) -> None:
with (
self.feature(
{
"organizations:legacy-webhook-new-path": True,
"organizations:legacy-webhook-disable-old-path": True,
}
),
):
WebhookActionHandler.execute(self.invocation)

mock_legacy.assert_called_once_with(self.invocation)
mock_sentry_app.assert_not_called()

@responses.activate
@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.execute_via_group_type_registry",
Expand Down
100 changes: 100 additions & 0 deletions tests/sentry/sentry_apps/services/legacy_webhook/test_service.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
import uuid
from unittest import mock
from unittest.mock import MagicMock

from django.urls import reverse

from sentry.models.options.project_option import ProjectOption
from sentry.sentry_apps.services.legacy_webhook.service import (
build_legacy_webhook_payload,
send_legacy_webhooks_for_invocation,
send_sentry_app_webhook_for_invocation,
split_urls,
)
from sentry.testutils.cases import TestCase
from sentry.testutils.skips import requires_snuba
from sentry.utils import json
from sentry.utils.http import absolute_uri
from sentry.workflow_engine.models import Action
from sentry.workflow_engine.types import ActionInvocation, WorkflowEventData
from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
Expand Down Expand Up @@ -133,3 +139,97 @@ def test_triggering_rules_prefers_legacy_rule_label(self, mock_task: mock.MagicM

payload = mock_task.delay.call_args.kwargs["payload"]
assert payload["triggering_rules"] == ["My Custom Rule Name"]


class TestSendSentryAppWebhookForInvocation(BaseWorkflowTest):
def setUp(self) -> None:
super().setUp()
self.sentry_app = self.create_sentry_app(
organization=self.organization,
name="My Test App",
is_alertable=True,
)
self.detector = self.create_detector(project=self.project)
self.workflow = self.create_workflow(environment=self.environment)
self.action = self.create_action(
type=Action.Type.WEBHOOK,
config={"target_identifier": self.sentry_app.slug},
)
self.group, self.event, self.group_event = self.create_group_event()
self.event_data = WorkflowEventData(
event=self.group_event, workflow_env=self.environment, group=self.group
)
self.invocation = ActionInvocation(
event_data=self.event_data,
action=self.action,
detector=self.detector,
notification_uuid=str(uuid.uuid4()),
workflow_id=self.workflow.id,
)

@mock.patch("sentry.utils.sentry_apps.webhooks.safe_urlopen")
def test_sends_signed_webhook_to_sentry_app(self, safe_urlopen: MagicMock) -> None:
safe_urlopen.return_value = mock.MagicMock(
ok=True,
status_code=200,
headers={},
content=b"{}",
text="",
raise_for_status=lambda: None,
)
self.create_sentry_app_installation(
organization=self.organization, slug=self.sentry_app.slug
)

with self.tasks():
send_sentry_app_webhook_for_invocation(self.invocation)

assert safe_urlopen.called
((_, kwargs),) = safe_urlopen.call_args_list
data = json.loads(kwargs["data"])

assert data["action"] == "triggered"
assert "installation" in data
assert data["data"]["triggered_rule"] == self.workflow.name
assert data["data"]["event"]["event_id"] == self.group_event.event_id
assert data["data"]["event"]["project"] == self.project.id
assert data["data"]["event"]["issue_id"] == str(self.group.id)
assert data["data"]["event"]["url"] == absolute_uri(
reverse(
"sentry-api-0-project-event-details",
args=[self.organization.slug, self.project.slug, self.group_event.event_id],
)
)

assert kwargs["headers"].keys() >= {
"Content-Type",
"Request-ID",
"Sentry-Hook-Resource",
"Sentry-Hook-Timestamp",
"Sentry-Hook-Signature",
}

@mock.patch(
"sentry.sentry_apps.tasks.sentry_apps.send_alert_webhook_v2",
)
def test_missing_app_logs_warning(self, mock_task: mock.MagicMock) -> None:
action = self.create_action(
type=Action.Type.WEBHOOK,
config={"target_identifier": "nonexistent-app"},
)
invocation = ActionInvocation(
event_data=self.event_data,
action=action,
detector=self.detector,
notification_uuid=str(uuid.uuid4()),
workflow_id=self.workflow.id,
)

with mock.patch("sentry.sentry_apps.services.legacy_webhook.service.logger") as mock_logger:
send_sentry_app_webhook_for_invocation(invocation)

mock_logger.warning.assert_called_once_with(
"webhook_action_handler.sentry_app_not_found",
extra={"sentry_app_slug": "nonexistent-app"},
)
mock_task.delay.assert_not_called()
Loading