Skip to content
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

✨ feat(aci): use Action & Group for Slack Issue Alert Threads #84821

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
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
199 changes: 190 additions & 9 deletions src/sentry/integrations/slack/actions/notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,19 @@
MessagingInteractionType,
)
from sentry.integrations.models.integration import Integration
from sentry.integrations.repository import get_default_issue_alert_repository
from sentry.integrations.repository import (
get_default_issue_alert_repository,
get_default_notification_action_repository,
)
from sentry.integrations.repository.base import NotificationMessageValidationError
from sentry.integrations.repository.issue_alert import (
IssueAlertNotificationMessageRepository,
NewIssueAlertNotificationMessage,
)
from sentry.integrations.repository.notification_action import (
NewNotificationActionNotificationMessage,
NotificationActionNotificationMessageRepository,
)
from sentry.integrations.services.integration import RpcIntegration
from sentry.integrations.slack.actions.form import SlackNotifyServiceForm
from sentry.integrations.slack.message_builder.issues import SlackIssuesMessageBuilder
Expand All @@ -47,6 +54,7 @@
from sentry.rules.base import CallbackFuture
from sentry.types.rules import RuleFuture
from sentry.utils import metrics
from sentry.workflow_engine.models.action import Action

_default_logger: Logger = getLogger(__name__)

Expand All @@ -71,9 +79,12 @@ def __init__(self, *args: Any, **kwargs: Any) -> None:
"notes": {"type": "string", "placeholder": "e.g., @jane, @on-call-team"},
}

self._repository: IssueAlertNotificationMessageRepository = (
self._issue_repository: IssueAlertNotificationMessageRepository = (
get_default_issue_alert_repository()
)
self._action_repository: NotificationActionNotificationMessageRepository = (
get_default_notification_action_repository()
)

def after(
self, event: GroupEvent, notification_uuid: str | None = None
Expand Down Expand Up @@ -158,11 +169,13 @@ def get_thread_ts(lifecycle: EventLifecycle) -> str | None:
return None

try:
parent_notification_message = self._repository.get_parent_notification_message(
rule_id=rule_id,
group_id=event.group.id,
rule_action_uuid=rule_action_uuid,
open_period_start=open_period_start,
parent_notification_message = (
self._issue_repository.get_parent_notification_message(
rule_id=rule_id,
group_id=event.group.id,
rule_action_uuid=rule_action_uuid,
open_period_start=open_period_start,
)
)
except Exception as e:
lifecycle.record_halt(e)
Expand Down Expand Up @@ -236,7 +249,170 @@ def get_thread_ts(lifecycle: EventLifecycle) -> str | None:

if rule_action_uuid and rule_id:
try:
self._repository.create_notification_message(
self._issue_repository.create_notification_message(
data=new_notification_message_object
)
except NotificationMessageValidationError as err:
extra = (
new_notification_message_object.__dict__
if new_notification_message_object
else None
)
_default_logger.info(
"Validation error for new notification message", exc_info=err, extra=extra
)
except Exception:
# if there's an error trying to save a notification message, don't let that error block this flow
# we already log at the repository layer, no need to log again here
pass

self.record_notification_sent(event, channel, rule, notification_uuid)

def send_notification_noa(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

this function is extremely long. is there any way to break this up a bit, like the shared logic between this and send_notification? there are also nested functions in here i'm not sure are totally necessary but i get that it was the previous implementation

Copy link
Member Author

Choose a reason for hiding this comment

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

i can pull this out, but i didn't want to reuse code b/c it makes feature flagging annoying since we have nested if else indentation deep into the function which makes readability poor.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually i am not sure if i can pull it out either (need to test), since we are returning this function itself and the signature can't change. it would have to be a pure function outside of the class in that case but it also needs access to variables that exist in the outer function 🤔

Copy link
Member

Choose a reason for hiding this comment

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

another day, another punt to notification platform :D

rules = [f.rule for f in futures]
additional_attachment = get_additional_attachment(
integration, self.project.organization
)
blocks = SlackIssuesMessageBuilder(
group=event.group,
event=event,
tags=tags,
rules=rules,
notes=self.get_option("notes", ""),
).build(notification_uuid=notification_uuid)

if additional_attachment:
for block in additional_attachment:
blocks["blocks"].append(block)

if payload_blocks := blocks.get("blocks"):
json_blocks = orjson.dumps(payload_blocks).decode()

rule = rules[0] if rules else None
rule_to_use = self.rule if self.rule else rule
# In the NOA, we will store the action id in the rule id field
action_id = rule_to_use.id if rule_to_use else None

if not action_id:
# We are logging because this should never happen, all actions should have an uuid
# We can monitor for this, and create an alert if this ever appears
_default_logger.info(
"No action id found in the rule future",
)
return

try:
action = Action.objects.get(id=action_id)
except Action.DoesNotExist:
_default_logger.info(
"Action not found",
extra={
"action_id": action_id,
},
)
return

new_notification_message_object = NewNotificationActionNotificationMessage(
action_id=action_id,
group_id=event.group.id,
)

open_period_start: datetime | None = None
if (
event.group.issue_category == GroupCategory.UPTIME
or event.group.issue_category == GroupCategory.METRIC_ALERT
):
open_period_start = open_period_start_for_group(event.group)
new_notification_message_object.open_period_start = open_period_start

def get_thread_ts(lifecycle: EventLifecycle) -> str | None:
"""Find the thread in which to post this notification as a reply.
Return None to post the notification as a top-level message.
"""

if not (
OrganizationOption.objects.get_value(
organization=self.project.organization,
key="sentry:issue_alerts_thread_flag",
default=ISSUE_ALERTS_THREAD_DEFAULT,
)
):
return None

try:
parent_notification_message = (
self._action_repository.get_parent_notification_message(
action=action,
group=event.group,
open_period_start=open_period_start,
)
)
except Exception as e:
lifecycle.record_halt(e)
return None

if parent_notification_message is None:
return None

new_notification_message_object.parent_notification_message_id = (
parent_notification_message.id
)
return parent_notification_message.message_identifier

with MessagingInteractionEvent(
MessagingInteractionType.GET_PARENT_NOTIFICATION, SlackMessagingSpec()
).capture() as lifecycle:
thread_ts = get_thread_ts(lifecycle)

# If this flow is triggered again for the same issue, we want it to be seen in the main channel
reply_broadcast = thread_ts is not None

client = SlackSdkClient(integration_id=integration.id)
text = str(blocks.get("text"))
# Wrap the Slack API call with lifecycle tracking
with MessagingInteractionEvent(
interaction_type=MessagingInteractionType.SEND_ISSUE_ALERT_NOTIFICATION,
spec=SlackMessagingSpec(),
).capture() as lifecycle:
try:
response = client.chat_postMessage(
blocks=json_blocks,
text=text,
channel=channel,
unfurl_links=False,
unfurl_media=False,
thread_ts=thread_ts,
reply_broadcast=reply_broadcast,
)
except SlackApiError as e:
# Record the error code and details from the exception
new_notification_message_object.error_code = e.response.status_code
new_notification_message_object.error_details = {
"msg": str(e),
"data": e.response.data,
"url": e.response.api_url,
}

log_params: dict[str, str | int] = {
"error": str(e),
"project_id": event.project_id,
"event_id": event.event_id,
"payload": json_blocks,
}
if self.get_option("channel"):
log_params["channel_name"] = self.get_option("channel")

lifecycle.add_extras(log_params)
record_lifecycle_termination_level(lifecycle, e)
else:
ts = response.get("ts")
new_notification_message_object.message_identifier = (
str(ts) if ts is not None else None
)

try:
self._action_repository.create_notification_message(
data=new_notification_message_object
)
except NotificationMessageValidationError as err:
Expand All @@ -258,7 +434,12 @@ def get_thread_ts(lifecycle: EventLifecycle) -> str | None:
key = f"slack:{integration.id}:{channel}"

metrics.incr("notifications.sent", instance="slack.notification", skip_internal=False)
yield self.future(send_notification, key=key)
if features.has(
"organizations:workflow-engine-notification-action", self.project.organization
):
yield self.future(send_notification_noa, key=key)
else:
yield self.future(send_notification, key=key)

def send_confirmation_notification(
self, rule: Rule, new: bool, changed: dict[str, Any] | None = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from sentry.silo.base import SiloMode
from sentry.testutils.asserts import assert_failure_metric
from sentry.testutils.cases import RuleTestCase
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import assume_test_silo_mode
from sentry.types.rules import RuleFuture

Expand Down Expand Up @@ -62,6 +63,8 @@ def setUp(self) -> None:
notification_uuid=self.notification_uuid,
)

self.action = self.create_action(target_identifier="C0123456789")

def test_when_rule_fire_history_is_passed_in(self) -> None:
instance = SlackNotifyServiceAction(
self.project, data={}, rule=self.rule, rule_fire_history=self.rule_fire_history
Expand Down Expand Up @@ -298,3 +301,132 @@ def test_send_confirmation_using_sdk_error(self, mock_metrics):
mock_metrics.incr.assert_called_with(
SLACK_DATADOG_METRIC, sample_rate=1.0, tags={"ok": False, "status": 200}
)

@with_feature("organizations:workflow-engine-notification-action")
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
@patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
@patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
def test_after_noa(self, mock_api_call, mock_post, mock_record):
mock_api_call.return_value = {
"body": orjson.dumps({"ok": True}).decode(),
"headers": {},
"status": 200,
}

# Create a rule with a numeric ID for the test
rule = self.get_rule(data=self.action_data)
rule.id = self.action.id

results = list(rule.after(event=self.event))
assert len(results) == 1

results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})])
blocks = mock_post.call_args.kwargs["blocks"]
blocks = orjson.loads(blocks)

assert (
blocks[0]["text"]["text"]
== f":large_yellow_circle: <http://testserver/organizations/{self.organization.slug}/issues/{self.event.group.id}/?referrer=slack&alert_rule_id={rule.id}&alert_type=issue|*Hello world*>"
)

assert NotificationMessage.objects.all().count() == 1

assert len(mock_record.mock_calls) == 4
thread_ts_start, thread_ts_success, send_notification_start, send_notification_success = (
mock_record.mock_calls
)
assert thread_ts_start.args[0] == EventLifecycleOutcome.STARTED
assert thread_ts_success.args[0] == EventLifecycleOutcome.SUCCESS
assert send_notification_start.args[0] == EventLifecycleOutcome.STARTED
assert send_notification_success.args[0] == EventLifecycleOutcome.SUCCESS

@with_feature("organizations:workflow-engine-notification-action")
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
@patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
@patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
def test_after_with_threads_noa(self, mock_api_call, mock_post, mock_record):
mock_api_call.return_value = {
"body": orjson.dumps({"ok": True}).decode(),
"headers": {},
"status": 200,
}

rule = self.get_rule(data=self.action_data)
rule.id = self.action.id
results = list(rule.after(event=self.event))
assert len(results) == 1

results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})])
blocks = mock_post.call_args.kwargs["blocks"]
blocks = orjson.loads(blocks)

assert (
blocks[0]["text"]["text"]
== f":large_yellow_circle: <http://testserver/organizations/{self.organization.slug}/issues/{self.event.group.id}/?referrer=slack&alert_rule_id={self.action.id}&alert_type=issue|*Hello world*>"
)

assert NotificationMessage.objects.all().count() == 1

assert len(mock_record.mock_calls) == 4
thread_ts_start, thread_ts_success, send_notification_start, send_notification_success = (
mock_record.mock_calls
)
assert thread_ts_start.args[0] == EventLifecycleOutcome.STARTED
assert thread_ts_success.args[0] == EventLifecycleOutcome.SUCCESS
assert send_notification_start.args[0] == EventLifecycleOutcome.STARTED
assert send_notification_success.args[0] == EventLifecycleOutcome.SUCCESS

@with_feature("organizations:workflow-engine-notification-action")
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
@patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
@patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
def test_after_reply_in_thread_noa(self, mock_api_call, mock_post, mock_record):
mock_api_call.return_value = {
"body": orjson.dumps({"ok": True}).decode(),
"headers": {},
"status": 200,
}

with assume_test_silo_mode(SiloMode.REGION):
msg = NotificationMessage.objects.create(
action_id=self.action.id,
group_id=self.event.group.id,
)

event = self.store_event(
data={
"message": "Hello world",
"level": "warning",
"platform": "python",
"culprit": "foo.bar",
},
project_id=self.project.id,
)

rule = self.get_rule(data=self.action_data)
rule.id = self.action.id
results = list(rule.after(event=event))
assert len(results) == 1

results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})])
blocks = mock_post.call_args.kwargs["blocks"]
blocks = orjson.loads(blocks)

assert (
blocks[0]["text"]["text"]
== f":large_yellow_circle: <http://testserver/organizations/{self.organization.slug}/issues/{self.event.group.id}/?referrer=slack&alert_rule_id={self.action.id}&alert_type=issue|*Hello world*>"
)

assert NotificationMessage.objects.all().count() == 2
assert (
NotificationMessage.objects.filter(parent_notification_message_id=msg.id).count() == 1
)

assert len(mock_record.mock_calls) == 4
thread_ts_start, thread_ts_success, send_notification_start, send_notification_success = (
mock_record.mock_calls
)
assert thread_ts_start.args[0] == EventLifecycleOutcome.STARTED
assert thread_ts_success.args[0] == EventLifecycleOutcome.SUCCESS
assert send_notification_start.args[0] == EventLifecycleOutcome.STARTED
assert send_notification_success.args[0] == EventLifecycleOutcome.SUCCESS
Loading