Add sidekiq_use_scoped_block configuration option (#1038)

* extract scope-creation to .job_scope

* Add sidekiq_use_scoped_block configuration option
- Will use Rollbar.scoped s.t. calls to Rollbar.log will include the sidekiq scope

* Sidekiq::ClearScope -> ResetScope, respect sidekiq_use_scoped_block option

* Fix useless `private` directive

* git mv spec/rollbar/sidekig/reset_scope_spec.rb spec/rollbar/sidekiq/

Author: Eric Tipton

lib/rollbar/configuration.rb

@@ -55,6 +55,7 @@ class Configuration
     attr_accessor :uncaught_exception_level
     attr_accessor :scrub_headers
     attr_accessor :sidekiq_threshold
+    attr_accessor :sidekiq_use_scoped_block
     attr_reader :transform
     attr_accessor :verify_ssl_peer
     attr_accessor :use_async
@@ -139,6 +140,7 @@ def initialize
       @uncaught_exception_level = 'error'
       @scrub_headers = ['Authorization']
       @sidekiq_threshold = 0
+      @sidekiq_use_scoped_block = false
       @safely = false
       @transform = []
       @use_async = false

lib/rollbar/plugins/sidekiq.rb

@@ -9,7 +9,7 @@
 
     Sidekiq.configure_server do |config|
       config.server_middleware do |chain|
-        chain.add Rollbar::Sidekiq::ClearScope
+        chain.add Rollbar::Sidekiq::ResetScope
       end
 
       config.error_handlers << proc do |e, context|

lib/rollbar/plugins/sidekiq/plugin.rb

@@ -4,29 +4,47 @@ module Rollbar
   class Sidekiq
     PARAM_BLACKLIST = %w[backtrace error_backtrace error_message error_class].freeze
 
-    class ClearScope
-      def call(_worker, _msg, _queue)
-        Rollbar.reset_notifier!
+    class ResetScope
+      def call(_worker, msg, _queue)
+        Rollbar.reset_notifier! # clears scope
 
-        yield
+        return yield unless Rollbar.configuration.sidekiq_use_scoped_block
+
+        Rollbar.scoped(Rollbar::Sidekiq.job_scope(msg)) { yield }
       end
     end
 
-    def self.handle_exception(ctx_hash, e)
-      job_hash = ctx_hash && (ctx_hash[:job] || ctx_hash)
-      return if skip_report?(job_hash, e)
+    def self.handle_exception(msg, e)
+      return if skip_report?(msg, e)
+
+      Rollbar.scope(job_scope(msg)).error(e, :use_exception_level_filters => true)
+    end
+
+    def self.skip_report?(msg, _e)
+      job_hash = job_hash_from_msg(msg)
+
+      return false if job_hash.nil?
+
+      # when rollbar middleware catches, sidekiq's retry_job processor hasn't set
+      # the retry_count for the current job yet, so adding 1 gives the actual retry count
+      actual_retry_count = job_hash.fetch('retry_count', -1) + 1
+      job_hash['retry'] && actual_retry_count < ::Rollbar.configuration.sidekiq_threshold
+    end
 
+    def self.job_scope(msg)
       scope = {
         :framework => "Sidekiq: #{::Sidekiq::VERSION}"
       }
+      job_hash = job_hash_from_msg(msg)
+
       unless job_hash.nil?
         params = job_hash.reject { |k| PARAM_BLACKLIST.include?(k) }
         scope[:request] = { :params => scrub_params(params) }
         scope[:context] = params['class']
         scope[:queue] = params['queue']
       end
 
-      Rollbar.scope(scope).error(e, :use_exception_level_filters => true)
+      scope
     end
 
     def self.scrub_params(params)
@@ -38,22 +56,21 @@ def self.scrub_params(params)
       Rollbar::Scrubbers::Params.call(options)
     end
 
-    def self.skip_report?(job_hash, _e)
-      return false if job_hash.nil?
-
-      # when rollbar middleware catches, sidekiq's retry_job processor hasn't set
-      # the retry_count for the current job yet, so adding 1 gives the actual retry count
-      actual_retry_count = job_hash.fetch('retry_count', -1) + 1
-      job_hash['retry'] && actual_retry_count < ::Rollbar.configuration.sidekiq_threshold
-    end
-
+    # see https://github.com/mperham/sidekiq/wiki/Middleware#server-middleware
     def call(_worker, msg, _queue)
-      Rollbar.reset_notifier!
+      Rollbar.reset_notifier! # clears scope
 
-      yield
+      return yield unless Rollbar.configuration.sidekiq_use_scoped_block
+
+      Rollbar.scoped(Rollbar::Sidekiq.job_scope(msg)) { yield }
     rescue Exception => e
       Rollbar::Sidekiq.handle_exception(msg, e)
       raise
     end
+
+    def self.job_hash_from_msg(msg)
+      msg && (msg[:job] || msg)
+    end
+    private_class_method :job_hash_from_msg
   end
 end

spec/rollbar/plugins/sidekiq_spec.rb

@@ -25,7 +25,7 @@
       }
     end
 
-    let(:ctx_hash) do
+    let(:msg) do
       { :context => 'Job raised exception', :job => job_hash }
     end
 
@@ -40,42 +40,42 @@
       }
     end
 
-    it 'constructs scope from ctx hash' do
+    it 'constructs scope from msg' do
       allow(rollbar).to receive(:error)
       expect(Rollbar).to receive(:scope).with(expected_scope) { rollbar }
 
-      described_class.handle_exception(ctx_hash, exception)
+      described_class.handle_exception(msg, exception)
     end
 
-    context 'sidekiq < 4.2.3 ctx hash' do
-      let(:ctx_hash) { job_hash }
+    context 'sidekiq < 4.2.3 msg' do
+      let(:msg) { job_hash }
 
-      it 'constructs scope from ctx hash' do
+      it 'constructs scope from msg' do
         allow(rollbar).to receive(:error)
         expect(Rollbar).to receive(:scope).with(expected_scope) { rollbar }
 
-        described_class.handle_exception(ctx_hash, exception)
+        described_class.handle_exception(msg, exception)
       end
     end
 
-    context 'sidekiq < 4.0.0 nil ctx hash from Launcher#actor_died' do
-      let(:ctx_hash) { nil }
+    context 'sidekiq < 4.0.0 nilmsg from Launcher#actor_died' do
+      let(:msg) { nil }
 
-      it 'constructs scope from ctx hash' do
+      it 'constructs scope from msg' do
         allow(rollbar).to receive(:error)
         expect(Rollbar).to receive(:scope).with(
           :framework => "Sidekiq: #{Sidekiq::VERSION}"
         ) { rollbar }
 
-        described_class.handle_exception(ctx_hash, exception)
+        described_class.handle_exception(msg, exception)
       end
     end
 
     it 'sends the passed-in error to rollbar' do
       allow(Rollbar).to receive(:scope).and_return(rollbar)
       expect(rollbar).to receive(:error).with(exception, :use_exception_level_filters => true)
 
-      described_class.handle_exception(ctx_hash, exception)
+      described_class.handle_exception(msg, exception)
     end
 
     context 'with fields in job hash to be scrubbed' do
@@ -153,7 +153,7 @@
   end
 
   describe '#call' do
-    let(:msg) { ['hello'] }
+    let(:msg) { { 'class' => 'SomeWorker', 'queue' => 'default' } }
     let(:exception) { StandardError.new('oh noes') }
     let(:middleware_block) { proc { raise exception } }
 
@@ -165,5 +165,37 @@
 
       expect { subject.call(nil, msg, nil, &middleware_block) }.to raise_error(exception)
     end
+
+    context 'when the block calls Rollbar.log without raising an error' do
+      let(:middleware_block) { proc { Rollbar.log('warning', 'Danger, Will Robinson') } }
+
+      context 'and Rollbar.configuration.sidekiq_use_scoped_block is false (default)' do
+        before do
+          Rollbar.configuration.sidekiq_use_scoped_block = false
+        end
+
+        it 'does NOT send the scope information to rollbar' do
+          expect(Rollbar).to receive(:log) do
+            expect(Rollbar.scope_object).not_to be_eql(described_class.job_scope(msg))
+          end
+
+          subject.call(nil, msg, nil, &middleware_block)
+        end
+      end
+
+      context 'and Rollbar.configuration.sidekiq_use_scoped_block is true' do
+        before do
+          Rollbar.configuration.sidekiq_use_scoped_block = true
+        end
+
+        it 'sends the scope information to rollbar' do
+          expect(Rollbar).to receive(:log) do
+            expect(Rollbar.scope_object).to be_eql(described_class.job_scope(msg))
+          end
+
+          subject.call(nil, msg, nil, &middleware_block)
+        end
+      end
+    end
   end
 end

spec/rollbar/sidekig/clear_scope_spec.rb

@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+require 'sidekiq'
+
+Rollbar.plugins.load!
+
+describe Rollbar::Sidekiq::ResetScope, :reconfigure_notifier => false do
+  describe '#call' do
+    let(:middleware_block) { proc {} }
+
+    it 'calls Rollbar.reset_notifier!' do
+      expect(Rollbar).to receive(:reset_notifier!)
+
+      subject.call(nil, nil, nil, &middleware_block)
+    end
+
+    context 'when the block calls Rollbar.log without raising an error' do
+      let(:middleware_block) { proc { Rollbar.log('warning', 'Danger, Will Robinson') } }
+      let(:msg) { { 'class' => 'SomeWorker', 'queue' => 'default' } }
+
+      context 'and Rollbar.configuration.sidekiq_use_scoped_block is false (default)' do
+        before do
+          Rollbar.configuration.sidekiq_use_scoped_block = false
+        end
+
+        it 'does NOT send the scope information to rollbar' do
+          expect(Rollbar).to receive(:log) do
+            expect(Rollbar.scope_object).not_to be_eql(Rollbar::Sidekiq.job_scope(msg))
+          end
+
+          subject.call(nil, msg, nil, &middleware_block)
+        end
+      end
+
+      context 'and Rollbar.configuration.sidekiq_use_scoped_block is true' do
+        before do
+          Rollbar.configuration.sidekiq_use_scoped_block = true
+        end
+
+        it 'sends the scope information to rollbar' do
+          expect(Rollbar).to receive(:log) do
+            expect(Rollbar.scope_object).to be_eql(Rollbar::Sidekiq.job_scope(msg))
+          end
+
+          subject.call(nil, msg, nil, &middleware_block)
+        end
+      end
+    end
+  end
+end