From 187f6a609c955d64309d1b393d5544aa8a4f9370 Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 22 Apr 2025 11:08:23 -0400 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=20Setup=20new=20group?= =?UTF-8?q?=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/sentry/issues/grouptype.py | 10 ++++++++++ src/sentry/notifications/utils/__init__.py | 8 ++++++-- .../utils/performance_issues/detectors/__init__.py | 14 -------------- .../detectors/experiments/__init__.py | 0 .../detectors/n_plus_one_api_calls_detector.py | 9 ++++----- .../performance_issues/performance_detection.py | 4 ++++ tests/sentry/utils/performance_issues/__init__.py | 0 .../performance_issues/experiments/__init__.py | 0 8 files changed, 24 insertions(+), 21 deletions(-) create mode 100644 src/sentry/utils/performance_issues/detectors/experiments/__init__.py create mode 100644 tests/sentry/utils/performance_issues/__init__.py create mode 100644 tests/sentry/utils/performance_issues/experiments/__init__.py diff --git a/src/sentry/issues/grouptype.py b/src/sentry/issues/grouptype.py index 1222bb06dbdf51..c389e7ae858067 100644 --- a/src/sentry/issues/grouptype.py +++ b/src/sentry/issues/grouptype.py @@ -339,6 +339,16 @@ class PerformanceNPlusOneAPICallsGroupType(GroupType): released = True +@dataclass(frozen=True) +class PerformanceNPlusOneAPICallsExperimentalGroupType(GroupType): + type_id = 1910 + slug = "performance_n_plus_one_api_calls_experimental" + description = "N+1 API Call (Experimental)" + category = GroupCategory.PERFORMANCE.value + default_priority = PriorityLevel.LOW + released = False + + @dataclass(frozen=True) class PerformanceMNPlusOneDBQueriesGroupType(PerformanceGroupTypeDefaults, GroupType): type_id = 1011 diff --git a/src/sentry/notifications/utils/__init__.py b/src/sentry/notifications/utils/__init__.py index c4de152a58b219..4512418b3a774c 100644 --- a/src/sentry/notifications/utils/__init__.py +++ b/src/sentry/notifications/utils/__init__.py @@ -21,6 +21,7 @@ from sentry.integrations.services.integration import integration_service from sentry.issues.grouptype import ( PerformanceConsecutiveDBQueriesGroupType, + PerformanceNPlusOneAPICallsExperimentalGroupType, PerformanceNPlusOneAPICallsGroupType, PerformanceRenderBlockingAssetSpanGroupType, ) @@ -336,7 +337,7 @@ def occurrence_perf_to_email_html(context: Any) -> str: def get_spans( - entries: list[dict[str, list[dict[str, str | float]] | str]] + entries: list[dict[str, list[dict[str, str | float]] | str]], ) -> list[dict[str, str | float]] | None: """Get the given event's spans""" if not len(entries): @@ -496,7 +497,10 @@ def from_problem_and_spans( spans: list[dict[str, str | float]] | None, event: Event | None = None, ) -> PerformanceProblemContext: - if problem.type == PerformanceNPlusOneAPICallsGroupType: + if problem.type in ( + PerformanceNPlusOneAPICallsGroupType, + PerformanceNPlusOneAPICallsExperimentalGroupType, + ): return NPlusOneAPICallProblemContext(problem, spans, event) if problem.type == PerformanceConsecutiveDBQueriesGroupType: return ConsecutiveDBQueriesProblemContext(problem, spans, event) diff --git a/src/sentry/utils/performance_issues/detectors/__init__.py b/src/sentry/utils/performance_issues/detectors/__init__.py index 393b859d50643f..e69de29bb2d1d6 100644 --- a/src/sentry/utils/performance_issues/detectors/__init__.py +++ b/src/sentry/utils/performance_issues/detectors/__init__.py @@ -1,14 +0,0 @@ -from .consecutive_db_detector import ConsecutiveDBSpanDetector # NOQA -from .consecutive_http_detector import ConsecutiveHTTPSpanDetector # NOQA -from .http_overhead_detector import HTTPOverheadDetector # NOQA -from .io_main_thread_detector import DBMainThreadDetector, FileIOMainThreadDetector # NOQA -from .large_payload_detector import LargeHTTPPayloadDetector # NOQA -from .mn_plus_one_db_span_detector import MNPlusOneDBSpanDetector # NOQA -from .n_plus_one_api_calls_detector import NPlusOneAPICallsDetector # NOQA -from .n_plus_one_db_span_detector import ( # NOQA - NPlusOneDBSpanDetector, - NPlusOneDBSpanDetectorExtended, -) -from .render_blocking_asset_span_detector import RenderBlockingAssetSpanDetector # NOQA -from .slow_db_query_detector import SlowDBQueryDetector # NOQA -from .uncompressed_asset_detector import UncompressedAssetSpanDetector # NOQA diff --git a/src/sentry/utils/performance_issues/detectors/experiments/__init__.py b/src/sentry/utils/performance_issues/detectors/experiments/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/src/sentry/utils/performance_issues/detectors/n_plus_one_api_calls_detector.py b/src/sentry/utils/performance_issues/detectors/n_plus_one_api_calls_detector.py index c6f95296039b98..968c47373a9d10 100644 --- a/src/sentry/utils/performance_issues/detectors/n_plus_one_api_calls_detector.py +++ b/src/sentry/utils/performance_issues/detectors/n_plus_one_api_calls_detector.py @@ -14,9 +14,7 @@ from sentry.issues.issue_occurrence import IssueEvidence from sentry.models.organization import Organization from sentry.models.project import Project -from sentry.utils.performance_issues.detectors.utils import get_total_span_duration - -from ..base import ( +from sentry.utils.performance_issues.base import ( DetectorType, PerformanceDetector, fingerprint_http_spans, @@ -25,8 +23,9 @@ get_url_from_span, parameterize_url, ) -from ..performance_problem import PerformanceProblem -from ..types import PerformanceProblemsMap, Span +from sentry.utils.performance_issues.detectors.utils import get_total_span_duration +from sentry.utils.performance_issues.performance_problem import PerformanceProblem +from sentry.utils.performance_issues.types import PerformanceProblemsMap, Span class NPlusOneAPICallsDetector(PerformanceDetector): diff --git a/src/sentry/utils/performance_issues/performance_detection.py b/src/sentry/utils/performance_issues/performance_detection.py index 174acefb3e9130..d8093b0757ef1d 100644 --- a/src/sentry/utils/performance_issues/performance_detection.py +++ b/src/sentry/utils/performance_issues/performance_detection.py @@ -22,6 +22,9 @@ from .base import DetectorType, PerformanceDetector from .detectors.consecutive_db_detector import ConsecutiveDBSpanDetector from .detectors.consecutive_http_detector import ConsecutiveHTTPSpanDetector +from .detectors.experiments.n_plus_one_api_calls_detector import ( + NPlusOneAPICallsExperimentalDetector, +) from .detectors.http_overhead_detector import HTTPOverheadDetector from .detectors.io_main_thread_detector import DBMainThreadDetector, FileIOMainThreadDetector from .detectors.large_payload_detector import LargeHTTPPayloadDetector @@ -322,6 +325,7 @@ def get_detection_settings(project_id: int | None = None) -> dict[DetectorType, NPlusOneDBSpanDetectorExtended, FileIOMainThreadDetector, NPlusOneAPICallsDetector, + NPlusOneAPICallsExperimentalDetector, MNPlusOneDBSpanDetector, UncompressedAssetSpanDetector, LargeHTTPPayloadDetector, diff --git a/tests/sentry/utils/performance_issues/__init__.py b/tests/sentry/utils/performance_issues/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/tests/sentry/utils/performance_issues/experiments/__init__.py b/tests/sentry/utils/performance_issues/experiments/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 From 043ccfa4e051b07c6ab98ed02531f18530a9febc Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 22 Apr 2025 11:12:59 -0400 Subject: [PATCH 2/3] =?UTF-8?q?=E2=9C=A8=20copy=20over=20existing=20detect?= =?UTF-8?q?ors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../n_plus_one_api_calls_detector.py | 310 ++++++++++++ .../test_n_plus_one_api_calls_detector.py | 465 ++++++++++++++++++ 2 files changed, 775 insertions(+) create mode 100644 src/sentry/utils/performance_issues/detectors/experiments/n_plus_one_api_calls_detector.py create mode 100644 tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py diff --git a/src/sentry/utils/performance_issues/detectors/experiments/n_plus_one_api_calls_detector.py b/src/sentry/utils/performance_issues/detectors/experiments/n_plus_one_api_calls_detector.py new file mode 100644 index 00000000000000..6370cacab7038d --- /dev/null +++ b/src/sentry/utils/performance_issues/detectors/experiments/n_plus_one_api_calls_detector.py @@ -0,0 +1,310 @@ +from __future__ import annotations + +import hashlib +import os +from collections import defaultdict +from collections.abc import Mapping, Sequence +from datetime import timedelta +from typing import Any +from urllib.parse import parse_qs, urlparse + +from django.utils.encoding import force_bytes + +from sentry.issues.grouptype import PerformanceNPlusOneAPICallsExperimentalGroupType +from sentry.issues.issue_occurrence import IssueEvidence +from sentry.models.organization import Organization +from sentry.models.project import Project +from sentry.utils.performance_issues.base import ( + DetectorType, + PerformanceDetector, + fingerprint_http_spans, + get_notification_attachment_body, + get_span_evidence_value, + get_url_from_span, + parameterize_url, +) +from sentry.utils.performance_issues.detectors.utils import get_total_span_duration +from sentry.utils.performance_issues.performance_problem import PerformanceProblem +from sentry.utils.performance_issues.types import PerformanceProblemsMap, Span + + +class NPlusOneAPICallsExperimentalDetector(PerformanceDetector): + """ + Detect parallel network calls to the same endpoint. + + [-------- transaction -----------] + [-------- parent span -----------] + [n0] https://service.io/resources/?id=12443 + [n1] https://service.io/resources/?id=13342 + [n2] https://service.io/resources/?id=13441 + ... + """ + + __slots__ = ["stored_problems"] + type = DetectorType.N_PLUS_ONE_API_CALLS + settings_key = DetectorType.N_PLUS_ONE_API_CALLS + + def __init__(self, settings: dict[DetectorType, Any], event: dict[str, Any]) -> None: + super().__init__(settings, event) + + # TODO: Only store the span IDs and timestamps instead of entire span objects + self.stored_problems: PerformanceProblemsMap = {} + self.spans: list[Span] = [] + self.span_hashes: dict[str, str | None] = {} + + def visit_span(self, span: Span) -> None: + if not NPlusOneAPICallsExperimentalDetector.is_span_eligible(span): + return + + op = span.get("op", None) + if op not in self.settings.get("allowed_span_ops", []): + return + + self.span_hashes[span["span_id"]] = get_span_hash(span) + + previous_span = self.spans[-1] if len(self.spans) > 0 else None + + if previous_span is None: + self.spans.append(span) + elif self._spans_are_concurrent(previous_span, span) and self._spans_are_similar( + previous_span, span + ): + self.spans.append(span) + else: + self._maybe_store_problem() + self.spans = [span] + + def is_creation_allowed_for_organization(self, organization: Organization) -> bool: + return True + + def is_creation_allowed_for_project(self, project: Project) -> bool: + return self.settings["detection_enabled"] + + @classmethod + def is_event_eligible(cls, event: dict[str, Any], project: Project | None = None) -> bool: + trace_op = event.get("contexts", {}).get("trace", {}).get("op") + if trace_op and trace_op not in ["navigation", "pageload", "ui.load", "ui.action"]: + return False + + return True + + @classmethod + def is_span_eligible(cls, span: Span) -> bool: + span_id = span.get("span_id", None) + op = span.get("op", None) + hash = span.get("hash", None) + + if not span_id or not op or not hash: + return False + + description = span.get("description") + if not description: + return False + + if description.strip()[:3].upper() != "GET": + return False + + url = get_url_from_span(span) + + # GraphQL URLs have complicated queries in them. Until we parse those + # queries to check for what's duplicated, we can't tell what is being + # duplicated. Ignore them for now + if "graphql" in url: + return False + + # Next.js infixes its data URLs with a build ID. (e.g., + # /_next/data//some-endpoint) This causes a fingerprinting + # explosion, since every deploy would change this ID and create new + # fingerprints. Since we're not parameterizing URLs yet, we need to + # exclude them + if "_next/data" in url: + return False + + # Next.js error pages cause an N+1 API Call that isn't useful to anyone + if "__nextjs_original-stack-frame" in url: + return False + + if not url: + return False + + # Once most users update their SDKs to use the latest standard, we + # won't have to do this, since the URLs will be sent in as `span.data` + # in a parsed format + parsed_url = urlparse(str(url)) + + # Ignore anything that looks like an asset. Some frameworks (and apps) + # fetch assets via XHR, which is not our concern + _pathname, extension = os.path.splitext(parsed_url.path) + if extension and extension in [".js", ".css", ".svg", ".png", ".mp3", ".jpg", ".jpeg"]: + return False + + return True + + def on_complete(self) -> None: + self._maybe_store_problem() + self.spans = [] + + def _maybe_store_problem(self) -> None: + if len(self.spans) < 1: + return + + if len(self.spans) < self.settings["count"]: + return + + total_duration = get_total_span_duration(self.spans) + if total_duration < self.settings["total_duration"]: + return + + last_span = self.spans[-1] + + fingerprint = self._fingerprint() + if not fingerprint: + return + + offender_span_ids = [span["span_id"] for span in self.spans] + + self.stored_problems[fingerprint] = PerformanceProblem( + fingerprint=fingerprint, + op=last_span["op"], + desc=os.path.commonprefix([span.get("description", "") or "" for span in self.spans]), + type=PerformanceNPlusOneAPICallsExperimentalGroupType, + cause_span_ids=[], + parent_span_ids=[last_span.get("parent_span_id", None)], + offender_span_ids=offender_span_ids, + evidence_data={ + "op": last_span["op"], + "cause_span_ids": [], + "parent_span_ids": [last_span.get("parent_span_id", None)], + "offender_span_ids": offender_span_ids, + "transaction_name": self._event.get("transaction", ""), + "num_repeating_spans": str(len(offender_span_ids)) if offender_span_ids else "", + "repeating_spans": self._get_path_prefix(self.spans[0]), + "repeating_spans_compact": get_span_evidence_value(self.spans[0], include_op=False), + "parameters": self._get_parameters(), + }, + evidence_display=[ + IssueEvidence( + name="Offending Spans", + value=get_notification_attachment_body( + last_span["op"], + os.path.commonprefix( + [span.get("description", "") or "" for span in self.spans] + ), + ), + # Has to be marked important to be displayed in the notifications + important=True, + ) + ], + ) + + def _get_parameters(self) -> list[str]: + if not self.spans or len(self.spans) == 0: + return [] + + urls = [get_url_from_span(span) for span in self.spans] + + all_parameters: Mapping[str, list[str]] = defaultdict(list) + + for url in urls: + parsed_url = urlparse(url) + parameters = parse_qs(parsed_url.query) + + for key, value in parameters.items(): + all_parameters[key] += value + + return [ + "{{{}: {}}}".format(key, ",".join(values)) for key, values in all_parameters.items() + ] + + def _get_path_prefix(self, repeating_span: Span) -> str: + if not repeating_span: + return "" + + url = get_url_from_span(repeating_span) + parsed_url = urlparse(url) + return parsed_url.path or "" + + def _fingerprint(self) -> str | None: + first_url = get_url_from_span(self.spans[0]) + parameterized_first_url = parameterize_url(first_url) + + # Check if we parameterized the URL at all. If not, do not attempt + # fingerprinting. Unparameterized URLs run too high a risk of + # fingerprinting explosions. Query parameters are parameterized by + # definition, so exclude them from comparison + if without_query_params(parameterized_first_url) == without_query_params(first_url): + return None + + fingerprint = fingerprint_http_spans([self.spans[0]]) + + return f"1-{PerformanceNPlusOneAPICallsExperimentalGroupType.type_id}-{fingerprint}" + + def _spans_are_concurrent(self, span_a: Span, span_b: Span) -> bool: + span_a_start: int = span_a.get("start_timestamp", 0) or 0 + span_b_start: int = span_b.get("start_timestamp", 0) or 0 + + return timedelta(seconds=abs(span_a_start - span_b_start)) < timedelta( + milliseconds=self.settings["concurrency_threshold"] + ) + + def _spans_are_similar(self, span_a: Span, span_b: Span) -> bool: + return ( + self.span_hashes[span_a["span_id"]] == self.span_hashes[span_b["span_id"]] + and span_a["parent_span_id"] == span_b["parent_span_id"] + ) + + +HTTP_METHODS = { + "GET", + "HEAD", + "POST", + "PUT", + "DELETE", + "CONNECT", + "OPTIONS", + "TRACE", + "PATCH", +} + + +def get_span_hash(span: Span) -> str | None: + if span.get("op") != "http.client": + return span.get("hash") + + parts = remove_http_client_query_string_strategy(span) + if not parts: + return None + + hash = hashlib.md5() + for part in parts: + hash.update(force_bytes(part, errors="replace")) + + return hash.hexdigest()[:16] + + +def remove_http_client_query_string_strategy(span: Span) -> Sequence[str] | None: + """ + This is an inline version of the `http.client` parameterization code in + `"default:2022-10-27"`, the default span grouping strategy at time of + writing. It's inlined here to insulate this detector from changes in the + strategy, which are coming soon. + """ + + # Check the description is of the form ` ` + description = span.get("description") or "" + parts = description.split(" ", 1) + if len(parts) != 2: + return None + + # Ensure that this is a valid http method + method, url_str = parts + method = method.upper() + if method not in HTTP_METHODS: + return None + + url = urlparse(url_str) + return [method, url.scheme, url.netloc, url.path] + + +def without_query_params(url: str) -> str: + return urlparse(url)._replace(query="").geturl() diff --git a/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py b/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py new file mode 100644 index 00000000000000..1aef0468d04651 --- /dev/null +++ b/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py @@ -0,0 +1,465 @@ +from __future__ import annotations + +from collections.abc import Callable +from typing import Any +from uuid import uuid4 + +import pytest + +from sentry.issues.grouptype import PerformanceNPlusOneAPICallsExperimentalGroupType +from sentry.testutils.cases import TestCase +from sentry.testutils.performance_issues.event_generators import ( + create_event, + create_span, + get_event, +) +from sentry.utils.performance_issues.base import DetectorType, parameterize_url +from sentry.utils.performance_issues.detectors.experiments.n_plus_one_api_calls_detector import ( + NPlusOneAPICallsExperimentalDetector, + without_query_params, +) +from sentry.utils.performance_issues.performance_detection import ( + get_detection_settings, + run_detector_on_data, +) +from sentry.utils.performance_issues.performance_problem import PerformanceProblem + + +@pytest.mark.django_db +class NPlusOneAPICallsExperimentalDetectorTest(TestCase): + def setUp(self): + super().setUp() + self._settings = get_detection_settings() + + def find_problems(self, event: dict[str, Any]) -> list[PerformanceProblem]: + detector = NPlusOneAPICallsExperimentalDetector(self._settings, event) + run_detector_on_data(detector, event) + return list(detector.stored_problems.values()) + + def create_event(self, description_maker: Callable[[int], str]) -> dict[str, Any]: + total_duration = self._settings[DetectorType.N_PLUS_ONE_API_CALLS]["total_duration"] + 1 + count = self._settings[DetectorType.N_PLUS_ONE_API_CALLS]["count"] + 1 + hash = uuid4().hex[:16] + + return create_event( + [ + create_span( + "http.client", + total_duration / count, + description_maker(i), + hash=hash, + ) + for i in range(count) + ] + ) + + def create_eligible_spans(self, duration: float, count: int) -> list: + spans = [] + + for i in range(count): + spans.append( + create_span( + "http.client", + duration, + f"GET /api/0/organizations/books?book_id={i}", + f"hash{i}", + ) + ) + + return spans + + def test_detects_problems_with_many_concurrent_calls_to_same_url(self): + event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-issue-stream") + + problems = self.find_problems(event) + assert self.find_problems(event) == [ + PerformanceProblem( + fingerprint="1-1010-d750ce46bb1b13dd5780aac48098d5e20eea682c", + op="http.client", + type=PerformanceNPlusOneAPICallsExperimentalGroupType, + desc="GET /api/0/organizations/sentry/events/?field=replayId&field=count%28%29&per_page=50&query=issue.id%3A", + parent_span_ids=["a0c39078d1570b00"], + cause_span_ids=[], + offender_span_ids=[ + "ba198ace55bdb20f", + "8a20c71faa0fb6a7", + "9269c825d935b33a", + "9ea82f759505e0f3", + "8c55019639e94ab3", + "9b86746e9cc7fbf0", + "806aa31fe1874495", + "bf409b62d9c30197", + "896ac7d28addb37f", + "9c859aeaf6bfaea9", + "950d8f569bbe3d9e", + "b19a2811b457e87a", + "b566d4ce5b46d4f0", + "b33e9da4441a4800", + "8b68818410aa45d8", + "8ac4e73b53fc2077", + "9fe4a1aff019e39e", + "b29cd0c0cd85ae85", + "b3ff0062caa3ea51", + "a3fde2e38a66cc2c", + "b78802cd80762f57", + "9e2ea4d33b1c1bc6", + "bb827dc7a11085f4", + "a34089b08b6d0646", + "950801c0d7576650", + ], + evidence_data={ + "op": "http.client", + "parent_span_ids": ["a0c39078d1570b00"], + "cause_span_ids": [], + "offender_span_ids": [ + "ba198ace55bdb20f", + "8a20c71faa0fb6a7", + "9269c825d935b33a", + "9ea82f759505e0f3", + "8c55019639e94ab3", + "9b86746e9cc7fbf0", + "806aa31fe1874495", + "bf409b62d9c30197", + "896ac7d28addb37f", + "9c859aeaf6bfaea9", + "950d8f569bbe3d9e", + "b19a2811b457e87a", + "b566d4ce5b46d4f0", + "b33e9da4441a4800", + "8b68818410aa45d8", + "8ac4e73b53fc2077", + "9fe4a1aff019e39e", + "b29cd0c0cd85ae85", + "b3ff0062caa3ea51", + "a3fde2e38a66cc2c", + "b78802cd80762f57", + "9e2ea4d33b1c1bc6", + "bb827dc7a11085f4", + "a34089b08b6d0646", + "950801c0d7576650", + ], + }, + evidence_display=[], + ) + ] + assert problems[0].title == "N+1 API Call" + + def test_does_not_detect_problems_with_low_total_duration_of_spans(self): + event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-issue-stream") + event["spans"] = self.create_eligible_spans( + 100, 10 + ) # total duration is 1s, greater than default + + problems = self.find_problems(event) + assert len(problems) == 1 + + event["spans"] = self.create_eligible_spans( + 10, 5 + ) # total duration is 50ms, lower than default + + problems = self.find_problems(event) + assert problems == [] + + def test_detects_problems_with_low_span_duration_high_total_duration(self): + event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-issue-stream") + event["spans"] = self.create_eligible_spans(100, 10) # total duration is 1s + + problems = self.find_problems(event) + assert len(problems) == 1 + + event["spans"] = self.create_eligible_spans(10, 50) # total duration is 500ms + + problems = self.find_problems(event) + assert len(problems) == 1 + + def test_does_not_detect_problems_with_low_span_count(self): + event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-issue-stream") + event["spans"] = self.create_eligible_spans( + 1000, self._settings[DetectorType.N_PLUS_ONE_API_CALLS]["count"] + ) + + problems = self.find_problems(event) + assert len(problems) == 1 + + event["spans"] = self.create_eligible_spans( + 1000, self._settings[DetectorType.N_PLUS_ONE_API_CALLS]["count"] - 1 + ) + + problems = self.find_problems(event) + assert problems == [] + + def test_does_not_detect_problem_with_unparameterized_urls(self): + event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-weather-app") + assert self.find_problems(event) == [] + + def test_does_not_detect_problem_with_concurrent_calls_to_different_urls(self): + event = get_event("n-plus-one-api-calls/not-n-plus-one-api-calls") + assert self.find_problems(event) == [] + + def test_fingerprints_events(self): + event = self.create_event(lambda i: "GET /clients/11/info") + [problem] = self.find_problems(event) + + assert problem.fingerprint == "1-1010-e9daac10ea509a0bf84a8b8da45d36394868ad67" + + def test_fingerprints_identical_relative_urls_together(self): + event1 = self.create_event(lambda i: "GET /clients/11/info") + [problem1] = self.find_problems(event1) + + event2 = self.create_event(lambda i: "GET /clients/11/info") + [problem2] = self.find_problems(event2) + + assert problem1.fingerprint == problem2.fingerprint + + def test_fingerprints_same_relative_urls_together(self): + event1 = self.create_event(lambda i: f"GET /clients/42/info?id={i}") + [problem1] = self.find_problems(event1) + + event2 = self.create_event(lambda i: f"GET /clients/42/info?id={i*2}") + [problem2] = self.find_problems(event2) + + assert problem1.fingerprint == problem2.fingerprint + + def test_fingerprints_same_parameterized_integer_relative_urls_together(self): + event1 = self.create_event(lambda i: f"GET /clients/17/info?id={i}") + [problem1] = self.find_problems(event1) + + event2 = self.create_event(lambda i: f"GET /clients/16/info?id={i*2}") + [problem2] = self.find_problems(event2) + + assert problem1.fingerprint == problem2.fingerprint + + def test_fingerprints_different_relative_url_separately(self): + event1 = self.create_event(lambda i: f"GET /clients/11/info?id={i}") + [problem1] = self.find_problems(event1) + + event2 = self.create_event(lambda i: f"GET /projects/11/details?pid={i}") + [problem2] = self.find_problems(event2) + + assert problem1.fingerprint != problem2.fingerprint + + def test_ignores_hostname_for_fingerprinting(self): + event1 = self.create_event(lambda i: f"GET http://service.io/clients/42/info?id={i}") + [problem1] = self.find_problems(event1) + + event2 = self.create_event(lambda i: f"GET /clients/42/info?id={i}") + [problem2] = self.find_problems(event2) + + assert problem1.fingerprint == problem2.fingerprint + + +@pytest.mark.parametrize( + "url,parameterized_url", + [ + ( + "", + "", + ), + ( + "http://service.io", + "http://service.io", + ), + ( + "https://www.service.io/resources/11", + "https://www.service.io/resources/*", + ), + ( + "https://www.service.io/resources/11/details", + "https://www.service.io/resources/*/details", + ), + ( + "https://www.service.io/resources/11/details?id=1&sort=down", + "https://www.service.io/resources/*/details?id=*&sort=*", + ), + ( + "https://www.service.io/resources/11/details?sort=down&id=1", + "https://www.service.io/resources/*/details?id=*&sort=*", + ), + ( + "https://service.io/clients/somecord/details?id=17", + "https://service.io/clients/somecord/details?id=*", + ), + ( + "/clients/11/project/1343", + "/clients/*/project/*", + ), + ( + "/clients/11/project/1343-turtles", + "/clients/*/project/*", + ), + ( + "/clients/11/project/1343turtles", + "/clients/*/project/1343turtles", + ), + ( + "/clients/563712f9722fb0996ac8f3905b40786f/project/1343", # md5 + "/clients/*/project/*", + ), + ( + "/clients/563712f9722fb0996z/project/", # md5-like + "/clients/563712f9722fb0996z/project/", + ), + ( + "/clients/403926033d001b5279df37cbbe5287b7c7c267fa/project/1343", # sha1 + "/clients/*/project/*", + ), + ( + "/clients/8ff81d74-606d-4c75-ac5e-cee65cbbc866/project/1343", # uuid + "/clients/*/project/*", + ), + ( + "/clients/hello-123s/project/1343", # uuid-like + "/clients/hello-123s/project/*", + ), + ( + "/item/5c9b9b609c172be2a013f534/details", # short hash + "/item/*/details", + ), + ( + "/item/be9a25322d/details", # shorter short hash + "/item/*/details", + ), + ( + "/item/defaced12/details", # false short hash + "/item/defaced12/details", + ), + ( + "/item/defaced12-abba/details", # false short hash 2 + "/item/defaced12-abba/details", + ), + ], +) +def test_parameterizes_url(url, parameterized_url): + r = parameterize_url(url) + assert r == parameterized_url + + +@pytest.mark.parametrize( + "span", + [ + { + "span_id": "a", + "op": "http.client", + "hash": "b", + "description": "GET http://service.io/resource", + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource", + "hash": "a", + "data": { + "url": "/resource", + }, + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource", + "hash": "a", + "data": { + "url": { + "pathname": "/resource", + } + }, + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource.json?param=something", + "hash": "a", + }, + ], +) +def test_allows_eligible_spans(span): + assert NPlusOneAPICallsExperimentalDetector.is_span_eligible(span) + + +@pytest.mark.parametrize( + "span", + [ + {"span_id": "a", "op": None}, + {"op": "http.client"}, + { + "span_id": "a", + "op": "http.client", + "hash": "a", + "description": "POST http://service.io/resource", + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource.js", + "hash": "a", + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET /resource.js", + "hash": "a", + "data": {"url": "/resource.js"}, + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource?graphql=somequery", + "hash": "a", + }, + { + "span_id": "a", + "op": "http.client", + "description": "GET http://service.io/resource", # New JS SDK removes query string from description + "hash": "a", + "data": { + "http.query": "graphql=somequery", + "url": "http://service.io/resource", + }, + }, + { + "span_id": "a", + "op": "http.client", + "hash": "b", + "description": "GET /_next/data/LjdprRSkUtLP0bMUoWLur/items.json?collection=hello", + }, + { + "span_id": "a", + "op": "http.client", + "hash": "b", + "description": "GET /__nextjs_original-stack-frame?isServerSide=false&file=webpack-internal%3A%2F%2F%2F.%2Fnode_modules%2Freact-dom%2Fcjs%2Freact-dom.development.js&methodName=Object.invokeGuardedCallbackDev&arguments=&lineNumber=73&column=3`", + }, + ], +) +def test_rejects_ineligible_spans(span): + assert not NPlusOneAPICallsExperimentalDetector.is_span_eligible(span) + + +@pytest.mark.parametrize( + "url,url_without_query", + [ + ("", ""), + ("http://service.io", "http://service.io"), + ("http://service.io/resource", "http://service.io/resource"), + ("/resource?id=1", "/resource"), + ("/resource?id=1&sort=down", "/resource"), + ], +) +def test_removes_query_params(url, url_without_query): + assert without_query_params(url) == url_without_query + + +@pytest.mark.parametrize( + "event", + [get_event("n-plus-one-api-calls/not-n-plus-one-api-calls")], +) +def test_allows_eligible_events(event): + assert NPlusOneAPICallsExperimentalDetector.is_event_eligible(event) + + +@pytest.mark.parametrize( + "event", + [ + {"contexts": {"trace": {"op": "task"}}}, + ], +) +def test_rejects_ineligible_events(event): + assert not NPlusOneAPICallsExperimentalDetector.is_event_eligible(event) From 30bf6653792e0edac86038239db216fd6417cb8f Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 22 Apr 2025 12:49:18 -0400 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9C=85=20Fix=20group=20type=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../experiments/test_n_plus_one_api_calls_detector.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py b/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py index 1aef0468d04651..3b7b6aefca73bb 100644 --- a/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py +++ b/tests/sentry/utils/performance_issues/experiments/test_n_plus_one_api_calls_detector.py @@ -74,7 +74,7 @@ def test_detects_problems_with_many_concurrent_calls_to_same_url(self): problems = self.find_problems(event) assert self.find_problems(event) == [ PerformanceProblem( - fingerprint="1-1010-d750ce46bb1b13dd5780aac48098d5e20eea682c", + fingerprint=f"1-{PerformanceNPlusOneAPICallsExperimentalGroupType.type_id}-d750ce46bb1b13dd5780aac48098d5e20eea682c", op="http.client", type=PerformanceNPlusOneAPICallsExperimentalGroupType, desc="GET /api/0/organizations/sentry/events/?field=replayId&field=count%28%29&per_page=50&query=issue.id%3A", @@ -142,7 +142,7 @@ def test_detects_problems_with_many_concurrent_calls_to_same_url(self): evidence_display=[], ) ] - assert problems[0].title == "N+1 API Call" + assert problems[0].title == "N+1 API Call (Experimental)" def test_does_not_detect_problems_with_low_total_duration_of_spans(self): event = get_event("n-plus-one-api-calls/n-plus-one-api-calls-in-issue-stream") @@ -200,7 +200,10 @@ def test_fingerprints_events(self): event = self.create_event(lambda i: "GET /clients/11/info") [problem] = self.find_problems(event) - assert problem.fingerprint == "1-1010-e9daac10ea509a0bf84a8b8da45d36394868ad67" + assert ( + problem.fingerprint + == f"1-{PerformanceNPlusOneAPICallsExperimentalGroupType.type_id}-e9daac10ea509a0bf84a8b8da45d36394868ad67" + ) def test_fingerprints_identical_relative_urls_together(self): event1 = self.create_event(lambda i: "GET /clients/11/info")