Repository: getsentry/sentry
Pull request: ref(escalating): Remove forecast floor bound
Remove floor bound for escalating issues algorithm.
Log exception and call forecast generation function on fetch if the forecast does not exist

fixes https://github.com/getsentry/sentry/issues/51947

Problem statement:
Remove forecast floor bound in escalating algorithm
### Problem Statement

The forecast floor bound of 200 is not a good estimation of when an issue with < 30 events/hr is escalating.

### Solution Brainstorm

Remove the forecast floor bound of 200.
Change the forecast fetch fallback to call the forecast generation algorithm and log an exception instead of returning the default floor bound.

### Product Area

Issues

Hints from issue discussion:

Commit message: fix: Test

Patch to review:
diff --git a/src/sentry/api/endpoints/group_details.py b/src/sentry/api/endpoints/group_details.py
index d19223e573dc1b..17a1d96b8a4dd4 100644
--- a/src/sentry/api/endpoints/group_details.py
+++ b/src/sentry/api/endpoints/group_details.py
@@ -222,17 +222,17 @@ def get(self, request: Request, group) -> Response:
             if "forecast" in expand and features.has(
                 "organizations:escalating-issues", group.organization
             ):
-                fetched_forecast = EscalatingGroupForecast.fetch(
-                    group.project_id, group.id
-                ).to_dict()
-                data.update(
-                    {
-                        "forecast": {
-                            "data": fetched_forecast.get("forecast"),
-                            "date_added": fetched_forecast.get("date_added"),
+                fetched_forecast = EscalatingGroupForecast.fetch(group.project_id, group.id)
+                if fetched_forecast:
+                    fetched_forecast = fetched_forecast.to_dict()
+                    data.update(
+                        {
+                            "forecast": {
+                                "data": fetched_forecast.get("forecast"),
+                                "date_added": fetched_forecast.get("date_added"),
+                            }
                         }
-                    }
-                )
+                    )
 
             action_list = self._get_actions(request, group)
             data.update(
diff --git a/src/sentry/issues/escalating.py b/src/sentry/issues/escalating.py
index d5e0b4841db4ec..860648fee4db73 100644
--- a/src/sentry/issues/escalating.py
+++ b/src/sentry/issues/escalating.py
@@ -267,7 +267,7 @@ def is_escalating(group: Group) -> Tuple[bool, Optional[int]]:
     group_hourly_count = get_group_hourly_count(group)
     forecast_today = EscalatingGroupForecast.fetch_todays_forecast(group.project.id, group.id)
     # Check if current event occurance is greater than forecast for today's date
-    if group_hourly_count > forecast_today:
+    if forecast_today and group_hourly_count > forecast_today:
         return True, forecast_today
     return False, None
 
diff --git a/src/sentry/issues/escalating_group_forecast.py b/src/sentry/issues/escalating_group_forecast.py
index 5f8fc6509c1b7b..3ff3426173b1e9 100644
--- a/src/sentry/issues/escalating_group_forecast.py
+++ b/src/sentry/issues/escalating_group_forecast.py
@@ -9,13 +9,12 @@
 import logging
 from dataclasses import dataclass
 from datetime import datetime, timedelta
-from typing import List, TypedDict, cast
+from typing import List, Optional, TypedDict, cast
 
 from sentry import nodestore
 from sentry.utils.dates import parse_timestamp
 
 GROUP_FORECAST_TTL = 14
-DEFAULT_MINIMUM_CEILING_FORECAST = [200] * 14
 
 
 class EscalatingGroupForecastData(TypedDict):
@@ -48,21 +47,29 @@ def save(self) -> None:
         )
 
     @classmethod
-    def fetch(cls, project_id: int, group_id: int) -> EscalatingGroupForecast:
+    def fetch(cls, project_id: int, group_id: int) -> Optional[EscalatingGroupForecast]:
+        from sentry.issues.forecasts import generate_and_save_missing_forecasts
+
         results = nodestore.get(cls.build_storage_identifier(project_id, group_id))
         if results:
             return EscalatingGroupForecast.from_dict(results)
-        return EscalatingGroupForecast(
-            project_id=project_id,
-            group_id=group_id,
-            forecast=DEFAULT_MINIMUM_CEILING_FORECAST,
-            date_added=datetime.now(),
+        logger.exception(
+            "Forecast does not exist for project id: "
+            + str(project_id)
+            + " group id: "
+            + str(group_id)
         )
+        generate_and_save_missing_forecasts.delay(group_id=group_id)
+        return None
 
     @classmethod
-    def fetch_todays_forecast(cls, project_id: int, group_id: int) -> int:
+    def fetch_todays_forecast(cls, project_id: int, group_id: int) -> Optional[int]:
         date_now = datetime.now().date()
         escalating_forecast = EscalatingGroupForecast.fetch(project_id, group_id)
+
+        if not escalating_forecast:
+            return None
+
         forecast_today_index = (date_now - escalating_forecast.date_added.date()).days
         if forecast_today_index >= len(escalating_forecast.forecast):
             logger.error("Forecast list index is out of range")
diff --git a/src/sentry/issues/escalating_issues_alg.py b/src/sentry/issues/escalating_issues_alg.py
index ed55ff643ca989..f08873d4c1f58e 100644
--- a/src/sentry/issues/escalating_issues_alg.py
+++ b/src/sentry/issues/escalating_issues_alg.py
@@ -78,18 +78,6 @@ def generate_issue_forecast(
 
     ts_max = max(ts_data)
 
-    # minimum ceiling for issues that don't have much data from the past week
-    floor_bound = 200
-
-    # if the max number of issues in an hour bucket is 30,
-    # a floor bound is used as the issue ceiling for the next week
-    if ts_max < 30:
-        for output_ts in output_dates:
-            output.append(
-                {"forecasted_date": output_ts.strftime("%Y-%m-%d"), "forecasted_value": floor_bound}
-            )
-        return output
-
     # if we have less than a week's worth of data (new issue),
     # set the threshold to 10x the max of the dataset to account for
     # how the pattern of the issue will change over the first week
diff --git a/src/sentry/issues/forecasts.py b/src/sentry/issues/forecasts.py
index 13139afc94c89f..90b46ae90c84d9 100644
--- a/src/sentry/issues/forecasts.py
+++ b/src/sentry/issues/forecasts.py
@@ -19,6 +19,7 @@
     standard_version,
 )
 from sentry.models import Group
+from sentry.tasks.base import instrumented_task
 
 logger = logging.getLogger(__name__)
 
@@ -71,3 +72,16 @@ def generate_and_save_forecasts(groups: Sequence[Group]) -> None:
     )
     group_counts = parse_groups_past_counts(past_counts)
     save_forecast_per_group(groups, group_counts)
+
+
+@instrumented_task(
+    name="sentry.tasks.weekly_escalating_forecast.generate_and_save_missing_forecasts",
+    queue="weekly_escalating_forecast",
+)
+def generate_and_save_missing_forecasts(group_id: int) -> None:
+    """
+    Runs generate_and_save_forecasts in a task as a fallback if the forecast does not exist.
+    This should not happen, but exists as a fallback.
+    """
+    group = Group.objects.filter(id=group_id)
+    generate_and_save_forecasts(group)
diff --git a/tests/sentry/issues/test_escalating_issues_alg.py b/tests/sentry/issues/test_escalating_issues_alg.py
index 87f4df8c3560ab..4d9e3ad5083441 100644
--- a/tests/sentry/issues/test_escalating_issues_alg.py
+++ b/tests/sentry/issues/test_escalating_issues_alg.py
@@ -671,7 +671,7 @@ def test_low_freq_events() -> None:
     ceilings_list = generate_issue_forecast(data, START_TIME)
     ceilings = [x["forecasted_value"] for x in ceilings_list]
 
-    assert ceilings == [200] * 14, "Ceilings are incorrect"
+    assert ceilings == [36] * 14, "Ceilings are incorrect"
 
 
 def test_output() -> None:
diff --git a/tests/sentry/issues/test_ignored.py b/tests/sentry/issues/test_ignored.py
index 9f3260ce020a1a..81864bfa2dbbf6 100644
--- a/tests/sentry/issues/test_ignored.py
+++ b/tests/sentry/issues/test_ignored.py
@@ -1,9 +1,6 @@
 from unittest.mock import MagicMock, patch
 
-from sentry.issues.escalating_group_forecast import (
-    DEFAULT_MINIMUM_CEILING_FORECAST,
-    EscalatingGroupForecast,
-)
+from sentry.issues.escalating_group_forecast import EscalatingGroupForecast
 from sentry.issues.ignored import handle_archived_until_escalating, handle_ignored
 from sentry.models import (
     Group,
@@ -66,8 +63,13 @@ def test_ignored_user_count(self) -> None:
 @apply_feature_flag_on_cls("organizations:escalating-issues")
 class HandleArchiveUntilEscalating(TestCase):
     @patch("sentry.issues.forecasts.query_groups_past_counts", return_value={})
+    @patch("sentry.issues.forecasts.generate_and_save_missing_forecasts.delay")
+    @patch("sentry.issues.escalating_group_forecast.logger")
     def test_archive_until_escalating_no_counts(
-        self, mock_query_groups_past_counts: MagicMock
+        self,
+        mock_logger: MagicMock,
+        mock_generate_and_save_missing_forecasts: MagicMock,
+        mock_query_groups_past_counts: MagicMock,
     ) -> None:
         self.group = self.create_group()
         add_group_to_inbox(self.group, GroupInboxReason.NEW)
@@ -84,10 +86,14 @@ def test_archive_until_escalating_no_counts(
         assert status_details == {"ignoreUntilEscalating": True}
 
         fetched_forecast = EscalatingGroupForecast.fetch(self.group.project.id, self.group.id)
-        assert fetched_forecast is not None
-        assert fetched_forecast.project_id == self.group.project.id
-        assert fetched_forecast.group_id == self.group.id
-        assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
+        assert fetched_forecast is None
+        assert mock_logger.exception.call_args.args[0] == (
+            "Forecast does not exist for project id: "
+            + str(self.group.project.id)
+            + " group id: "
+            + str(self.group.id)
+        )
+        assert mock_generate_and_save_missing_forecasts.call_count == 1
 
     @patch("sentry.issues.forecasts.query_groups_past_counts")
     def test_archive_until_escalating_with_counts(
@@ -116,7 +122,7 @@ def test_archive_until_escalating_with_counts(
         assert fetched_forecast is not None
         assert fetched_forecast.project_id == self.group.project.id
         assert fetched_forecast.group_id == self.group.id
-        assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
+        assert fetched_forecast.forecast == [100] * 14
 
     @patch("sentry.issues.forecasts.query_groups_past_counts", return_value={})
     @patch("sentry.signals.issue_archived.send_robust")
diff --git a/tests/sentry/issues/test_utils.py b/tests/sentry/issues/test_utils.py
index 6378ef62338a3f..24d0162720a828 100644
--- a/tests/sentry/issues/test_utils.py
+++ b/tests/sentry/issues/test_utils.py
@@ -1,4 +1,3 @@
-import random
 import uuid
 from dataclasses import replace
 from datetime import datetime, timedelta
@@ -169,7 +168,7 @@ def get_mock_groups_past_counts_response(
                         {
                             "group_id": group.id,
                             "hourBucket": hourly_time.strftime("%Y-%m-%dT%H:%M:%S%f") + "+00:00",
-                            "count()": random.randint(1, 10),
+                            "count()": 10,
                             "project_id": group.project.id,
                         }
                     )
diff --git a/tests/sentry/tasks/test_weekly_escalating_forecast.py b/tests/sentry/tasks/test_weekly_escalating_forecast.py
index 721c59e339b6c8..8b9f0cd18b19f0 100644
--- a/tests/sentry/tasks/test_weekly_escalating_forecast.py
+++ b/tests/sentry/tasks/test_weekly_escalating_forecast.py
@@ -4,10 +4,7 @@
 
 import pytz
 
-from sentry.issues.escalating_group_forecast import (
-    DEFAULT_MINIMUM_CEILING_FORECAST,
-    EscalatingGroupForecast,
-)
+from sentry.issues.escalating_group_forecast import EscalatingGroupForecast
 from sentry.models.group import Group, GroupStatus
 from sentry.tasks.weekly_escalating_forecast import run_escalating_forecast
 from sentry.testutils.cases import APITestCase, SnubaTestCase
@@ -27,10 +24,14 @@ def create_archived_until_escalating_groups(self, num_groups: int) -> List[Group
             group_list.append(group)
         return group_list
 
-    @patch("sentry.analytics.record")
+    @patch("sentry.issues.forecasts.generate_and_save_missing_forecasts.delay")
+    @patch("sentry.issues.escalating_group_forecast.logger")
     @patch("sentry.issues.escalating.query_groups_past_counts")
     def test_empty_escalating_forecast(
-        self, mock_query_groups_past_counts: MagicMock, record_mock: MagicMock
+        self,
+        mock_query_groups_past_counts: MagicMock,
+        mock_logger: MagicMock,
+        mock_generate_and_save_missing_forecasts: MagicMock,
     ) -> None:
         with self.tasks():
             group_list = self.create_archived_until_escalating_groups(num_groups=1)
@@ -38,14 +39,16 @@ def test_empty_escalating_forecast(
             mock_query_groups_past_counts.return_value = {}
 
             run_escalating_forecast()
-            fetched_forecast = EscalatingGroupForecast.fetch(
-                group_list[0].project.id, group_list[0].id
+            group = group_list[0]
+            fetched_forecast = EscalatingGroupForecast.fetch(group.project.id, group.id)
+            assert fetched_forecast is None
+            assert mock_logger.exception.call_args.args[0] == (
+                "Forecast does not exist for project id: "
+                + str(group.project.id)
+                + " group id: "
+                + str(group.id)
             )
-            assert fetched_forecast is not None
-            assert fetched_forecast.project_id == group_list[0].project.id
-            assert fetched_forecast.group_id == group_list[0].id
-            assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
-            record_mock.assert_called_with("issue_forecasts.saved", num_groups=0)
+        assert mock_generate_and_save_missing_forecasts.call_count == 1
 
     @patch("sentry.analytics.record")
     @patch("sentry.issues.forecasts.query_groups_past_counts")
@@ -67,7 +70,7 @@ def test_single_group_escalating_forecast(
             assert fetched_forecast is not None
             assert fetched_forecast.project_id == group_list[0].project.id
             assert fetched_forecast.group_id == group_list[0].id
-            assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
+            assert fetched_forecast.forecast == [100] * 14
             assert fetched_forecast.date_added.replace(
                 second=0, microsecond=0
             ) == approximate_date_added.replace(second=0, microsecond=0)
@@ -95,7 +98,7 @@ def test_multiple_groups_escalating_forecast(
                 assert fetched_forecast is not None
                 assert fetched_forecast.project_id == group_list[i].project.id
                 assert fetched_forecast.group_id == group_list[i].id
-                assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
+                assert fetched_forecast.forecast == [100] * 14
                 assert fetched_forecast.date_added.replace(
                     second=0, microsecond=0
                 ) == approximate_date_added.replace(second=0, microsecond=0)
diff --git a/tests/snuba/api/endpoints/test_group_details.py b/tests/snuba/api/endpoints/test_group_details.py
index b00e110e194b4e..f54bfecbd9f310 100644
--- a/tests/snuba/api/endpoints/test_group_details.py
+++ b/tests/snuba/api/endpoints/test_group_details.py
@@ -2,6 +2,7 @@
 
 from rest_framework.exceptions import ErrorDetail
 
+from sentry.issues.forecasts import generate_and_save_forecasts
 from sentry.models import (
     GROUP_OWNER_TYPE,
     Environment,
@@ -201,6 +202,8 @@ def test_group_expand_forecasts(self):
                 project_id=self.project.id,
             )
             group = event.group
+            generate_and_save_forecasts([group])
+
             url = f"/api/0/issues/{group.id}/?expand=forecast"
 
             response = self.client.get(url, format="json")


Code review comments:
diff_hunk: @@ -48,21 +47,29 @@ def save(self) -> None:
         )
 
     @classmethod
-    def fetch(cls, project_id: int, group_id: int) -> EscalatingGroupForecast:
+    def fetch(cls, project_id: int, group_id: int) -> Optional[EscalatingGroupForecast]:
+        from sentry.issues.forecasts import generate_and_save_missing_forecasts
+
         results = nodestore.get(cls.build_storage_identifier(project_id, group_id))
         if results:
             return EscalatingGroupForecast.from_dict(results)
-        return EscalatingGroupForecast(
-            project_id=project_id,
-            group_id=group_id,
-            forecast=DEFAULT_MINIMUM_CEILING_FORECAST,
-            date_added=datetime.now(),
+        logger.exception(
+            "Forecast does not exist for project id: "
+            + str(project_id)
+            + " group id: "
+            + str(group_id)
path: src/sentry/issues/escalating_group_forecast.py
text: @user1:
```suggestion
        logger.exception(
            f"Forecast does not exist for project id: {str(project_id)} group id: {str(group_id)}"
        )
```

diff_hunk: @@ -84,10 +86,14 @@ def test_archive_until_escalating_no_counts(
         assert status_details == {"ignoreUntilEscalating": True}
 
         fetched_forecast = EscalatingGroupForecast.fetch(self.group.project.id, self.group.id)
-        assert fetched_forecast is not None
-        assert fetched_forecast.project_id == self.group.project.id
-        assert fetched_forecast.group_id == self.group.id
-        assert fetched_forecast.forecast == DEFAULT_MINIMUM_CEILING_FORECAST
+        assert fetched_forecast is None
+        assert mock_logger.exception.call_args.args[0] == (
+            "Forecast does not exist for project id: "
+            + str(self.group.project.id)
+            + " group id: "
+            + str(self.group.id)
path: tests/sentry/issues/test_ignored.py
text: @user1:
same as above, f-strings are a bit easier to read IMO