Non-standard Crontab syntax isn't being transformed for Crons check-ins

[gaprl]

How do you use Sentry?

Sentry Saas (sentry.io)

Version

2.29.1

Steps to Reproduce

We have the following monitor with day_of_the_week as sat, but this isn't a standard crontab schedule. We should parse this before sending the check-in to use a number from 1-7, otherwise Crons will not be able to process the check-in.

Here's one of our own that doesn't work: https://github.com/getsentry/sentry/blob/e4a87d6bb80c52c24c5cfe58f5824252ce9ea6fd/src/sentry/conf/server.py#L1182-L1187

Expected Result

To parse "sat" as 6 when sending the monitor config.

Actual Result

The monitor with "sat" is sending the following check-in payload:

{
  "check_in_id": "bb42e598bf51411199d7280cfbf33af3",
  "contexts": {
    "trace": {
      "trace_id": "9c35f5b679234823a2a5e6f7dc56c6c7"
    }
  },
  "duration": 3336.9681780338287,
  "environment": "prod",
  "monitor_config": {
    "schedule": {
      "type": "crontab",
      "value": "0 0 * * saturday"
    },
    "timezone": "UTC"
  },
  "monitor_slug": "schedule-weekly-organization-reports-new",
  "status": "ok"
}

0 0 * * saturday is invalid, it should be 0 0 * * 6

[szokeasaurusrex]

Hi @gaprl – in general, I'd say we should implement any data normalization/transformations in Relay or the backend so that we can use the logic across all SDKs.

However, in this specific case, where Celery's crontab format is being used, I think it does make sense to fix the behavior in the SDK, since the crontab class is able to parse the value "sat" as expected and use it for the cron schedule. In the SDK, it should be possible for us to extract the value and send it in a format that the server understands. So, we can make this fix specifically for Celery crontab schedules.

[szokeasaurusrex]

Edit to above comment: after some further investigation, it seems Celery is using a non-standard flavor of cronjob syntax. In order to properly support all cron schedules in Sentry, we would need to add support in the backend for Celery cron schedules. I don't think this syntax difference is the cause of this issue, but it could cause other problems when using the crons product with Celery jobs. The issue I discovered would arise specifically when both the day of the month and the day of the week are specified (e.g. in Celery 0 0 1 * 1 would run on the first day of the month if it is a Monday, where Sentry appears to expect the same schedule to mean that the job would run the first day of every month and on every Monday).

For this specific problem of a schedule like 0 0 * * saturday or 0 0 * * sat being misinterpreted in Sentry, we should perform the normalization on the server-side or in Relay, not in the SDK.

I'll close this issue for now since it should get fixed in Relay/the backend, but we should likely discuss further about the other problem with interactions between day of week and day of month; fixing this likely would require changes in both the Sentry backend and the SDKs

[evanpurkhiser]

Is there a new issue to track this? This is likely affecting customers too

[szokeasaurusrex]

@evanpurkhiser I can write up a new issue for this tomorrow. I wasn't sure how much of an edge case it is for folks to have conflicting day of week and day of month entries, which might be handled differently depending on the cron implementation

[szokeasaurusrex]

Hi @evanpurkhiser, made an issue over here: getsentry/sentry#92203

Please let me know if you have any questions of if anything is unclear