Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not support sub-minute cron intervals #2172

Merged
merged 3 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 27 additions & 18 deletions sentry_sdk/integrations/celery.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ def _get_humanized_interval(seconds):
interval = int(seconds / divider)
return (interval, unit)

return (1, "minute")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we just set everything that is under one minute to exactly one minute. Now We return the correct second interval.

return (int(seconds), "second")


def _get_monitor_config(celery_schedule, app):
Expand All @@ -400,6 +400,12 @@ def _get_monitor_config(celery_schedule, app):
celery_schedule.seconds
)

if schedule_unit == "second":
logger.warning(
"Intervals shorter than one minute are not supported by Sentry Crons."
)
return {}
Comment on lines +403 to +407
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when a second interval is returned, we print a warning and do not return a monitor config (this is the behavior the backend guys requested, because then they ignore the checkins)


else:
logger.warning(
"Celery schedule type '%s' not supported by Sentry Crons.",
Expand Down Expand Up @@ -441,24 +447,27 @@ def sentry_apply_entry(*args, **kwargs):

monitor_config = _get_monitor_config(celery_schedule, app)

headers = schedule_entry.options.pop("headers", {})
headers.update(
{
"sentry-monitor-slug": monitor_name,
"sentry-monitor-config": monitor_config,
}
)

check_in_id = capture_checkin(
monitor_slug=monitor_name,
monitor_config=monitor_config,
status=MonitorStatus.IN_PROGRESS,
)
headers.update({"sentry-monitor-check-in-id": check_in_id})
is_supported_schedule = bool(monitor_config)
if is_supported_schedule:
Comment on lines +450 to +451
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now only create the "in_progress" checkin for supported schedule types (so if the schedule is sub-minute interval, the check in will not be sent)

Also because the headers are not set in case it is an unsupported type, the ok/failure checkins will also not be sent.

headers = schedule_entry.options.pop("headers", {})
headers.update(
{
"sentry-monitor-slug": monitor_name,
"sentry-monitor-config": monitor_config,
}
)

check_in_id = capture_checkin(
monitor_slug=monitor_name,
monitor_config=monitor_config,
status=MonitorStatus.IN_PROGRESS,
)
headers.update({"sentry-monitor-check-in-id": check_in_id})

# Set the Sentry configuration in the options of the ScheduleEntry.
# Those will be picked up in `apply_async` and added to the headers.
schedule_entry.options["headers"] = headers

# Set the Sentry configuration in the options of the ScheduleEntry.
# Those will be picked up in `apply_async` and added to the headers.
schedule_entry.options["headers"] = headers
return original_apply_entry(*args, **kwargs)

Scheduler.apply_entry = sentry_apply_entry
Expand Down
34 changes: 28 additions & 6 deletions tests/integrations/celery/test_celery_beat_crons.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,11 @@ def test_get_headers():
@pytest.mark.parametrize(
"seconds, expected_tuple",
[
(0, (1, "minute")),
(0.00001, (1, "minute")),
(1, (1, "minute")),
(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")),
Expand Down Expand Up @@ -205,13 +207,12 @@ def test_crons_task_retry():
)


def test_get_monitor_config():
def test_get_monitor_config_crontab():
app = MagicMock()
app.conf = MagicMock()
app.conf.timezone = "Europe/Vienna"

celery_schedule = crontab(day_of_month="3", hour="12", minute="*/10")

monitor_config = _get_monitor_config(celery_schedule, app)
assert monitor_config == {
"schedule": {
Expand All @@ -222,8 +223,23 @@ def test_get_monitor_config():
}
assert "unit" not in monitor_config["schedule"]

celery_schedule = schedule(run_every=3)

def test_get_monitor_config_seconds():
app = MagicMock()
app.conf = MagicMock()
app.conf.timezone = "Europe/Vienna"

celery_schedule = schedule(run_every=3) # seconds
monitor_config = _get_monitor_config(celery_schedule, app)
assert monitor_config == {}


def test_get_monitor_config_minutes():
app = MagicMock()
app.conf = MagicMock()
app.conf.timezone = "Europe/Vienna"

celery_schedule = schedule(run_every=60) # seconds
monitor_config = _get_monitor_config(celery_schedule, app)
assert monitor_config == {
"schedule": {
Expand All @@ -234,6 +250,12 @@ def test_get_monitor_config():
"timezone": "Europe/Vienna",
}


def test_get_monitor_config_unknown():
app = MagicMock()
app.conf = MagicMock()
app.conf.timezone = "Europe/Vienna"

unknown_celery_schedule = MagicMock()
monitor_config = _get_monitor_config(unknown_celery_schedule, app)
assert monitor_config == {}
Expand Down