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

Add sidekiq-cron patch for automatic monitoring of jobs listed in the schedule #2170

Merged
merged 1 commit into from
Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
### Features

- Improve default slug generation for Crons [#2168](https://github.com/getsentry/sentry-ruby/pull/2168)
- Automatic Crons support for scheduling gems
- Add support for [`sidekiq-cron`](https://github.com/sidekiq-cron/sidekiq-cron) [#2170](https://github.com/getsentry/sentry-ruby/pull/2170)

You can opt in to the `sidekiq-cron` patch and we will automatically monitor check-ins for all jobs listed in your `config/schedule.yml` file.

```rb
config.enabled_patches += [:sidekiq_cron]
```

### Bug Fixes

Expand Down
1 change: 1 addition & 0 deletions sentry-sidekiq/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ sidekiq_version = "7.0" if sidekiq_version.nil?
sidekiq_version = Gem::Version.new(sidekiq_version)

gem "sidekiq", "~> #{sidekiq_version}"
gem "sidekiq-cron" if sidekiq_version >= Gem::Version.new("6.0")
st0012 marked this conversation as resolved.
Show resolved Hide resolved

gem "rails", "> 5.0.0", "< 7.1.0"

Expand Down
3 changes: 3 additions & 0 deletions sentry-sidekiq/lib/sentry-sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,6 @@ class Railtie < ::Rails::Railtie
chain.add Sentry::Sidekiq::SentryContextClientMiddleware
end
end

# patches
require "sentry/sidekiq/cron/job"
34 changes: 34 additions & 0 deletions sentry-sidekiq/lib/sentry/sidekiq/cron/job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# frozen_string_literal: true

return unless defined?(::Sidekiq::Cron::Job)
sl0thentr0py marked this conversation as resolved.
Show resolved Hide resolved

module Sentry
module Sidekiq
module Cron
module Job
def save
# validation failed, do nothing
return false unless super

# fail gracefully if can't find class
klass_const =
begin
::Sidekiq::Cron::Support.constantize(klass.to_s)
rescue NameError
return true
end

# only patch if not explicitly included in job by user
unless klass_const.send(:ancestors).include?(Sentry::Cron::MonitorCheckIns)
klass_const.send(:include, Sentry::Cron::MonitorCheckIns)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make it so any invocation of the related Job class, including invocations triggered by sidekiq-cron, and invocations that the user did manually, with just HappyWorkerJob.perform_async? It looks like it would, right?

I wonder if there are situations when users want to kick the same job for a one-off thing that they don't want to be reported to Sentry. I, personally, think that since the defined sidekiq-cron job just wraps a Sidekiq job, than whenever that job was triggered, it should be reported.

Do we want to add this to docs? Is this behavior consistent with other languages and frameworks Sentry supports?

/cc @sl0thentr0py

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add docs for all of this around releasing.

Would this make it so any invocation of the related Job class, including invocations triggered by sidekiq-cron, and invocations that the user did manually, with just HappyWorkerJob.perform_async? It looks like it would, right?

yes

I wonder if there are situations when users want to kick the same job for a one-off thing that they don't want to be reported to Sentry. I, personally, think that since the defined sidekiq-cron job just wraps a Sidekiq job, than whenever that job was triggered, it should be reported.

This is a new feature, we will add special config once people use it and request it. This is fine as a first version, it's an opt-in patch either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there could be multiple ways to invoke save in addition to the initial creation. Although the current implementation doesn't seem to have obvious performance impact, I feel a better place to inject such one-off activation logic is load_from_array instead.
I understand that it's an experimental feature for now, so I'm not suggesting an immediate refactor. Just want to point it out for future improvements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if people are scheduling cron jobs with Sidekiq::Cron::Job.create, we patch those too automatically, either way shouldn't matter, we can react to feedback from users later depending on usage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honest question so I can draft the docs PR:

  • to enable sidekiq-cron and/or sidekiq-scheduler monitoring, the users will have to manually add config.enabled_patches += [:sidekiq_cron], right?
  • When this patch is applied, and as long as sidekiq-cron is indeed available, we aim to instrument all jobs that it runs, all the time. The current logic that hooks into save does that. Correct?
  • One side-iffect is that we're instrumenting the job class itself, so whether it was the sidekiq-cron schedule that invoked the job on a timer, or the user's code invoked perform_* on that job, we will monitor that too. That's by design for now, but we'll keep an eye out for feedback. Right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes to all

klass_const.send(:sentry_monitor_check_ins,
slug: name,
monitor_config: Sentry::Cron::MonitorConfig.from_crontab(cron))
end
sl0thentr0py marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
end
end

Sentry.register_patch(:sidekiq_cron, Sentry::Sidekiq::Cron::Job, ::Sidekiq::Cron::Job)
11 changes: 11 additions & 0 deletions sentry-sidekiq/spec/fixtures/schedule.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
happy:
cron: "* * * * *"
class: "HappyWorkerDup"

manual:
cron: "* * * * *"
class: "SadWorkerWithCron"

invalid_cron:
cron: "not a crontab"
class: "ReportingWorker"
46 changes: 46 additions & 0 deletions sentry-sidekiq/spec/sentry/sidekiq/cron/job_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require 'spec_helper'

return unless defined?(Sidekiq::Cron::Job)

RSpec.describe Sentry::Sidekiq::Cron::Job do
before do
perform_basic_setup { |c| c.enabled_patches += [:sidekiq_cron] }
end

before do
schedule_file = 'spec/fixtures/schedule.yml'
schedule = Sidekiq::Cron::Support.load_yaml(ERB.new(IO.read(schedule_file)).result)
Sidekiq::Cron::Job.load_from_hash!(schedule, source: 'schedule')
end

it 'patches class' do
expect(Sidekiq::Cron::Job.ancestors).to include(described_class)
end

it 'patches HappyWorker' do
expect(HappyWorkerDup.ancestors).to include(Sentry::Cron::MonitorCheckIns)
expect(HappyWorkerDup.sentry_monitor_slug).to eq('happy')
expect(HappyWorkerDup.sentry_monitor_config).to be_a(Sentry::Cron::MonitorConfig)
expect(HappyWorkerDup.sentry_monitor_config.schedule).to be_a(Sentry::Cron::MonitorSchedule::Crontab)
expect(HappyWorkerDup.sentry_monitor_config.schedule.value).to eq('* * * * *')
end

it 'does not override SadWorkerWithCron manually set values' do
expect(SadWorkerWithCron.ancestors).to include(Sentry::Cron::MonitorCheckIns)
expect(SadWorkerWithCron.sentry_monitor_slug).to eq('failed_job')
expect(SadWorkerWithCron.sentry_monitor_config).to be_a(Sentry::Cron::MonitorConfig)
expect(SadWorkerWithCron.sentry_monitor_config.schedule).to be_a(Sentry::Cron::MonitorSchedule::Crontab)
expect(SadWorkerWithCron.sentry_monitor_config.schedule.value).to eq('5 * * * *')
end

it 'does not patch ReportingWorker because of invalid schedule' do
expect(ReportingWorker.ancestors).not_to include(Sentry::Cron::MonitorSchedule)
end

it 'does not raise error on invalid class' do
expect do
Sidekiq::Cron::Job.create(name: 'invalid_class', cron: '* * * * *', class: 'UndefinedClass')
end.not_to raise_error
end

end
2 changes: 1 addition & 1 deletion sentry-sidekiq/spec/sentry/sidekiq_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
event = transport.events.last.to_json_compatible

expect(event["message"]).to eq "I have something to say!"
expect(event["contexts"]["sidekiq"]).to eq("args" => [], "class" => "ReportingWorker", "jid" => "123123", "queue" => "default")
expect(event["contexts"]["sidekiq"]).to include("args" => [], "class" => "ReportingWorker", "jid" => "123123", "queue" => "default")
end

it "adds the failed job to the retry queue" do
Expand Down
7 changes: 4 additions & 3 deletions sentry-sidekiq/spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
WITH_SIDEKIQ_7 = Gem::Version.new(Sidekiq::VERSION) >= Gem::Version.new("7.0")
WITH_SIDEKIQ_6 = Gem::Version.new(Sidekiq::VERSION) >= Gem::Version.new("6.0") && !WITH_SIDEKIQ_7

if WITH_SIDEKIQ_7
require "sidekiq/embedded"
end
require "sidekiq/embedded" if WITH_SIDEKIQ_7
require 'sidekiq-cron' if RUBY_VERSION.to_f >= 2.7 && WITH_SIDEKIQ_6 || WITH_SIDEKIQ_7

require "sentry-ruby"

Expand Down Expand Up @@ -129,6 +128,8 @@ def perform
end
end

class HappyWorkerDup < HappyWorker; end

class HappyWorkerWithCron < HappyWorker
include Sentry::Cron::MonitorCheckIns
sentry_monitor_check_ins
Expand Down