fix(celery): Don't round down beat schedule intervals

sentry_sdk/crons/utils.py

@@ -0,0 +1,29 @@
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from typing import Optional, Tuple
+
+    from sentry_sdk._types import MonitorConfigSchedule, MonitorConfigScheduleUnit
+
+
+_INTERVAL_UNITS: "Tuple[Tuple[MonitorConfigScheduleUnit, int], ...]" = (
+    ("day", 60 * 60 * 24),
+    ("hour", 60 * 60),
+    ("minute", 60),
+)
+
+
+def _get_interval_schedule(seconds: float) -> "Optional[MonitorConfigSchedule]":
+    """
+    Express an interval in the largest unit that divides it evenly.
+
+    Returns None if the interval isn't a whole number of minutes.
+    """
+    for unit, unit_seconds in _INTERVAL_UNITS:
+        if seconds >= unit_seconds and seconds % unit_seconds == 0:
+            return {
+                "type": "interval",
+                "value": int(seconds // unit_seconds),
+                "unit": unit,
+            }
+    return None

sentry_sdk/integrations/celery/beat.py

@@ -2,11 +2,9 @@
 
 import sentry_sdk
 from sentry_sdk.crons import MonitorStatus, capture_checkin
+from sentry_sdk.crons.utils import _get_interval_schedule
 from sentry_sdk.integrations import DidNotEnable
-from sentry_sdk.integrations.celery.utils import (
-    _get_humanized_interval,
-    _now_seconds_since_epoch,
-)
+from sentry_sdk.integrations.celery.utils import _now_seconds_since_epoch
 from sentry_sdk.utils import (
     logger,
     match_regex_list,
@@ -74,19 +72,28 @@ def _get_monitor_config(
             "{0._orig_day_of_week}".format(celery_schedule)
         )
     elif isinstance(celery_schedule, schedule):
-        schedule_type = "interval"
-        (schedule_value, schedule_unit) = _get_humanized_interval(
-            celery_schedule.seconds
-        )
-
-        if schedule_unit == "second":
-            logger.warning(
-                "Intervals shorter than one minute are not supported by Sentry Crons. Monitor '%s' has an interval of %s seconds. Use the `exclude_beat_tasks` option in the celery integration to exclude it.",
-                monitor_name,
-                schedule_value,
-            )
+        seconds = celery_schedule.seconds
+        interval_schedule = _get_interval_schedule(seconds)
+
+        if interval_schedule is None:
+            if seconds < 60:
+                logger.warning(
+                    "Intervals shorter than one minute are not supported by Sentry Crons. Monitor '%s' has an interval of %s seconds. Use the `exclude_beat_tasks` option in the celery integration to exclude it.",
+                    monitor_name,
+                    int(seconds),
+                )
+            else:
+                logger.warning(
+                    "Sentry Crons only supports intervals of whole minutes. Monitor '%s' has an interval of %s seconds. Use the `exclude_beat_tasks` option in the celery integration to exclude it.",
+                    monitor_name,
+                    int(seconds),
+                )
             return {}
 
+        schedule_type = "interval"
+        schedule_value = interval_schedule["value"]
+        schedule_unit = interval_schedule["unit"]
+
     else:
         logger.warning(
             "Celery schedule type '%s' not supported by Sentry Crons.",

sentry_sdk/integrations/celery/utils.py

@@ -1,10 +1,4 @@
 import time
-from typing import TYPE_CHECKING, cast
-
-if TYPE_CHECKING:
-    from typing import Tuple
-
-    from sentry_sdk._types import MonitorConfigScheduleUnit
 
 
 def _now_seconds_since_epoch() -> float:
@@ -14,19 +8,3 @@ def _now_seconds_since_epoch() -> float:
     # Start happens in the Celery Beat process,
     # the end in a Celery Worker process.
     return time.time()
-
-
-def _get_humanized_interval(seconds: float) -> "Tuple[int, MonitorConfigScheduleUnit]":
-    TIME_UNITS = (  # noqa: N806
-        ("day", 60 * 60 * 24.0),
-        ("hour", 60 * 60.0),
-        ("minute", 60.0),
-    )
-
-    seconds = float(seconds)
-    for unit, divider in TIME_UNITS:
-        if seconds >= divider:
-            interval = int(seconds / divider)
-            return (interval, cast("MonitorConfigScheduleUnit", unit))
-
-    return (int(seconds), "second")

tests/integrations/celery/test_celery_beat_crons.py

@@ -15,7 +15,6 @@
     crons_task_retry,
     crons_task_success,
 )
-from sentry_sdk.integrations.celery.utils import _get_humanized_interval
 
 
 def test_get_headers():
@@ -52,25 +51,6 @@ def test_get_headers():
     assert _get_headers(fake_task) == {"bla": "blub", "tri": "blub", "bar": "baz"}
 
 
-@pytest.mark.parametrize(
-    "seconds, expected_tuple",
-    [
-        (0, (0, "second")),
-        (1, (1, "second")),
-        (0.00001, (0, "second")),
-        (59, (59, "second")),
-        (60, (1, "minute")),
-        (100, (1, "minute")),
-        (1000, (16, "minute")),
-        (10000, (2, "hour")),
-        (100000, (1, "day")),
-        (100000000, (1157, "day")),
-    ],
-)
-def test_get_humanized_interval(seconds, expected_tuple):
-    assert _get_humanized_interval(seconds) == expected_tuple
-
-
 def test_crons_task_success():
     fake_task = MagicMock()
     fake_task.request = {
@@ -339,6 +319,61 @@ def test_get_monitor_config_minutes():
     }
 
 
+@pytest.mark.parametrize(
+    "run_every, value, unit",
+    [
+        (datetime.timedelta(minutes=90), 90, "minute"),
+        (datetime.timedelta(hours=2), 2, "hour"),
+        (datetime.timedelta(hours=36), 36, "hour"),
+        (datetime.timedelta(days=1), 1, "day"),
+    ],
+)
+def test_get_monitor_config_interval_not_rounded(run_every, value, unit):
+    app = MagicMock()
+    app.timezone = "Europe/Vienna"
+
+    celery_schedule = schedule(run_every=run_every)
+
+    monitor_config = _get_monitor_config(celery_schedule, app, "foo")
+    assert monitor_config["schedule"] == {
+        "type": "interval",
+        "value": value,
+        "unit": unit,
+    }
+
+
+def test_get_monitor_config_sub_minute_interval():
+    app = MagicMock()
+    app.timezone = "Europe/Vienna"
+
+    celery_schedule = schedule(run_every=datetime.timedelta(seconds=30))
+
+    with mock.patch("sentry_sdk.integrations.logger.warning") as mock_logger_warning:
+        monitor_config = _get_monitor_config(celery_schedule, app, "foo")
+        mock_logger_warning.assert_called_with(
+            "Intervals shorter than one minute are not supported by Sentry Crons. Monitor '%s' has an interval of %s seconds. Use the `exclude_beat_tasks` option in the celery integration to exclude it.",
+            "foo",
+            30,
+        )
+        assert monitor_config == {}
+
+
+def test_get_monitor_config_interval_not_whole_minutes():
+    app = MagicMock()
+    app.timezone = "Europe/Vienna"
+
+    celery_schedule = schedule(run_every=datetime.timedelta(seconds=90))
+
+    with mock.patch("sentry_sdk.integrations.logger.warning") as mock_logger_warning:
+        monitor_config = _get_monitor_config(celery_schedule, app, "foo")
+        mock_logger_warning.assert_called_with(
+            "Sentry Crons only supports intervals of whole minutes. Monitor '%s' has an interval of %s seconds. Use the `exclude_beat_tasks` option in the celery integration to exclude it.",
+            "foo",
+            90,
+        )
+        assert monitor_config == {}
+
+
 def test_get_monitor_config_unknown():
     app = MagicMock()
     app.timezone = "Europe/Vienna"

tests/test_crons.py

@@ -5,6 +5,7 @@
 
 import sentry_sdk
 from sentry_sdk.crons import capture_checkin
+from sentry_sdk.crons.utils import _get_interval_schedule
 
 
 @sentry_sdk.monitor(monitor_slug="abc123")
@@ -482,3 +483,29 @@ async def test_contextmanager_error_async(sentry_init):
         assert fake_capture_checkin.call_args[1]["status"] == "error"
         assert fake_capture_checkin.call_args[1]["duration"]
         assert fake_capture_checkin.call_args[1]["check_in_id"]
+
+
+@pytest.mark.parametrize(
+    "seconds, expected",
+    [
+        (60, (1, "minute")),
+        (90 * 60, (90, "minute")),
+        (2 * 60 * 60, (2, "hour")),
+        (36 * 60 * 60, (36, "hour")),
+        (24 * 60 * 60, (1, "day")),
+        (7 * 24 * 60 * 60, (7, "day")),
+        (300.0, (5, "minute")),
+    ],
+)
+def test_get_interval_schedule(seconds, expected):
+    value, unit = expected
+    assert _get_interval_schedule(seconds) == {
+        "type": "interval",
+        "value": value,
+        "unit": unit,
+    }
+
+
+@pytest.mark.parametrize("seconds", [0, 0.5, 30, 59, 90, 61.5, 3601])
+def test_get_interval_schedule_not_whole_minutes(seconds):
+    assert _get_interval_schedule(seconds) is None