Crons: add sidekiq-scheduler zero-config monitor check-ins

[natikgadzhi]

Summary

This pull request adds Crons support for sidekiq-scheduler jobs with zero configuration.

• Part of Crons automatic support for gems #2134
• Based on Add sidekiq-cron patch for automatic monitoring of jobs listed in the schedule #2170 approach by @sl0thentr0py

Changes

• 3543bec First commit moves sidekiq-cron instrumentation to have a clearer naming. I'm not attached to this — okay to revert if you don't feel like this is needed. I think it's nice when directory structure helps you understand what we're instrumenting specifically, but I don't know for absolutely certain if this will work nicely with zeitwerk?
• 0b49536 implements sidekiq-scheduler integration

How this works

Similar to #2170, when SidekiqScheduler loads the jobs, we hook into it's new_job method, constantine the job class, and patch it with MonitorCheckIns module.

Open questions

• Is it okay to have both sidekiq-cron and sidekiq-scheduler in our Gemfile, and require both of them in spec_helper for sentry-sidekiq? I feel like folks could be surprised that Sentry tugs two extra dependencies, one of which they probably don't need at all.

  I don't know how to best work around this, though — if we drop the dependencies and wrap the require calls in error handling, I don't know if Bundler will allow us require anything from the global scope that is not a part of the bundle, right? So... 👀

• Add a changelog entry.

[codecov]

Codecov Report

Merging #2172 (989c530) into master (78fb37a) will decrease coverage by 0.04%.
The diff coverage is 91.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2172      +/-   ##
==========================================
- Coverage   97.34%   97.30%   -0.04%     
==========================================
  Files          98       99       +1     
  Lines        3657     3680      +23     
==========================================
+ Hits         3560     3581      +21     
- Misses         97       99       +2     

Components	Coverage Δ
sentry-ruby	98.02% <ø> (ø)
sentry-rails	94.98% <ø> (ø)
sentry-sidekiq	94.50% <91.30%> (-0.47%)	⬇️
sentry-resque	92.06% <ø> (ø)
sentry-delayed_job	94.36% <ø> (ø)
sentry-opentelemetry	100.00% <ø> (ø)

Files	Coverage Δ
sentry-sidekiq/lib/sentry-sidekiq.rb	85.18% <100.00%> (+0.56%)	⬆️
...-sidekiq/lib/sentry/sidekiq-scheduler/scheduler.rb	90.90% <90.90%> (ø)

[sl0thentr0py]

thx @natikgadzhi

pls revert the renames, those are intentional and follow the namespacing of the gem we are patching Sidekiq::Cron::Job -> Sentry::Sidekiq::Cron::Job. In general if you want to do changes like that, please open a separate PR and keep features/bug fixes isolated.

I'll review the other stuff now.

[sl0thentr0py]

Is it okay to have both sidekiq-cron and sidekiq-scheduler in our Gemfile, and require both of them in spec_helper for sentry-sidekiq? I feel like folks could be surprised that Sentry tugs two extra dependencies, one of which they probably don't need at all.

these are our dependencies for testing, not for our users.

[natikgadzhi]

In general if you want to do changes like that, please open a separate PR and keep features/bug fixes isolated.

I even considered a separate PR first, but then settled on a separate commit for that exact reason. No problem, I'm a noob here, so TIL. Will revert.

I'll clean the rest of the feedback up in a bit and push up the fix, then work on documentation.

[natikgadzhi]

@sl0thentr0py thank you a LOT for your feedback! I'm learning as I go, and I'm grateful for your patience with me.

I've worked through your suggestions, so this should be ready for another pass. Also added the changelog line.

UPD: Hold up, I see a bunch of CI failures, let me try these locally and see if I can work around them. Fixed! Not proud of the code there, though.

[st0012]

Let's merge the 2 under one condition?

[st0012]

This return is not necessary.

[natikgadzhi]

I think the upstream new_job returns the Rufus::Job. Wouldn't this method return whatever the unless above results in instead of the job, if we drop this line? (Just learning here, been a while since I wrote Ruby)

[st0012]

Oh I mean the return keyword here is not necessary as it's at the end of the method. We do need to return the same object as you said.

[natikgadzhi]

Speaking of — should we perhaps wire up Rubocop and enforce formatting in a pre-commit hook? Or would that be an overkill?

[natikgadzhi]

@st0012 thank you for the review! I'll clean this up later tonight, should be ready tomorrow.

I also see the Coverage decline, I'll add the test.

[natikgadzhi]

@st0012 @sl0thentr0py welp. Next steps seem to be:

• Code coverage: we could craft tests, but there's not much value to test a scenario where a class name is broken since that would error out upstream anyway.
• Ruby 2.5 doesn't like delegate. ;-( I could add those methods implementations I think, but those are also just tests, the actual integration will work fine. Should I clean this up?

[natikgadzhi]

@sl0thentr0py @st0012 Fixed the Ruby 2.5 compatibility, now the CI is just trolling me.

[sl0thentr0py]

re-ran just timestamp flakiness

[sl0thentr0py]

ok i made some minor changes and style cleanup, see last commit

good to go from my side, also tested, nice work @natikgadzhi !