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

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

Closed
flood4life opened this issue Dec 20, 2023 · 3 comments · Fixed by #2211
Assignees

Comments

@flood4life
Copy link

flood4life commented Dec 20, 2023

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
Copy link
Member

sl0thentr0py commented Dec 21, 2023

@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:

  • if you merely want global defaults, I'd prefer to keep them under the sentry config and then those will get applied to every job
  • if you want per job settings, those can be extra settings picked up from your sidekiq-scheduler yaml declaration and applied while patching

Do you want either/both?

@flood4life
Copy link
Author

@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
Copy link
Member

alright I have some time next week, should be done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants