getsentry / sentry-laravel

The official Laravel SDK for Sentry (sentry.io)
https://sentry.io
MIT License
1.25k stars 189 forks source link

[Cron Monitoring] Getting a bunch of "No check-in reported" when command scheduled with a `when` condition #778

Closed fredericgboutin-yapla closed 11 months ago

fredericgboutin-yapla commented 1 year ago

How do you use Sentry?

Sentry SaaS (sentry.io)

SDK version

3.8.0

Steps to reproduce

I have this command scheduled with a Sentry Monitor attached to it like this,

            $schedule->command('mycommand')
                ->sentryMonitor('my-command-monitor')
                ->dailyAt('13:45')
                ->when(ConsoleCommandHelper::isBiMonthly())
                ->onOneServer();

And then in Sentry, I end-up with a monitor that expects to see this CRON task check-in daily at 13:45 but it doesn't until it is effectively the 15 or the last day of the month, because of the ->when(ConsoleCommandHelper::isBiMonthly())

Basically I have a bunch of warnings in my Sentry monitor for this task and there's no way I can instruct it with a proper CRON expression.

Expected result

Except from not having false negatives being reported from Sentry, I'm not sure what I expect here because in reality the scheduled command is "evaluated" daily at 13:45 but only really executed on the 15 or on the last day of the month.

Should Laravel reports the evaluation to Sentry even if not executed?

Should Sentry allows for more complex schedules?

Is there any workaround I don't think of?

Actual result

I'm having a bunch of "No check-in reported" and I'm not sure what I can do, really.

cleptric commented 1 year ago

We can look into this. For now, you can update the monitor on sentry.io to only be expected twice per month and update your config like:

 $schedule->command('mycommand')
     ->sentryMonitor(
         monitorSlug: 'my-command-monitor',
         updateMonitorConfig: false, // Add this
     )
     ->dailyAt('13:45')
     ->when(ConsoleCommandHelper::isBiMonthly())
     ->onOneServer();

https://docs.sentry.io/platforms/php/guides/laravel/crons/#configure-a-job-monitor

stayallive commented 1 year ago

We can't do much with the ->when(ConsoleCommandHelper::isBiMonthly()), there is no reasonable reason why would understand that, although I get where you are coming from. We go of the schedule set and report that to Sentry to keep a watch on. And it is set to daily at 13:45.

Wouldn't it make more sense for you to set it up with a proper cron expression in the first place using ->cron('45 13 1,16 * *') instead of dailyAt()? This way it will work as expected and Sentry will know what to watch for?

fredericgboutin-yapla commented 1 year ago

We can't do much with the ->when(ConsoleCommandHelper::isBiMonthly()), there is no reasonable reason why would understand that, although I get where you are coming from. We go of the schedule set and report that to Sentry to keep a watch on. And it is set to daily at 13:45.

Wouldn't it make more sense for you to set it up with a proper cron expression in the first place using ->cron('45 13 1,16 * *') instead of dailyAt()? This way it will work as expected and Sentry will know what to watch for?

There is no proper CRON expression for "last day of the month" and the 1st of the month is not the last day either.

fredericgboutin-yapla commented 1 year ago

We can look into this. For now, you can update the monitor on sentry.io to only be expected twice per month and update your config like:

 $schedule->command('mycommand')
     ->sentryMonitor(
         monitorSlug: 'my-command-monitor',
         updateMonitorConfig: false, // Add this
     )
     ->dailyAt('13:45')
     ->when(ConsoleCommandHelper::isBiMonthly())
     ->onOneServer();

https://docs.sentry.io/platforms/php/guides/laravel/crons/#configure-a-job-monitor

How can you do that exactly in Sentry? There is CRON (which I cannot use to express "last day of month") and there is Interval, which is not the same. Am I missing something?

cleptric commented 1 year ago

We currently do not support these kinds of schedules, indeed. I pinged the team if there are any plans to maybe add this in the future. Until then, we can't do much in the SDK either.

fredericgboutin-yapla commented 1 year ago

This is a twofold problem. Yes there is the CRON and whatnot scheduling. But there is definitely the ->when() statement who looks thoroughly down played here, to my surprise.

https://laravel.com/docs/10.x/scheduling#truth-test-constraints

cleptric commented 1 year ago

Yep, can't offer you any solution right now. Sorry

stayallive commented 1 year ago

There are 2 parts at play here, the first is this package that sends the pings to Sentry at the correct times, twice a month.

The real problem is that this is monitored and watched by the Sentry server, and the server is instructed to expect a ping once a day at 13:45, which is in your case incorrect. And as you correctly pointed out there is no valid cron expression to express your particular schedule.

The problem is that the server expects a ping daily and it doesn't get that so it's alerting. And since there is no way to describe your schedule to the server at this moment (it only supports regular intervals or cron expressions) there is no way to solve this. This has nothing to do with the ->when() which works great (hence why you are being alerted by Sentry otherwise we would have pinged Sentry daily if we ignored the ->when()).

It's not the package that generates the alerts but the server and the server doesn't understand (and at this moment cannot be thought) your specific schedule.

I hope that clears it up a bit where the problem lies and why I believe it has nothing to do with the ->when() which works as expected here, only pinging Sentry when the command actually executed (twice a month) instead of what Sentry expects, daily.

fredericgboutin-yapla commented 1 year ago

And since there is no way to describe your schedule to the server at this moment (it only supports regular intervals or cron expressions) there is no way to solve this. This has nothing to do with the ->when() which works great (hence why you are being alerted by Sentry otherwise we would have pinged Sentry daily if we ignored the ->when()).

I partially agree. I specifically made a difference between "evaluating" the command and "executing" it. In that context, that would work to have the Sentry "server" knows that the command was "evaluated" and thus reporting the CRON as "successful". I don't like this approach as it would brush a false idea on the CRON execution, but it would "work".

Anyway. So here is the situation.

First, basically this problem is not gonna get resolved, so I would suggest to at least tag it as a "limitation" and ideally document it with a note or something in Laravel integration for Sentry.

Second, what am I supposed to do now? I guess I should tolerate false negative reports for that specific CRON in Sentry. The feature is in beta, I get that but the workaround and my feeling out of this is basically "your command doesn't fit the bill, so this is your problem" which sounds weird to me tbh.

cleptric commented 11 months ago

We clarified our docs in https://github.com/getsentry/sentry-docs/pull/8643

Tasks that use Laravel's between, unlessBetween, when and skip methods are currently not supported. For the best results, we recommend using Laravel's cron method to define your schedule's frequency.