getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
933 stars 494 forks source link

Ability to change the default grace period and timeout for all Cron Monitors when using the sidekiq_scheduler patches #2205

Closed flood4life closed 10 months ago

flood4life commented 10 months ago

Describe the idea

We find the sidekiq_scheduler patch extremely useful, especially as we have 100s of cron jobs defined there.

However, the default grace period of 1 minute does not work that well for our queue configuration: the overwhelming majority of our cron jobs will have latency of 1-3 minutes, and therefore Sentry will create cron issues every single time the checkin is delayed by a couple of minutes, resulting in a abysmal signal-to-noise ratio.

image

We would like the ability to change the checkin margins for all monitors created by the patch. An alternative could be an ability to configure the default values per Sentry project, but I'm not sure where to submit that, and what approach you like best.

Why do you think it's beneficial to most of the users

I'm not sure about most users, but at this scale (100s of cron jobs) it is very likely that their latency will exceed the default 1 minute.

Possible implementation

The sidekiq_scheduler patch is currently using only the schedule positional argument to instantiate Monitors, but they support the necessary keyword arguments (checkin_margin and max_runtime) as well.

https://github.com/getsentry/sentry-ruby/blob/master/sentry-sidekiq/lib/sentry/sidekiq-scheduler/scheduler.rb#L38-L43

https://github.com/getsentry/sentry-ruby/blob/master/sentry-ruby/lib/sentry/cron/monitor_config.rb#L26-L31

Perhaps Configuration could be extended to allow setting those two values, and the default value for those keyword arguments could be taken from configuration instead of being nils?

If that sounds good, I'm happy to take a stab at contributing this.

sl0thentr0py commented 10 months ago

@flood4life glad you find the patch useful! always nice to have user validation.

Yes, I was waiting for requests such as this before making assumptions on how to extend functionality. We can indeed expose those options for easier setup.

We have a few options:

Do you want either/both?

flood4life commented 10 months ago

@sl0thentr0py Global defaults is what we're after! All of our cron jobs are not that sensitive, and currently we do not have a use case for per job configuration.

FWIW, I think per job settings tweaking is already kind of achievable by simply using the Sentry::Cron::MonitorCheckIns directly and configuring the monitor manually. However, I like your proposal of allowing that configuration via sidekiq-scheduler yaml, and can see how it might look better in certain scenarios, but our organisation has no need for this at the moment.

And I really appreciate your very timely feedback on our issues ❤️

sl0thentr0py commented 10 months ago

alright I have some time next week, should be done