Closed natikgadzhi closed 10 months ago
Merging #2172 (989c530) into master (78fb37a) will decrease coverage by
0.04%
. The diff coverage is91.30%
.
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.
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.
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.
@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 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:
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?@sl0thentr0py @st0012 Fixed the Ruby 2.5 compatibility, now the CI is just trolling me.
re-ran just timestamp flakiness
Summary
This pull request adds Crons support for
sidekiq-scheduler
jobs with zero configuration.Changes
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?sidekiq-scheduler
integrationHow this works
Similar to #2170, when
SidekiqScheduler
loads the jobs, we hook into it'snew_job
method, constantine the job class, and patch it withMonitorCheckIns
module.Open questions
[x] Is it okay to have both
sidekiq-cron
andsidekiq-scheduler
in ourGemfile
, and require both of them inspec_helper
forsentry-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... 👀