From 0597a70fad2c896995b42c9da51a895e11be7aea Mon Sep 17 00:00:00 2001 From: Abdkhan14 <60121741+Abdkhan14@users.noreply.github.com> Date: Tue, 25 Jul 2023 11:25:35 -0400 Subject: [PATCH] =?UTF-8?q?feat(perf-detector-threshold-configuration)=20A?= =?UTF-8?q?dded=20new=20thresholds=20and=20=E2=80=A6=20(#53460)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For project: [Detector Threshold Configurations](https://www.notion.so/sentry/Detector-Threshold-Configuration-f8cb07e7cceb42388cedb09ea05fc116) Lowered defaults values and effects: [link](https://www.notion.so/sentry/Detector-Dry-Run-Results-40dc7a3e4d8b4b9ea8da90608fe54747) - We are GA'ing the project with lowered default values for thresholds and adding new thresholds. - Changed default threshold for Consecutive HTTP to 500ms to match prod sentry_option value: [query](https://redash.getsentry.net/queries/4546/source) - Added lowered defaults for N+1 DB duration (iterating down to 50ms), Slow DB Queries duration(iterating down to 500ms), Consecutive http span duration (iterating down to 500ms) - Updated Consecutive HTTP Detector to add new total min time saved threshold with a default of 2000s. - Updated Consecutive HTTP Detector tests to ensure new total duration threshold is respected. - Removed Extended Consecutive HTTP Detector and tests. --------- Co-authored-by: Abdullah Khan --- .../project_performance_issue_settings.py | 5 + src/sentry/options/defaults.py | 13 +- src/sentry/utils/performance_issues/base.py | 2 - .../detectors/consecutive_http_detector.py | 93 +---- .../performance_detection.py | 14 +- ...test_project_performance_issue_settings.py | 4 + .../test_consecutive_http_detector.py | 180 +++++++++- ...test_consecutive_http_detector_extended.py | 334 ------------------ .../test_performance_detection.py | 7 +- .../test_slow_db_span_detector.py | 2 +- 10 files changed, 219 insertions(+), 435 deletions(-) delete mode 100644 tests/sentry/utils/performance_issues/test_consecutive_http_detector_extended.py diff --git a/src/sentry/api/endpoints/project_performance_issue_settings.py b/src/sentry/api/endpoints/project_performance_issue_settings.py index 5151cd344caf92..274035fd67db84 100644 --- a/src/sentry/api/endpoints/project_performance_issue_settings.py +++ b/src/sentry/api/endpoints/project_performance_issue_settings.py @@ -58,6 +58,7 @@ class ConfigurableThresholds(Enum): RENDER_BLOCKING_ASSET_FCP_RATIO = "render_blocking_fcp_ratio" SLOW_DB_QUERY_DURATION = "slow_db_query_duration_threshold" N_PLUS_API_CALLS_DURATION = "n_plus_one_api_calls_total_duration_threshold" + CONSECUTIVE_HTTP_SPANS_MIN_TIME_SAVED = "consecutive_http_spans_min_time_saved_threshold" internal_only_project_settings_to_group_map: Dict[str, Type[GroupType]] = { @@ -84,6 +85,7 @@ class ConfigurableThresholds(Enum): ConfigurableThresholds.RENDER_BLOCKING_ASSET_FCP_RATIO.value: InternalProjectOptions.RENDER_BLOCKING_ASSET.value, ConfigurableThresholds.SLOW_DB_QUERY_DURATION.value: InternalProjectOptions.SLOW_DB_QUERY.value, ConfigurableThresholds.N_PLUS_API_CALLS_DURATION.value: InternalProjectOptions.N_PLUS_ONE_API_CALLS.value, + ConfigurableThresholds.CONSECUTIVE_HTTP_SPANS_MIN_TIME_SAVED.value: InternalProjectOptions.CONSECUTIVE_HTTP_SPANS.value, } @@ -125,6 +127,9 @@ class ProjectPerformanceIssueSettingsSerializer(serializers.Serializer): n_plus_one_api_calls_total_duration_threshold = serializers.IntegerField( required=False, min_value=100, max_value=TEN_SECONDS # ms ) + consecutive_http_spans_min_time_saved_threshold = serializers.IntegerField( + required=False, min_value=1000, max_value=TEN_SECONDS # ms + ) uncompressed_assets_detection_enabled = serializers.BooleanField(required=False) consecutive_http_spans_detection_enabled = serializers.BooleanField(required=False) large_http_payload_detection_enabled = serializers.BooleanField(required=False) diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 381ddb2be82421..1cf639ef0f678c 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -1229,12 +1229,12 @@ ) register( "performance.issues.n_plus_one_db.duration_threshold", - default=100.0, + default=90.0, flags=FLAG_AUTOMATOR_MODIFIABLE, ) register( "performance.issues.slow_db_query.duration_threshold", - default=1000.0, # ms + default=900.0, # ms flags=FLAG_AUTOMATOR_MODIFIABLE, ) register( @@ -1259,7 +1259,7 @@ ) register( "performance.issues.consecutive_http.max_duration_between_spans", - default=500, + default=500, # ms flags=FLAG_AUTOMATOR_MODIFIABLE, ) register( @@ -1269,7 +1269,12 @@ ) register( "performance.issues.consecutive_http.span_duration_threshold", - default=1000, + default=900, # ms + flags=FLAG_AUTOMATOR_MODIFIABLE, +) +register( + "performance.issues.consecutive_http.min_time_saved_threshold", + default=2000, # ms flags=FLAG_AUTOMATOR_MODIFIABLE, ) register( diff --git a/src/sentry/utils/performance_issues/base.py b/src/sentry/utils/performance_issues/base.py index ae0db4e3f3771a..1b35273f860193 100644 --- a/src/sentry/utils/performance_issues/base.py +++ b/src/sentry/utils/performance_issues/base.py @@ -37,7 +37,6 @@ class DetectorType(Enum): N_PLUS_ONE_API_CALLS = "n_plus_one_api_calls" CONSECUTIVE_DB_OP = "consecutive_db" CONSECUTIVE_HTTP_OP = "consecutive_http" - CONSECUTIVE_HTTP_OP_EXTENDED = "consecutive_http_ext" LARGE_HTTP_PAYLOAD = "large_http_payload" FILE_IO_MAIN_THREAD = "file_io_main_thread" M_N_PLUS_ONE_DB = "m_n_plus_one_db" @@ -57,7 +56,6 @@ class DetectorType(Enum): DetectorType.M_N_PLUS_ONE_DB: PerformanceMNPlusOneDBQueriesGroupType, DetectorType.UNCOMPRESSED_ASSETS: PerformanceUncompressedAssetsGroupType, DetectorType.CONSECUTIVE_HTTP_OP: PerformanceConsecutiveHTTPQueriesGroupType, - DetectorType.CONSECUTIVE_HTTP_OP_EXTENDED: PerformanceConsecutiveHTTPQueriesGroupType, DetectorType.DB_MAIN_THREAD: PerformanceDBMainThreadGroupType, DetectorType.LARGE_HTTP_PAYLOAD: PerformanceLargeHTTPPayloadGroupType, DetectorType.HTTP_OVERHEAD: PerformanceHTTPOverheadGroupType, diff --git a/src/sentry/utils/performance_issues/detectors/consecutive_http_detector.py b/src/sentry/utils/performance_issues/detectors/consecutive_http_detector.py index 26fcbb790367cd..3156bef1c7ba29 100644 --- a/src/sentry/utils/performance_issues/detectors/consecutive_http_detector.py +++ b/src/sentry/utils/performance_issues/detectors/consecutive_http_detector.py @@ -50,6 +50,10 @@ def visit_span(self, span: Span) -> None: if not span_id or not self._is_eligible_http_span(span): return + span_duration = get_span_duration(span).total_seconds() * 1000 + if span_duration < self.settings.get("span_duration_threshold"): + return + if self._overlaps_last_span(span): self._validate_and_store_performance_problem() self._reset_variables() @@ -63,11 +67,12 @@ def _validate_and_store_performance_problem(self): exceeds_count_threshold = len(self.consecutive_http_spans) >= self.settings.get( "consecutive_count_threshold" ) - exceeds_span_duration_threshold = all( - get_span_duration(span).total_seconds() * 1000 - > self.settings.get("span_duration_threshold") - for span in self.consecutive_http_spans - ) + + exceeds_min_time_saved_duration = False + if self.consecutive_http_spans: + exceeds_min_time_saved_duration = self._calculate_time_saved() >= self.settings.get( + "min_time_saved" + ) subceeds_duration_between_spans_threshold = all( get_duration_between_spans( @@ -79,11 +84,17 @@ def _validate_and_store_performance_problem(self): if ( exceeds_count_threshold - and exceeds_span_duration_threshold and subceeds_duration_between_spans_threshold + and exceeds_min_time_saved_duration ): self._store_performance_problem() + def _calculate_time_saved(self) -> float: + total_time = get_total_span_duration(self.consecutive_http_spans) + max_span_duration = get_max_span_duration(self.consecutive_http_spans) + + return total_time - max_span_duration + def _store_performance_problem(self) -> None: fingerprint = self._fingerprint() offender_span_ids = [span.get("span_id", None) for span in self.consecutive_http_spans] @@ -166,73 +177,3 @@ def is_creation_allowed_for_organization(self, organization: Organization) -> bo def is_creation_allowed_for_project(self, project: Project) -> bool: return self.settings["detection_enabled"] - - -class ConsecutiveHTTPSpanDetectorExtended(ConsecutiveHTTPSpanDetector): - """ - Detector goals: - - Extend Consecutive HTTP Span Detector to mimic detection using thresholds from - - Consecutive DB Queries Detector. - """ - - type = DetectorType.CONSECUTIVE_HTTP_OP_EXTENDED - settings_key = DetectorType.CONSECUTIVE_HTTP_OP_EXTENDED - - def visit_span(self, span: Span) -> None: - if is_event_from_browser_javascript_sdk(self.event()): - return - - span_id = span.get("span_id", None) - - if not span_id or not self._is_eligible_http_span(span): - return - - span_duration = get_span_duration(span).total_seconds() * 1000 - if span_duration < self.settings.get("span_duration_threshold"): - return - - if self._overlaps_last_span(span): - self._validate_and_store_performance_problem() - self._reset_variables() - - self._add_problem_span(span) - - def _validate_and_store_performance_problem(self): - exceeds_count_threshold = len(self.consecutive_http_spans) >= self.settings.get( - "consecutive_count_threshold" - ) - - exceeds_min_time_saved_duration = False - if self.consecutive_http_spans: - exceeds_min_time_saved_duration = self._calculate_time_saved() >= self.settings.get( - "min_time_saved" - ) - - subceeds_duration_between_spans_threshold = all( - get_duration_between_spans( - self.consecutive_http_spans[idx - 1], self.consecutive_http_spans[idx] - ) - < self.settings.get("max_duration_between_spans") - for idx in range(1, len(self.consecutive_http_spans)) - ) - - if ( - exceeds_count_threshold - and subceeds_duration_between_spans_threshold - and exceeds_min_time_saved_duration - ): - self._store_performance_problem() - - def _calculate_time_saved(self) -> float: - total_time = get_total_span_duration(self.consecutive_http_spans) - max_span_duration = get_max_span_duration(self.consecutive_http_spans) - - return total_time - max_span_duration - - def is_creation_allowed_for_organization(self, organization: Organization) -> bool: - # Only collecting metrics. - return False - - def is_creation_allowed_for_project(self, project: Project) -> bool: - # Only collecting metrics. - return False diff --git a/src/sentry/utils/performance_issues/performance_detection.py b/src/sentry/utils/performance_issues/performance_detection.py index 50b13e1140ea56..fab4a3b944b1cf 100644 --- a/src/sentry/utils/performance_issues/performance_detection.py +++ b/src/sentry/utils/performance_issues/performance_detection.py @@ -14,9 +14,6 @@ from sentry.utils import metrics from sentry.utils.event import is_event_from_browser_javascript_sdk from sentry.utils.event_frames import get_sdk_name -from sentry.utils.performance_issues.detectors.consecutive_http_detector import ( - ConsecutiveHTTPSpanDetectorExtended, -) from sentry.utils.safe import get_path from .base import DetectorType, PerformanceDetector @@ -159,6 +156,9 @@ def get_merged_settings(project_id: Optional[int] = None) -> Dict[str | Any, Any "consecutive_http_spans_span_duration_threshold": options.get( "performance.issues.consecutive_http.span_duration_threshold" ), + "consecutive_http_spans_min_time_saved_threshold": options.get( + "performance.issues.consecutive_http.min_time_saved_threshold" + ), "large_http_payload_size_threshold": options.get( "performance.issues.large_http_payload.size_threshold" ), @@ -287,18 +287,13 @@ def get_detection_settings(project_id: Optional[int] = None) -> Dict[DetectorTyp "span_duration_threshold": settings[ "consecutive_http_spans_span_duration_threshold" ], # ms + "min_time_saved": settings["consecutive_http_spans_min_time_saved_threshold"], # ms "consecutive_count_threshold": settings["consecutive_http_spans_count_threshold"], "max_duration_between_spans": settings[ "consecutive_http_spans_max_duration_between_spans" ], # ms "detection_enabled": settings["consecutive_http_spans_detection_enabled"], }, - DetectorType.CONSECUTIVE_HTTP_OP_EXTENDED: { - "span_duration_threshold": 500, # ms - "min_time_saved": 2000, # time saved by running all queries in parallel - "consecutive_count_threshold": 3, - "max_duration_between_spans": 1000, # ms - }, DetectorType.LARGE_HTTP_PAYLOAD: { "payload_size_threshold": settings["large_http_payload_size_threshold"], "detection_enabled": settings["large_http_payload_detection_enabled"], @@ -320,7 +315,6 @@ def _detect_performance_problems( detectors: List[PerformanceDetector] = [ ConsecutiveDBSpanDetector(detection_settings, data), ConsecutiveHTTPSpanDetector(detection_settings, data), - ConsecutiveHTTPSpanDetectorExtended(detection_settings, data), DBMainThreadDetector(detection_settings, data), SlowDBQueryDetector(detection_settings, data), RenderBlockingAssetSpanDetector(detection_settings, data), diff --git a/tests/sentry/api/endpoints/test_project_performance_issue_settings.py b/tests/sentry/api/endpoints/test_project_performance_issue_settings.py index 62a6eaa3fdd8a4..bd567a566690fc 100644 --- a/tests/sentry/api/endpoints/test_project_performance_issue_settings.py +++ b/tests/sentry/api/endpoints/test_project_performance_issue_settings.py @@ -92,6 +92,7 @@ def test_get_project_options_overrides_threshold_defaults(self, get_value): "performance.issues.uncompressed_asset.size_threshold": 200000, "performance.issues.consecutive_db.min_time_saved_threshold": 300, "performance.issues.n_plus_one_api_calls.total_duration": 300, + "performance.issues.consecutive_http.min_time_saved_threshold": 2000, } ): with self.feature(PERFORMANCE_ISSUE_FEATURES): @@ -110,6 +111,7 @@ def test_get_project_options_overrides_threshold_defaults(self, get_value): assert response.data["uncompressed_asset_size_threshold"] == 200000 assert response.data["consecutive_db_min_time_saved_threshold"] == 300 assert response.data["n_plus_one_api_calls_total_duration_threshold"] == 300 + assert response.data["consecutive_http_spans_min_time_saved_threshold"] == 2000 get_value.return_value = { "n_plus_one_db_duration_threshold": 10000, @@ -122,6 +124,7 @@ def test_get_project_options_overrides_threshold_defaults(self, get_value): "file_io_on_main_thread_duration_threshold": 33, "consecutive_db_min_time_saved_threshold": 5000, "n_plus_one_api_calls_total_duration_threshold": 500, + "consecutive_http_spans_min_time_saved_threshold": 1000, } with self.feature(PERFORMANCE_ISSUE_FEATURES): @@ -140,6 +143,7 @@ def test_get_project_options_overrides_threshold_defaults(self, get_value): assert response.data["file_io_on_main_thread_duration_threshold"] == 33 assert response.data["consecutive_db_min_time_saved_threshold"] == 5000 assert response.data["n_plus_one_api_calls_total_duration_threshold"] == 500 + assert response.data["consecutive_http_spans_min_time_saved_threshold"] == 1000 def test_get_returns_error_without_feature_enabled(self): with self.feature({}): diff --git a/tests/sentry/utils/performance_issues/test_consecutive_http_detector.py b/tests/sentry/utils/performance_issues/test_consecutive_http_detector.py index 4765241c90a932..9af12c3e0c85a0 100644 --- a/tests/sentry/utils/performance_issues/test_consecutive_http_detector.py +++ b/tests/sentry/utils/performance_issues/test_consecutive_http_detector.py @@ -5,7 +5,7 @@ import pytest from sentry.issues.grouptype import PerformanceConsecutiveHTTPQueriesGroupType -from sentry.models import ProjectOption +from sentry.models.options.project_option import ProjectOption from sentry.spans.grouping.strategy.base import Span from sentry.testutils import TestCase from sentry.testutils.performance_issues.event_generators import ( @@ -23,10 +23,12 @@ ) from sentry.utils.performance_issues.performance_problem import PerformanceProblem +MIN_SPAN_DURATION = 900 # ms + @region_silo_test @pytest.mark.django_db -class ConsecutiveDbDetectorTest(TestCase): +class ConsecutiveHTTPSpansDetectorTest(TestCase): def setUp(self): super().setUp() self._settings = get_detection_settings() @@ -89,6 +91,32 @@ def test_detects_consecutive_http_issue(self): ) ] + def test_does_not_detects_consecutive_http_issue_low_time_saved(self): + spans = [ # min time saved by parallelizing is 2s + create_span("http.client", 1000, "GET /api/0/organizations/endpoint1", "hash1"), + create_span("http.client", 1000, "GET /api/0/organizations/endpoint2", "hash2"), + create_span("http.client", 1000, "GET /api/0/organizations/endpoint3", "hash3"), + ] + spans = [ + modify_span_start(span, 1000 * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert len(problems) == 1 + + spans = [ # min time saved by parallelizing is 1s + create_span("http.client", 500, "GET /api/0/organizations/endpoint1", "hash1"), + create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), + create_span("http.client", 1000, "GET /api/0/organizations/endpoint3", "hash3"), + ] + spans = [ + modify_span_start(span, 1000 * spans.index(span)) for span in spans + ] # ensure spans don't overlap + + problems = self.find_problems(create_event(spans)) + + assert problems == [] + def test_does_not_detect_consecutive_http_issue_with_frontend_events(self): event = { **self.create_issue_event(), @@ -97,12 +125,139 @@ def test_does_not_detect_consecutive_http_issue_with_frontend_events(self): problems = self.find_problems(event) assert problems == [] - def test_does_not_detect_consecutive_http_issue_with_low_duration(self): - event = self.create_issue_event(100) - problems = self.find_problems(event) + def test_does_not_detect_consecutive_http_issue_with_low_count(self): + spans = [ # all thresholds are exceeded, except count + create_span("http.client", 3000, "GET /api/0/organizations/endpoint1", "hash1"), + create_span("http.client", 3000, "GET /api/0/organizations/endpoint2", "hash2"), + ] + spans = [ + modify_span_start(span, 3000 * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) assert problems == [] + def test_detects_consecutive_http_issue_with_trailing_low_duration_span(self): + spans = [ + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint1", "hash1" + ), # all thresholds are exceeded. + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint2", "hash2" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint3", "hash3" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint4", "hash4" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint5", "hash5" + ), + ] + spans = [ + modify_span_start(span, MIN_SPAN_DURATION * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert len(problems) == 1 + + spans = [ + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint1", "hash1" + ), # some spans with low durations, all other thresholds are exceeded. + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint2", "hash2" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint3", "hash3" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint4", "hash4" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint5", "hash5" + ), + create_span("http.client", 400, "GET /api/0/organizations/endpoint6", "hash6"), + ] + spans = [ + modify_span_start(span, MIN_SPAN_DURATION * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert len(problems) == 1 + + def test_does_not_detect_consecutive_http_issue_with_low_duration_spans(self): + spans = [ + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint1", "hash1" + ), # all thresholds are exceeded. + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint2", "hash2" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint3", "hash3" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint4", "hash4" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint5", "hash5" + ), + ] + spans = [ + modify_span_start(span, MIN_SPAN_DURATION * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert len(problems) == 1 + + spans = [ + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint1", "hash1" + ), # some spans with low durations, all other thresholds are exceeded. + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint2", "hash2" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint3", "hash3" + ), + create_span("http.client", 400, "GET /api/0/organizations/endpoint4", "hash4"), + create_span("http.client", 400, "GET /api/0/organizations/endpoint5", "hash5"), + create_span("http.client", 400, "GET /api/0/organizations/endpoint5", "hash5"), + ] + spans = [ + modify_span_start(span, MIN_SPAN_DURATION * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert problems == [] + + def test_detects_consecutive_http_issue_with_low_duration_spans(self): + spans = [ + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint1", "hash1" + ), # spans with low durations, but min_time_saved + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint2", "hash2" + ), # exceeds threshold + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint3", "hash3" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint4", "hash4" + ), + create_span( + "http.client", MIN_SPAN_DURATION, "GET /api/0/organizations/endpoint5", "hash5" + ), + ] + spans = [ + modify_span_start(span, MIN_SPAN_DURATION * spans.index(span)) for span in spans + ] # ensure spans don't overlap + problems = self.find_problems(create_event(spans)) + + assert len(problems) == 1 + def test_detects_consecutive_with_non_http_between_http_spans(self): spans = self.create_issue_spans() @@ -142,7 +297,20 @@ def test_detects_consecutive_with_non_http_between_http_spans(self): ] def test_does_not_detect_nextjs_asset(self): - spans = self.create_issue_spans() + span_duration = 2000 # ms + spans = [ + create_span( + "http.client", span_duration, "GET /api/0/organizations/endpoint1", "hash1" + ), + create_span( + "http.client", span_duration, "GET /api/0/organizations/endpoint2", "hash2" + ), + create_span( + "http.client", span_duration, "GET /api/0/organizations/endpoint3", "hash3" + ), + ] + spans = [modify_span_start(span, span_duration * spans.index(span)) for span in spans] + assert len(self.find_problems(create_event(spans))) == 1 spans[0] = modify_span_start( diff --git a/tests/sentry/utils/performance_issues/test_consecutive_http_detector_extended.py b/tests/sentry/utils/performance_issues/test_consecutive_http_detector_extended.py deleted file mode 100644 index 890c766cd9fd4a..00000000000000 --- a/tests/sentry/utils/performance_issues/test_consecutive_http_detector_extended.py +++ /dev/null @@ -1,334 +0,0 @@ -from __future__ import annotations - -from typing import Any - -import pytest - -from sentry.issues.grouptype import PerformanceConsecutiveHTTPQueriesGroupType -from sentry.spans.grouping.strategy.base import Span -from sentry.testutils import TestCase -from sentry.testutils.performance_issues.event_generators import ( - create_event, - create_span, - modify_span_start, -) -from sentry.testutils.silo import region_silo_test -from sentry.utils.performance_issues.detectors.consecutive_http_detector import ( - ConsecutiveHTTPSpanDetectorExtended, -) -from sentry.utils.performance_issues.performance_detection import ( - get_detection_settings, - run_detector_on_data, -) -from sentry.utils.performance_issues.performance_problem import PerformanceProblem - - -@region_silo_test -@pytest.mark.django_db -class ConsecutiveHTTPSpansDetectorExtendedTest(TestCase): - def setUp(self): - super().setUp() - self._settings = get_detection_settings() - - def find_problems(self, event: dict[str, Any]) -> list[PerformanceProblem]: - detector = ConsecutiveHTTPSpanDetectorExtended(self._settings, event) - run_detector_on_data(detector, event) - return list(detector.stored_problems.values()) - - def create_issue_spans(self, span_duration=2000) -> list[Span]: - spans = [ - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint1", "hash1" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint2", "hash2" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint3", "hash3" - ), - ] - spans = [ - modify_span_start(span, span_duration * spans.index(span)) for span in spans - ] # ensure spans don't overlap - - return spans - - def create_issue_event(self, span_duration=2000): - spans = self.create_issue_spans(span_duration) - return create_event(spans) - - def test_detects_consecutive_http_issue(self): - event = self.create_issue_event() - problems = self.find_problems(event) - - assert problems == [ - PerformanceProblem( - fingerprint="1-1009-00b8644b56309c8391aa365783145162ab9c589a", - op="http", - desc="GET /api/0/organizations/endpoint1", - type=PerformanceConsecutiveHTTPQueriesGroupType, - parent_span_ids=None, - cause_span_ids=[], - offender_span_ids=[ - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - ], - evidence_data={ - "parent_span_ids": [], - "cause_span_ids": [], - "offender_span_ids": [ - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - ], - "op": "http", - }, - evidence_display=[], - ) - ] - - def test_does_not_detects_consecutive_http_issue_low_time_saved(self): - spans = [ # min time saved by parallelizing is 2s - create_span("http.client", 1000, "GET /api/0/organizations/endpoint1", "hash1"), - create_span("http.client", 1000, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 1000, "GET /api/0/organizations/endpoint3", "hash3"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert len(problems) == 1 - - spans = [ # min time saved by parallelizing is 1s - create_span("http.client", 500, "GET /api/0/organizations/endpoint1", "hash1"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 1000, "GET /api/0/organizations/endpoint3", "hash3"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - - problems = self.find_problems(create_event(spans)) - - assert problems == [] - - def test_does_not_detect_consecutive_http_issue_with_frontend_events(self): - event = { - **self.create_issue_event(), - "sdk": {"name": "sentry.javascript.browser"}, - } - problems = self.find_problems(event) - assert problems == [] - - def test_does_not_detect_consecutive_http_issue_with_low_count(self): - spans = [ # count less than threshold - create_span("http.client", 20, "GET /api/0/organizations/endpoint1", "hash1"), - ] - - problems = self.find_problems(create_event(spans)) - assert problems == [] - - def test_detects_consecutive_http_issue_with_trailing_low_duration_span(self): - spans = [ - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint1", "hash1" - ), # all thresholds are exceeded. - create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint3", "hash3"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint4", "hash4"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint5", "hash5"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert len(problems) == 1 - - spans = [ - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint1", "hash1" - ), # some spans with low durations, all other thresholds are exceeded. - create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint3", "hash3"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint4", "hash4"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint5", "hash5"), - create_span("http.client", 400, "GET /api/0/organizations/endpoint6", "hash6"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert len(problems) == 1 - - def test_does_not_detect_consecutive_http_issue_with_low_duration_spans(self): - spans = [ - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint1", "hash1" - ), # all thresholds are exceeded. - create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint3", "hash3"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint4", "hash4"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint5", "hash5"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert len(problems) == 1 - - spans = [ - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint1", "hash1" - ), # some spans with low durations, all other thresholds are exceeded. - create_span("http.client", 500, "GET /api/0/organizations/endpoint2", "hash2"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint3", "hash3"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint4", "hash4"), - create_span("http.client", 400, "GET /api/0/organizations/endpoint5", "hash5"), - create_span("http.client", 400, "GET /api/0/organizations/endpoint5", "hash5"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert problems == [] - - def test_detects_consecutive_http_issue_with_low_duration_spans(self): - spans = [ - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint1", "hash1" - ), # spans with low durations, but min_time_saved - create_span( - "http.client", 500, "GET /api/0/organizations/endpoint2", "hash2" - ), # exceeds threshold - create_span("http.client", 500, "GET /api/0/organizations/endpoint3", "hash3"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint4", "hash4"), - create_span("http.client", 500, "GET /api/0/organizations/endpoint5", "hash5"), - ] - spans = [ - modify_span_start(span, 1000 * spans.index(span)) for span in spans - ] # ensure spans don't overlap - problems = self.find_problems(create_event(spans)) - - assert len(problems) == 1 - - def test_detects_consecutive_with_non_http_between_http_spans(self): - spans = self.create_issue_spans() - - spans.insert( - 1, modify_span_start(create_span("resource.script", 500, "/static/js/bundle.js"), 2000) - ) - - event = create_event(spans) - - problems = self.find_problems(event) - - assert problems == [ - PerformanceProblem( - fingerprint="1-1009-00b8644b56309c8391aa365783145162ab9c589a", - op="http", - desc="GET /api/0/organizations/endpoint1", - type=PerformanceConsecutiveHTTPQueriesGroupType, - parent_span_ids=None, - cause_span_ids=[], - offender_span_ids=[ - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - ], - evidence_data={ - "parent_span_ids": [], - "cause_span_ids": [], - "offender_span_ids": [ - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - "bbbbbbbbbbbbbbbb", - ], - "op": "http", - }, - evidence_display=[], - ) - ] - - def test_does_not_detect_nextjs_asset(self): - span_duration = 2000 # ms - spans = [ - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint1", "hash1" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint2", "hash2" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint3", "hash3" - ), - ] - spans = [modify_span_start(span, span_duration * spans.index(span)) for span in spans] - - assert len(self.find_problems(create_event(spans))) == 1 - - spans[0] = modify_span_start( - create_span("http.client", 2000, "GET /_next/static/css/file-hash-abc.css", "hash4"), - 0, - ) - - assert self.find_problems(create_event(spans)) == [] - - def test_does_not_detect_with_high_duration_between_spans(self): - span_duration = 2000 - spans = [ - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint1", "hash1" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint2", "hash2" - ), - create_span( - "http.client", span_duration, "GET /api/0/organizations/endpoint3", "hash3" - ), - ] - - spans = [ - modify_span_start(span, (10000 + span_duration) * spans.index(span)) for span in spans - ] # ensure spans don't overlap - - assert self.find_problems(create_event(spans)) == [] - - def test_fingerprints_match_with_duplicate_http(self): - span_duration = 2000 - spans = [ - create_span("http.client", span_duration, "GET /api/endpoint1", "hash1"), - create_span("http.client", span_duration, "GET /api/endpoint2", "hash2"), - create_span("http.client", span_duration, "GET /api/endpoint3", "hash3"), - ] - - spans = [ - modify_span_start(span, span_duration * spans.index(span)) for span in spans - ] # ensure spans don't overlap - - problem_1 = self.find_problems(create_event(spans))[0] - - spans.append( - modify_span_start( - create_span("http.client", span_duration, "GET /api/endpoint3", "hash3"), 6000 - ) - ) - - problem_2 = self.find_problems(create_event(spans))[0] - - assert problem_2.fingerprint == "1-1009-515a42c2614f98fa886b6d9ad1ddfe1929329f53" - assert problem_1.fingerprint == problem_2.fingerprint - - def test_issue_creation_not_allowed_for_project(self): - project = self.create_project() - event = self.create_issue_event() - - settings = get_detection_settings(project.id) - detector = ConsecutiveHTTPSpanDetectorExtended(settings, event) - - assert not detector.is_creation_allowed_for_project(project) diff --git a/tests/sentry/utils/performance_issues/test_performance_detection.py b/tests/sentry/utils/performance_issues/test_performance_detection.py index 2c758fc2a452f8..6d00afccdac356 100644 --- a/tests/sentry/utils/performance_issues/test_performance_detection.py +++ b/tests/sentry/utils/performance_issues/test_performance_detection.py @@ -182,7 +182,7 @@ def test_project_options_overrides_default_threshold_settings(self): default_settings = get_detection_settings(self.project) - assert default_settings[DetectorType.N_PLUS_ONE_DB_QUERIES]["duration_threshold"] == 100 + assert default_settings[DetectorType.N_PLUS_ONE_DB_QUERIES]["duration_threshold"] == 90 assert ( default_settings[DetectorType.RENDER_BLOCKING_ASSET_SPAN]["fcp_ratio_threshold"] == 0.33 ) @@ -193,9 +193,10 @@ def test_project_options_overrides_default_threshold_settings(self): ) assert default_settings[DetectorType.UNCOMPRESSED_ASSETS]["duration_threshold"] == 300 assert default_settings[DetectorType.CONSECUTIVE_DB_OP]["min_time_saved"] == 100 - assert default_settings[DetectorType.SLOW_DB_QUERY][0]["duration_threshold"] == 1000 assert default_settings[DetectorType.N_PLUS_ONE_API_CALLS]["total_duration"] == 300 assert default_settings[DetectorType.LARGE_HTTP_PAYLOAD]["payload_size_threshold"] == 300000 + assert default_settings[DetectorType.CONSECUTIVE_HTTP_OP]["min_time_saved"] == 2000 + assert default_settings[DetectorType.SLOW_DB_QUERY][0]["duration_threshold"] == 900 self.project_option_mock.return_value = { "n_plus_one_db_duration_threshold": 100000, @@ -208,6 +209,7 @@ def test_project_options_overrides_default_threshold_settings(self): "file_io_on_main_thread_duration_threshold": 33, "consecutive_db_min_time_saved_threshold": 500, "n_plus_one_api_calls_total_duration_threshold": 500, + "consecutive_http_spans_min_time_saved_threshold": 1000, } configured_settings = get_detection_settings(self.project) @@ -232,6 +234,7 @@ def test_project_options_overrides_default_threshold_settings(self): assert configured_settings[DetectorType.DB_MAIN_THREAD][0]["duration_threshold"] == 50 assert configured_settings[DetectorType.FILE_IO_MAIN_THREAD][0]["duration_threshold"] == 33 assert configured_settings[DetectorType.CONSECUTIVE_DB_OP]["min_time_saved"] == 500 + assert configured_settings[DetectorType.CONSECUTIVE_HTTP_OP]["min_time_saved"] == 1000 @override_options(BASE_DETECTOR_OPTIONS) def test_n_plus_one_extended_detection_no_parent_span(self): diff --git a/tests/sentry/utils/performance_issues/test_slow_db_span_detector.py b/tests/sentry/utils/performance_issues/test_slow_db_span_detector.py index ce665b5f23dde8..6cb4d274c61c61 100644 --- a/tests/sentry/utils/performance_issues/test_slow_db_span_detector.py +++ b/tests/sentry/utils/performance_issues/test_slow_db_span_detector.py @@ -34,7 +34,7 @@ def find_problems(self, event: dict[str, Any]) -> list[PerformanceProblem]: return list(detector.stored_problems.values()) def test_calls_detect_slow_span(self): - no_slow_span_event = create_event([create_span("db", 999.0)] * 1) + no_slow_span_event = create_event([create_span("db", 899.0)] * 1) slow_not_allowed_op_span_event = create_event([create_span("random", 1001.0, "example")]) slow_span_event = create_event([create_span("db", 1001.0)] * 1)