From 54ffb37d7b1c73c86e264d7e6460e026cbacb4ea Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 18 Sep 2023 18:57:16 -0700 Subject: [PATCH 1/3] stop excluding scores equal to 0 --- src/sentry/event_manager.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/sentry/event_manager.py b/src/sentry/event_manager.py index 43e5de23fccf61..f163f180e5cac6 100644 --- a/src/sentry/event_manager.py +++ b/src/sentry/event_manager.py @@ -1624,9 +1624,10 @@ def _save_aggregate( group = _create_group(project, event, **kwargs) - if features.has( - "projects:first-event-severity-calculation", event.project - ) and not group.data.get("metadata", {}).get("severity"): + if ( + features.has("projects:first-event-severity-calculation", event.project) + and group.data.get("metadata", {}).get("severity") is None + ): logger.error( "Group created without severity score", extra={ @@ -1843,7 +1844,7 @@ def _create_group(project: Project, event: Event, **kwargs: Any) -> Group: group_data = kwargs.pop("data", {}) if features.has("projects:first-event-severity-calculation", event.project): severity = _get_severity_score(event) - if severity: + if severity is not None: # Severity can be 0 group_data.setdefault("metadata", {}) group_data["metadata"]["severity"] = severity @@ -2510,9 +2511,10 @@ def _save_grouphash_and_group( group = _create_group(project, event, **group_kwargs) group_hash.update(group=group) - if features.has( - "projects:first-event-severity-calculation", event.project - ) and not group.data.get("metadata", {}).get("severity"): + if ( + features.has("projects:first-event-severity-calculation", event.project) + and group.data.get("metadata", {}).get("severity") is None + ): logger.error( "Group created without severity score", extra={ From 97d9c4221bb4484d2ffdbeb1d1a509a7634a89e0 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 18 Sep 2023 18:57:16 -0700 Subject: [PATCH 2/3] make feature disabled test more precise --- tests/sentry/event_manager/test_event_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/event_manager/test_event_manager.py b/tests/sentry/event_manager/test_event_manager.py index 3009b14d9a3821..53106f8288457c 100644 --- a/tests/sentry/event_manager/test_event_manager.py +++ b/tests/sentry/event_manager/test_event_manager.py @@ -2639,7 +2639,7 @@ def test_severity_score_flag_off(self, mock_get_severity_score: MagicMock): event = manager.save(self.project.id) mock_get_severity_score.assert_not_called() - assert event.group and event.group.get_event_metadata().get("severity") is None + assert event.group and "severity" not in event.group.get_event_metadata() class AutoAssociateCommitTest(TestCase, EventManagerTestMixin): From e2dde11ec8b7a9db3816ac6f3b3d1388038486a6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 18 Sep 2023 18:57:16 -0700 Subject: [PATCH 3/3] add tests --- .../event_manager/test_event_manager.py | 78 ++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/tests/sentry/event_manager/test_event_manager.py b/tests/sentry/event_manager/test_event_manager.py index 53106f8288457c..d03c9298711dc6 100644 --- a/tests/sentry/event_manager/test_event_manager.py +++ b/tests/sentry/event_manager/test_event_manager.py @@ -2641,6 +2641,36 @@ def test_severity_score_flag_off(self, mock_get_severity_score: MagicMock): mock_get_severity_score.assert_not_called() assert event.group and "severity" not in event.group.get_event_metadata() + @patch("sentry.event_manager._get_severity_score", return_value=None) + def test_no_severity_score_assigned_when_value_is_None( + self, mock_get_severity_score: MagicMock + ): + with self.feature({"projects:first-event-severity-calculation": True}): + manager = EventManager( + make_event( + exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]} + ) + ) + event = manager.save(self.project.id) + + mock_get_severity_score.assert_called() + assert event.group and "severity" not in event.group.get_event_metadata() + + @patch("sentry.event_manager._get_severity_score", return_value=0) + def test_severity_score_still_assigned_when_value_is_zero( + self, mock_get_severity_score: MagicMock + ): + with self.feature({"projects:first-event-severity-calculation": True}): + manager = EventManager( + make_event( + exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]} + ) + ) + event = manager.save(self.project.id) + + mock_get_severity_score.assert_called() + assert event.group and event.group.get_event_metadata().get("severity") == 0 + class AutoAssociateCommitTest(TestCase, EventManagerTestMixin): def setUp(self): @@ -3717,6 +3747,29 @@ def test_logs_error_on_no_severity_score_when_enabled( }, ) + @patch("sentry.event_manager.logger.error") + @patch("sentry.event_manager._get_severity_score", return_value=0) + def test_no_error_logged_on_zero_severity_score_when_enabled( + self, + mock_get_severity_score: MagicMock, + mock_logger_error: MagicMock, + ): + with self.feature({"projects:first-event-severity-calculation": True}): + event = _get_event_instance( + make_event( + exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]} + ), + self.project.id, + ) + + group, _ = _save_grouphash_and_group(self.project, event, "dogs are great") + + logger_messages = [call.args[0] for call in mock_logger_error.mock_calls] + + mock_get_severity_score.assert_called() + assert group.data["metadata"]["severity"] == 0 + assert "Group created without severity score" not in logger_messages + @patch("sentry.event_manager.logger.error") @patch("sentry.event_manager._get_severity_score", return_value=None) def test_no_error_logged_on_no_severity_score_when_disabled( @@ -3768,6 +3821,30 @@ def test_logs_error_on_no_severity_score_when_enabled( }, ) + @patch("sentry.event_manager._save_aggregate", wraps=_save_aggregate) + @patch("sentry.event_manager.logger.error") + @patch("sentry.event_manager._get_severity_score", return_value=0) + def test_no_error_logged_on_zero_severity_score_when_enabled( + self, + mock_get_severity_score: MagicMock, + mock_logger_error: MagicMock, + mock_save_aggregate: MagicMock, + ): + with self.feature({"projects:first-event-severity-calculation": True}): + manager = EventManager( + make_event( + exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]} + ) + ) + event = manager.save(self.project.id) + + logger_messages = [call.args[0] for call in mock_logger_error.mock_calls] + + mock_save_aggregate.assert_called() + mock_get_severity_score.assert_called() + assert event.group and event.group.data["metadata"]["severity"] == 0 + assert "Group created without severity score" not in logger_messages + @patch("sentry.event_manager._save_aggregate", wraps=_save_aggregate) @patch("sentry.event_manager.logger.error") @patch("sentry.event_manager._get_severity_score", return_value=None) @@ -3789,5 +3866,4 @@ def test_no_error_logged_on_no_severity_score_when_disabled( mock_save_aggregate.assert_called() mock_get_severity_score.assert_not_called() - assert "Group created without severity score" not in logger_messages