From 744c33608c5f0d4d416e3c3422f216d95d905592 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 9 May 2025 13:53:59 -0400 Subject: [PATCH 1/2] fix(taskworker) Remove pickle usage from sentryapps task The `event` parameter of process_service_hook requires pickle. By generating the payload before spawning a task, we don't have to use pickle. Refs SENTRY-3V47 --- src/sentry/sentry_apps/tasks/service_hooks.py | 27 ++++++++++++------- src/sentry/tasks/post_process.py | 19 ++++++++----- tests/sentry/tasks/test_post_process.py | 25 +++++++++++++++-- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/service_hooks.py b/src/sentry/sentry_apps/tasks/service_hooks.py index c17d53e30776c5..4ff374ce22ef38 100644 --- a/src/sentry/sentry_apps/tasks/service_hooks.py +++ b/src/sentry/sentry_apps/tasks/service_hooks.py @@ -1,5 +1,6 @@ import logging from time import time +from typing import Any from sentry.api.serializers import serialize from sentry.http import safe_urlopen @@ -40,16 +41,24 @@ def get_payload_v0(event): taskworker_config=TaskworkerConfig(namespace=sentryapp_tasks, retry=Retry(times=3)), ) @retry -def process_service_hook(servicehook_id, event, **kwargs): +def process_service_hook( + servicehook_id: int, + event: Any, + payload: str | None = None, + project_id: int | None = None, + **kwargs, +): try: servicehook = ServiceHook.objects.get(id=servicehook_id) except ServiceHook.DoesNotExist: return - if servicehook.version == 0: - payload = get_payload_v0(event) - else: - raise NotImplementedError + project_id = project_id or event.project_id + if not payload: + if servicehook.version == 0: + payload = json.dumps(get_payload_v0(event)) + else: + raise NotImplementedError from sentry import tsdb @@ -59,10 +68,8 @@ def process_service_hook(servicehook_id, event, **kwargs): "Content-Type": "application/json", "X-ServiceHook-Timestamp": str(int(time())), "X-ServiceHook-GUID": servicehook.guid, - "X-ServiceHook-Signature": servicehook.build_signature(json.dumps(payload)), + "X-ServiceHook-Signature": servicehook.build_signature(payload), } - safe_urlopen( - url=servicehook.url, data=json.dumps(payload), headers=headers, timeout=5, verify_ssl=False - ) - logger.info("service_hook.success", extra={"project_id": event.project_id}) + safe_urlopen(url=servicehook.url, data=payload, headers=headers, timeout=5, verify_ssl=False) + logger.info("service_hook.success", extra={"project_id": project_id}) diff --git a/src/sentry/tasks/post_process.py b/src/sentry/tasks/post_process.py index 78c3cd6839ad5f..2b191734f9e356 100644 --- a/src/sentry/tasks/post_process.py +++ b/src/sentry/tasks/post_process.py @@ -72,7 +72,7 @@ class PostProcessJob(TypedDict, total=False): has_escalated: bool -def _get_service_hooks(project_id): +def _get_service_hooks(project_id: int) -> list[tuple[int, list[str]]]: from sentry.sentry_apps.models.servicehook import ServiceHook cache_key = f"servicehooks:1:{project_id}" @@ -1180,7 +1180,7 @@ def process_service_hooks(job: PostProcessJob) -> None: if job["is_reprocessed"]: return - from sentry.sentry_apps.tasks.service_hooks import process_service_hook + from sentry.sentry_apps.tasks.service_hooks import get_payload_v0, process_service_hook event, has_alert = job["event"], job["has_alert"] @@ -1189,10 +1189,17 @@ def process_service_hooks(job: PostProcessJob) -> None: if has_alert: allowed_events.add("event.alert") - if allowed_events: - for servicehook_id, events in _get_service_hooks(project_id=event.project_id): - if any(e in allowed_events for e in events): - process_service_hook.delay(servicehook_id=servicehook_id, event=event) + # TODO: when we have multiple service hook versions, this will need to change. + payload = json.dumps(get_payload_v0(event)) + + for servicehook_id, events in _get_service_hooks(project_id=event.project_id): + if any(e in allowed_events for e in events): + process_service_hook.delay( + servicehook_id=servicehook_id, + project_id=event.project_id, + event=event, + payload=payload, + ) def process_resource_change_bounds(job: PostProcessJob) -> None: diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index abcd78130263af..c16531460b3648 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -99,6 +99,21 @@ def __eq__(self, other): return matching_id +class ServiceHookPayloadMatcher: + def __init__(self, event: Event): + self.event = event + + def __eq__(self, other: str) -> bool: + assert isinstance(other, str), "other should be a string" + payload = json.loads(other) + assert payload["event"]["id"] == self.event.event_id + assert payload["group"]["id"] == str(self.event.group.id) + assert "url" in payload["group"] + assert "tags" in payload["event"] + assert "project" in payload + return True + + class BasePostProgressGroupMixin(BaseTestCase, metaclass=abc.ABCMeta): @abc.abstractmethod def create_event(self, data, project_id, assert_no_errors=True): @@ -518,7 +533,10 @@ def test_service_hook_fires_on_new_event(self, mock_process_service_hook): ) mock_process_service_hook.delay.assert_called_once_with( - servicehook_id=hook.id, event=EventMatcher(event) + servicehook_id=hook.id, + project_id=self.project.id, + event=EventMatcher(event), + payload=ServiceHookPayloadMatcher(event), ) @patch("sentry.sentry_apps.tasks.service_hooks.process_service_hook") @@ -547,7 +565,10 @@ def test_service_hook_fires_on_alert(self, mock_processor, mock_process_service_ ) mock_process_service_hook.delay.assert_called_once_with( - servicehook_id=hook.id, event=EventMatcher(event) + servicehook_id=hook.id, + project_id=self.project.id, + event=EventMatcher(event), + payload=ServiceHookPayloadMatcher(event), ) @patch("sentry.sentry_apps.tasks.service_hooks.process_service_hook") From fec2311f74c6a8e72db1df5a389c4ea765f160d5 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 9 May 2025 15:29:29 -0400 Subject: [PATCH 2/2] mypy --- tests/sentry/tasks/test_post_process.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/sentry/tasks/test_post_process.py b/tests/sentry/tasks/test_post_process.py index c16531460b3648..59432b0dd6105c 100644 --- a/tests/sentry/tasks/test_post_process.py +++ b/tests/sentry/tasks/test_post_process.py @@ -103,8 +103,10 @@ class ServiceHookPayloadMatcher: def __init__(self, event: Event): self.event = event - def __eq__(self, other: str) -> bool: + def __eq__(self, other: Any) -> bool: assert isinstance(other, str), "other should be a string" + assert self.event.group + payload = json.loads(other) assert payload["event"]["id"] == self.event.event_id assert payload["group"]["id"] == str(self.event.group.id)