Improvements around the cron monitoring feature (#2212)

* Test MonitorCheckIns end-to-end

* Apply global cron configs outside of MonitorConfig class

This has a few benefits:

- Keep the MonitorConfig class simple
- Make check in logic centralized in MonitorCheckIns module

* Make MonitorCheckIns' rescue more precise

* Remove unnecessary extend

Author: st0012

sentry-ruby/lib/sentry/cron/monitor_check_ins.rb

@@ -13,32 +13,42 @@ def perform(*args, **opts)
                                                 monitor_config: monitor_config)
 
           start = Sentry.utc_now.to_i
-          # need to do this on ruby <= 2.6 sadly
-          ret = method(:perform).super_method.arity == 0 ? super() : super
-          duration = Sentry.utc_now.to_i - start
 
-          Sentry.capture_check_in(slug,
-                                  :ok,
-                                  check_in_id: check_in_id,
-                                  duration: duration,
-                                  monitor_config: monitor_config)
+          begin
+            # need to do this on ruby <= 2.6 sadly
+            ret = method(:perform).super_method.arity == 0 ? super() : super
+            duration = Sentry.utc_now.to_i - start
 
-          ret
-        rescue Exception
-          duration = Sentry.utc_now.to_i - start
+            Sentry.capture_check_in(slug,
+                                    :ok,
+                                    check_in_id: check_in_id,
+                                    duration: duration,
+                                    monitor_config: monitor_config)
 
-          Sentry.capture_check_in(slug,
-                                  :error,
-                                  check_in_id: check_in_id,
-                                  duration: duration,
-                                  monitor_config: monitor_config)
+            ret
+          rescue Exception
+            duration = Sentry.utc_now.to_i - start
 
-          raise
+            Sentry.capture_check_in(slug,
+                                    :error,
+                                    check_in_id: check_in_id,
+                                    duration: duration,
+                                    monitor_config: monitor_config)
+
+            raise
+          end
         end
       end
 
       module ClassMethods
         def sentry_monitor_check_ins(slug: nil, monitor_config: nil)
+          if monitor_config && Sentry.configuration
+            cron_config = Sentry.configuration.cron
+            monitor_config.checkin_margin ||= cron_config.default_checkin_margin
+            monitor_config.max_runtime ||= cron_config.default_max_runtime
+            monitor_config.timezone ||= cron_config.default_timezone
+          end
+
           @sentry_monitor_slug = slug
           @sentry_monitor_config = monitor_config
 
@@ -57,8 +67,6 @@ def sentry_monitor_config
         end
       end
 
-      extend ClassMethods
-
       def self.included(base)
         base.extend(ClassMethods)
       end

sentry-ruby/lib/sentry/cron/monitor_config.rb

@@ -25,9 +25,9 @@ class MonitorConfig
 
       def initialize(schedule, checkin_margin: nil, max_runtime: nil, timezone: nil)
         @schedule = schedule
-        @checkin_margin = checkin_margin || Sentry.configuration&.cron&.default_checkin_margin
-        @max_runtime = max_runtime || Sentry.configuration&.cron&.default_max_runtime
-        @timezone = timezone || Sentry.configuration&.cron&.default_timezone
+        @checkin_margin = checkin_margin
+        @max_runtime = max_runtime
+        @timezone = timezone
       end
 
       def self.from_crontab(crontab, **options)

sentry-ruby/spec/sentry/cron/monitor_check_ins_spec.rb

@@ -33,8 +33,9 @@ def perform(a, b = 42, c: 99)
     it_behaves_like 'original_job'
 
     it 'does not call capture_check_in' do
-      expect(Sentry).not_to receive(:capture_check_in)
       job.perform(1)
+
+      expect(sentry_events.count).to eq(0)
     end
   end
 
@@ -105,20 +106,21 @@ def perform(a, b = 42, c: 99)
         expect(Job.ancestors.first).to eq(described_class::Patch)
       end
 
-      it 'calls capture_check_in twice' do
-        expect(Sentry).to receive(:capture_check_in).with(
-          'job',
-          :in_progress,
-          hash_including(monitor_config: nil)
-        ).ordered.and_call_original
+      it 'records 2 check-in events' do
+        job.perform(1)
+
+        expect(sentry_events.count).to eq(2)
+        in_progress_event = sentry_events.first
 
-        expect(Sentry).to receive(:capture_check_in).with(
-          'job',
-          :ok,
-          hash_including(:check_in_id, monitor_config: nil, duration: 0)
-        ).ordered.and_call_original
+        expect(in_progress_event.monitor_slug).to eq('job')
+        expect(in_progress_event.status).to eq(:in_progress)
+        expect(in_progress_event.monitor_config).to be_nil
 
-        job.perform(1)
+        ok_event = sentry_events.last
+
+        expect(ok_event.monitor_slug).to eq('job')
+        expect(ok_event.status).to eq(:ok)
+        expect(ok_event.monitor_config).to be_nil
       end
     end
 
@@ -143,25 +145,21 @@ def perform; work end
         expect(Job.ancestors.first).to eq(described_class::Patch)
       end
 
-      it 'does the work' do
-        expect(job).to receive(:work).and_call_original
-        expect(job.perform).to eq(42)
-      end
+      it 'records 2 check-in events' do
+        expect(job.perform(1)).to eq(42)
 
-      it 'calls capture_check_in twice' do
-        expect(Sentry).to receive(:capture_check_in).with(
-          'job',
-          :in_progress,
-          hash_including(monitor_config: nil)
-        ).ordered.and_call_original
+        expect(sentry_events.count).to eq(2)
+        in_progress_event = sentry_events.first
 
-        expect(Sentry).to receive(:capture_check_in).with(
-          'job',
-          :ok,
-          hash_including(:check_in_id, monitor_config: nil, duration: 0)
-        ).ordered.and_call_original
+        expect(in_progress_event.monitor_slug).to eq('job')
+        expect(in_progress_event.status).to eq(:in_progress)
+        expect(in_progress_event.monitor_config).to be_nil
 
-        job.perform(1)
+        ok_event = sentry_events.last
+
+        expect(ok_event.monitor_slug).to eq('job')
+        expect(ok_event.status).to eq(:ok)
+        expect(ok_event.monitor_config).to be_nil
       end
     end
 
@@ -174,10 +172,7 @@ def perform; work end
 
           sentry_monitor_check_ins
 
-          def work(a, b, c); a + b + c end
-
-          def perform(a, b = 42, c: 99)
-            work(a, b, c)
+          def perform
           end
         end
 
@@ -190,17 +185,17 @@ def perform(a, b = 42, c: 99)
       end
     end
 
-    context 'patched with custom options' do
-      let(:config) { Sentry::Cron::MonitorConfig::from_interval(1, :minute) }
+    context 'patched with monitor config' do
+      let(:monitor_config) { Sentry::Cron::MonitorConfig::from_interval(1, :minute) }
 
       before do
         mod = described_class
-        conf = config
+        config = monitor_config
 
         job_class = Class.new do
           include mod
 
-          sentry_monitor_check_ins slug: 'custom_slug', monitor_config: conf
+          sentry_monitor_check_ins slug: 'custom_slug', monitor_config: config
 
           def work(a, b, c); a + b + c end
 
@@ -222,37 +217,104 @@ def perform(a, b = 42, c: 99)
 
       it 'has correct custom options' do
         expect(Job.sentry_monitor_slug).to eq('custom_slug')
-        expect(Job.sentry_monitor_config).to eq(config)
+        expect(Job.sentry_monitor_config).to eq(monitor_config)
+      end
+
+      it 'records 2 check-in events' do
+        job.perform(1)
+
+        expect(sentry_events.count).to eq(2)
+        in_progress_event = sentry_events.first
+
+        expect(in_progress_event.monitor_slug).to eq('custom_slug')
+        expect(in_progress_event.status).to eq(:in_progress)
+        expect(in_progress_event.monitor_config).to eq(monitor_config)
+        expect(in_progress_event.monitor_config.checkin_margin).to eq(nil)
+        expect(in_progress_event.monitor_config.max_runtime).to eq(nil)
+        expect(in_progress_event.monitor_config.timezone).to eq(nil)
+
+        ok_event = sentry_events.last
+
+        expect(ok_event.monitor_slug).to eq('custom_slug')
+        expect(ok_event.status).to eq(:ok)
+        expect(ok_event.monitor_config).to eq(monitor_config)
+      end
+    end
+
+    context 'with custom monitor config object and cron configs' do
+      let(:monitor_config) { Sentry::Cron::MonitorConfig::from_interval(1, :minute) }
+
+      before do
+        perform_basic_setup do |config|
+          config.cron.default_checkin_margin = 10
+          config.cron.default_max_runtime = 20
+          config.cron.default_timezone = 'Europe/Vienna'
+        end
+
+        mod = described_class
+        config = monitor_config
+
+        job_class = Class.new do
+          include mod
+
+          sentry_monitor_check_ins slug: 'custom_slug', monitor_config: config
+
+          def work(a, b, c); a + b + c end
+
+          def perform(a, b = 42, c: 99)
+            work(a, b, c)
+          end
+        end
+
+        stub_const('Job', job_class)
       end
 
-      it 'calls capture_check_in twice' do
-        expect(Sentry).to receive(:capture_check_in).with(
-          'custom_slug',
-          :in_progress,
-          hash_including(monitor_config: config)
-        ).ordered.and_call_original
+      let(:job) { Job.new }
+
+      it_behaves_like 'original_job'
+
+      it 'prepends the patch' do
+        expect(Job.ancestors.first).to eq(described_class::Patch)
+      end
 
-        expect(Sentry).to receive(:capture_check_in).with(
-          'custom_slug',
-          :ok,
-          hash_including(:check_in_id, monitor_config: config, duration: 0)
-        ).ordered.and_call_original
+      it 'has correct custom options' do
+        expect(Job.sentry_monitor_slug).to eq('custom_slug')
+        expect(Job.sentry_monitor_config).to eq(monitor_config)
+      end
 
+      it 'records 2 check-in events' do
         job.perform(1)
+
+        expect(sentry_events.count).to eq(2)
+        in_progress_event = sentry_events.first
+
+        expect(in_progress_event.monitor_slug).to eq('custom_slug')
+        expect(in_progress_event.status).to eq(:in_progress)
+        expect(in_progress_event.monitor_config.checkin_margin).to eq(10)
+        expect(in_progress_event.monitor_config.max_runtime).to eq(20)
+        expect(in_progress_event.monitor_config.timezone).to eq('Europe/Vienna')
+
+        ok_event = sentry_events.last
+
+        expect(ok_event.monitor_slug).to eq('custom_slug')
+        expect(ok_event.status).to eq(:ok)
+        expect(ok_event.monitor_config.checkin_margin).to eq(10)
+        expect(ok_event.monitor_config.max_runtime).to eq(20)
+        expect(ok_event.monitor_config.timezone).to eq('Europe/Vienna')
       end
     end
 
     context 'patched with custom options with exception' do
-      let(:config) { Sentry::Cron::MonitorConfig::from_crontab('5 * * * *') }
+      let(:monitor_config) { Sentry::Cron::MonitorConfig::from_crontab('5 * * * *') }
 
       before do
         mod = described_class
-        conf = config
+        config = monitor_config
 
         job_class = Class.new do
           include mod
 
-          sentry_monitor_check_ins slug: 'custom_slug', monitor_config: conf
+          sentry_monitor_check_ins slug: 'custom_slug', monitor_config: config
 
           def work(a, b, c);
             1 / 0
@@ -284,23 +346,24 @@ def perform(a, b = 42, c: 99)
 
       it 'has correct custom options' do
         expect(Job.sentry_monitor_slug).to eq('custom_slug')
-        expect(Job.sentry_monitor_config).to eq(config)
+        expect(Job.sentry_monitor_config).to eq(monitor_config)
       end
 
       it 'calls capture_check_in twice with error status and re-raises exception' do
-        expect(Sentry).to receive(:capture_check_in).with(
-          'custom_slug',
-          :in_progress,
-          hash_including(monitor_config: config)
-        ).ordered.and_call_original
-
-        expect(Sentry).to receive(:capture_check_in).with(
-          'custom_slug',
-          :error,
-          hash_including(:check_in_id, monitor_config: config, duration: 0)
-        ).ordered.and_call_original
-
         expect { job.perform(1) }.to raise_error(ZeroDivisionError)
+
+        expect(sentry_events.count).to eq(2)
+        in_progress_event = sentry_events.first
+
+        expect(in_progress_event.monitor_slug).to eq('custom_slug')
+        expect(in_progress_event.status).to eq(:in_progress)
+        expect(in_progress_event.monitor_config).to eq(monitor_config)
+
+        error_event = sentry_events.last
+
+        expect(error_event.monitor_slug).to eq('custom_slug')
+        expect(error_event.status).to eq(:error)
+        expect(error_event.monitor_config).to eq(monitor_config)
       end
     end
   end