From f7ac8049359c5d0f39c9f13d5dbf3ec8fe056b47 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 23 Jul 2024 11:51:21 -0700 Subject: [PATCH] update tests --- .../grouping/test_seer_grouping.py | 18 ----- tests/sentry/grouping/ingest/test_seer.py | 12 +++ .../seer/similarity/test_similar_issues.py | 73 +++++++++++++++---- 3 files changed, 70 insertions(+), 33 deletions(-) diff --git a/tests/sentry/event_manager/grouping/test_seer_grouping.py b/tests/sentry/event_manager/grouping/test_seer_grouping.py index bd9212e51ad250..aefe287aa83dff 100644 --- a/tests/sentry/event_manager/grouping/test_seer_grouping.py +++ b/tests/sentry/event_manager/grouping/test_seer_grouping.py @@ -10,7 +10,6 @@ from sentry.testutils.helpers.eventprocessing import save_new_event from sentry.testutils.helpers.features import with_feature from sentry.testutils.pytest.mocking import capture_results -from sentry.utils.circuit_breaker import with_circuit_breaker from sentry.utils.types import NonNone @@ -152,23 +151,6 @@ def test_obeys_seer_similarity_flags(self): assert get_seer_similar_issues_return_values[0][1] == existing_event.group assert new_event.group_id == existing_event.group_id - @patch("sentry.event_manager.should_call_seer_for_grouping", return_value=True) - @patch("sentry.event_manager.with_circuit_breaker", wraps=with_circuit_breaker) - @patch("sentry.event_manager.get_seer_similar_issues", return_value=({}, None)) - def test_obeys_circult_breaker( - self, mock_get_seer_similar_issues: MagicMock, mock_with_circuit_breaker: MagicMock, _ - ): - with patch("sentry.utils.circuit_breaker._should_call_callback", return_value=True): - save_new_event({"message": "Dogs are great!"}, self.project) - assert mock_with_circuit_breaker.call_count == 1 - assert mock_get_seer_similar_issues.call_count == 1 - - with patch("sentry.utils.circuit_breaker._should_call_callback", return_value=False): - save_new_event({"message": "Adopt don't shop"}, self.project) - - assert mock_with_circuit_breaker.call_count == 2 # increased - assert mock_get_seer_similar_issues.call_count == 1 # didn't increase - @patch("sentry.event_manager.should_call_seer_for_grouping", return_value=True) @patch("sentry.event_manager.get_seer_similar_issues", return_value=({}, None)) def test_calls_seer_if_no_group_found(self, mock_get_seer_similar_issues: MagicMock, _): diff --git a/tests/sentry/grouping/ingest/test_seer.py b/tests/sentry/grouping/ingest/test_seer.py index 945ed67422d272..90da60da248c7a 100644 --- a/tests/sentry/grouping/ingest/test_seer.py +++ b/tests/sentry/grouping/ingest/test_seer.py @@ -137,6 +137,18 @@ def test_obeys_project_ratelimit(self): is expected_result ) + @with_feature("projects:similarity-embeddings-grouping") + def test_obeys_circuit_breaker(self): + for request_allowed, expected_result in [(True, True), (False, False)]: + with patch( + "sentry.grouping.ingest.seer.seer_similarity_circuit_breaker.should_allow_request", + return_value=request_allowed, + ): + assert ( + should_call_seer_for_grouping(self.event, self.primary_hashes) + is expected_result + ) + @with_feature("projects:similarity-embeddings-grouping") def test_obeys_customized_fingerprint_check(self): default_fingerprint_event = Event( diff --git a/tests/sentry/seer/similarity/test_similar_issues.py b/tests/sentry/seer/similarity/test_similar_issues.py index 5efd172f635fde..72fe03bdf74815 100644 --- a/tests/sentry/seer/similarity/test_similar_issues.py +++ b/tests/sentry/seer/similarity/test_similar_issues.py @@ -96,9 +96,17 @@ def test_no_groups_found(self, mock_seer_request: MagicMock, mock_metrics_incr: tags={"response_status": 200, "outcome": "no_similar_groups"}, ) + @mock.patch( + "sentry.seer.similarity.similar_issues.seer_similarity_circuit_breaker.record_error" + ) @mock.patch("sentry.seer.similarity.similar_issues.metrics.incr") @mock.patch("sentry.seer.similarity.similar_issues.seer_grouping_connection_pool.urlopen") - def test_bad_response_data(self, mock_seer_request: MagicMock, mock_metrics_incr: MagicMock): + def test_bad_response_data( + self, + mock_seer_request: MagicMock, + mock_metrics_incr: MagicMock, + mock_record_circuit_breaker_error: MagicMock, + ): cases: list[tuple[Any, str]] = [ (None, "AttributeError"), ([], "AttributeError"), @@ -139,14 +147,22 @@ def test_bad_response_data(self, mock_seer_request: MagicMock, mock_metrics_incr sample_rate=SIMILARITY_REQUEST_METRIC_SAMPLE_RATE, tags={"response_status": 200, "outcome": "error", "error": expected_error}, ) + assert mock_record_circuit_breaker_error.call_count == 0 mock_metrics_incr.reset_mock() + @mock.patch( + "sentry.seer.similarity.similar_issues.seer_similarity_circuit_breaker.record_error" + ) @mock.patch("sentry.seer.similarity.similar_issues.metrics.incr") @mock.patch("sentry.seer.similarity.similar_issues.logger") @mock.patch("sentry.seer.similarity.similar_issues.seer_grouping_connection_pool.urlopen") def test_redirect( - self, mock_seer_request: MagicMock, mock_logger: MagicMock, mock_metrics_incr: MagicMock + self, + mock_seer_request: MagicMock, + mock_logger: MagicMock, + mock_metrics_incr: MagicMock, + mock_record_circuit_breaker_error: MagicMock, ): mock_seer_request.return_value = HTTPResponse( status=308, headers={"location": "/new/and/improved/endpoint/"} @@ -161,12 +177,20 @@ def test_redirect( sample_rate=SIMILARITY_REQUEST_METRIC_SAMPLE_RATE, tags={"response_status": 308, "outcome": "error", "error": "Redirect"}, ) + assert mock_record_circuit_breaker_error.call_count == 0 + @mock.patch( + "sentry.seer.similarity.similar_issues.seer_similarity_circuit_breaker.record_error" + ) @mock.patch("sentry.seer.similarity.similar_issues.metrics.incr") @mock.patch("sentry.seer.similarity.similar_issues.logger") @mock.patch("sentry.seer.similarity.similar_issues.seer_grouping_connection_pool.urlopen") def test_request_error( - self, mock_seer_request: MagicMock, mock_logger: MagicMock, mock_metrics_incr: MagicMock + self, + mock_seer_request: MagicMock, + mock_logger: MagicMock, + mock_metrics_incr: MagicMock, + mock_record_circuit_breaker_error: MagicMock, ): for request_error, expected_error_tag in [ (TimeoutError, "TimeoutError"), @@ -192,25 +216,44 @@ def test_request_error( sample_rate=SIMILARITY_REQUEST_METRIC_SAMPLE_RATE, tags={"outcome": "error", "error": expected_error_tag}, ) + assert mock_record_circuit_breaker_error.call_count == 1 + + mock_logger.warning.reset_mock() + mock_metrics_incr.reset_mock() + mock_record_circuit_breaker_error.reset_mock() + @mock.patch( + "sentry.seer.similarity.similar_issues.seer_similarity_circuit_breaker.record_error" + ) @mock.patch("sentry.seer.similarity.similar_issues.metrics.incr") @mock.patch("sentry.seer.similarity.similar_issues.logger") @mock.patch("sentry.seer.similarity.similar_issues.seer_grouping_connection_pool.urlopen") def test_error_status( - self, mock_seer_request: MagicMock, mock_logger: MagicMock, mock_metrics_incr: MagicMock + self, + mock_seer_request: MagicMock, + mock_logger: MagicMock, + mock_metrics_incr: MagicMock, + mock_record_circuit_breaker_error: MagicMock, ): - mock_seer_request.return_value = HTTPResponse("No soup for you", status=403) + for response, status, counts_for_circuit_breaker in [ + ("No soup for you", 403, False), + ("No soup, period", 500, True), + ]: + mock_seer_request.return_value = HTTPResponse(response, status=status) - assert get_similarity_data_from_seer(self.request_params) == [] - mock_logger.error.assert_called_with( - f"Received 403 when calling Seer endpoint {SEER_SIMILAR_ISSUES_URL}.", - extra={"response_data": "No soup for you"}, - ) - mock_metrics_incr.assert_any_call( - "seer.similar_issues_request", - sample_rate=SIMILARITY_REQUEST_METRIC_SAMPLE_RATE, - tags={"response_status": 403, "outcome": "error", "error": "RequestError"}, - ) + assert get_similarity_data_from_seer(self.request_params) == [] + mock_logger.error.assert_called_with( + f"Received {status} when calling Seer endpoint {SEER_SIMILAR_ISSUES_URL}.", + extra={"response_data": response}, + ) + mock_metrics_incr.assert_any_call( + "seer.similar_issues_request", + sample_rate=SIMILARITY_REQUEST_METRIC_SAMPLE_RATE, + tags={"response_status": status, "outcome": "error", "error": "RequestError"}, + ) + assert mock_record_circuit_breaker_error.call_count == ( + 1 if counts_for_circuit_breaker else 0 + ) @mock.patch("sentry.seer.similarity.similar_issues.seer_grouping_connection_pool.urlopen") def test_returns_sorted_results(self, mock_seer_request: MagicMock):