Crons: add sidekiq-scheduler zero-config monitor check-ins#2172
Crons: add sidekiq-scheduler zero-config monitor check-ins#2172sl0thentr0py merged 8 commits intogetsentry:masterfrom
sidekiq-scheduler zero-config monitor check-ins#2172Conversation
Codecov Report
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
|
|
thx @natikgadzhi pls revert the renames, those are intentional and follow the namespacing of the gem we are patching I'll review the other stuff now. |
these are our dependencies for testing, not for our users. |
sentry-sidekiq/lib/sentry/sidekiq/sidekiq-scheduler/scheduler.rb
Outdated
Show resolved
Hide resolved
sentry-sidekiq/lib/sentry/sidekiq/sidekiq-scheduler/scheduler.rb
Outdated
Show resolved
Hide resolved
sentry-sidekiq/lib/sentry/sidekiq/sidekiq-scheduler/scheduler.rb
Outdated
Show resolved
Hide resolved
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. |
0b49536 to
9e205c9
Compare
|
@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.
|
sentry-sidekiq/Gemfile
Outdated
|
|
||
| gem "sidekiq", "~> #{sidekiq_version}" | ||
| gem "sidekiq-cron" if sidekiq_version >= Gem::Version.new("6.0") | ||
| gem "sidekiq-scheduler" if sidekiq_version >= Gem::Version.new("6.0") |
There was a problem hiding this comment.
Let's merge the 2 under one condition?
| ::Sidekiq.logger.info "Injected Sentry Crons monitor checkins into #{config.fetch("class")}" | ||
| end | ||
|
|
||
| return rufus_job |
There was a problem hiding this comment.
This return is not necessary.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Speaking of — should we perhaps wire up Rubocop and enforce formatting in a pre-commit hook? Or would that be an overkill?
|
@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. |
|
@st0012 @sl0thentr0py welp. Next steps seem to be:
|
|
@sl0thentr0py @st0012 Fixed the Ruby 2.5 compatibility, now the CI is just trolling me. |
|
re-ran just timestamp flakiness |
a3b3a6f to
610cf44
Compare
- Adds support for `sidekiq-scheduler` instrumentation without any configuration from the user. - Based on getsentry#2170.
- AppliesApply review feedback. - Adds support for interval and every interval_types for sidekiq-scheduler-schedule - Adds specs for the above.
610cf44 to
d9ea90b
Compare
* need int for interval * constantize doesn't exist outside rails * cleanup specs
sl0thentr0py
left a comment
There was a problem hiding this comment.
ok i made some minor changes and style cleanup, see last commit
good to go from my side, also tested, nice work @natikgadzhi !
Summary
This pull request adds Crons support for
sidekiq-schedulerjobs with zero configuration.sidekiq-cronpatch for automatic monitoring of jobs listed in the schedule #2170 approach by @sl0thentr0pyChanges
sidekiq-croninstrumentation 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?sidekiq-schedulerintegrationHow this works
Similar to #2170, when
SidekiqSchedulerloads the jobs, we hook into it'snew_jobmethod, constantine the job class, and patch it withMonitorCheckInsmodule.Open questions
Is it okay to have both
sidekiq-cronandsidekiq-schedulerin ourGemfile, and require both of them inspec_helperforsentry-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
requirecalls 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.