Merge pull request #555 from nateberkopec/better-transaction-names

Sidekiq: identify culprit + drop Sidekiq 2

Author: nateberkopec

Gemfile

@@ -4,6 +4,7 @@ gemspec
 
 group :test do
   gem "rack"
+  gem "sidekiq"
 end
 
 group :development do

gemfiles/rails32.gemfile

@@ -1,5 +1,6 @@
 source "http://rubygems.org"
 
 gem "rails", ["~> 3.2.6", "!= 3.2.22.1"]
+gem "sidekiq", "3.2.1"
 
 gemspec :path => "../"

gemfiles/rails42.gemfile

@@ -2,5 +2,6 @@ source "http://rubygems.org"
 
 gem "rails", "~> 4.2.0"
 gem "mime-types", "< 3.0.0"
+gem "sidekiq", "3.2.1"
 
 gemspec :path => "../"

gemfiles/rails5.gemfile

@@ -1,5 +1,6 @@
 source "http://rubygems.org"
 
 gem "rails", "5.0.0"
+gem "sidekiq"
 
 gemspec :path => "../"

lib/raven/integrations/sidekiq.rb

@@ -3,56 +3,62 @@
 
 module Raven
   class Sidekiq
-    def call(_worker, msg, _queue)
-      started_at = Time.now
-      yield
-    rescue Exception => ex
-      Raven.capture_exception(ex, :extra => { :sidekiq => msg },
-                                  :time_spent => Time.now - started_at)
-      raise
+    ACTIVEJOB_RESERVED_PREFIX = "_aj_".freeze
+
+    def call(ex, context)
+      context = filter_context(context)
+      Raven.capture_exception(
+        ex,
+        :message => ex.message,
+        :extra => { :sidekiq => context },
+        :culprit => culprit_from_context(context)
+      )
     ensure
       Context.clear!
       BreadcrumbBuffer.clear!
     end
-  end
-end
 
-if Sidekiq::VERSION < '3'
-  # old behavior
-  ::Sidekiq.configure_server do |config|
-    config.server_middleware do |chain|
-      chain.add ::Raven::Sidekiq
+    private
+
+    # Once an ActiveJob is queued, ActiveRecord references get serialized into
+    # some internal reserved keys, such as _aj_globalid.
+    #
+    # The problem is, if this job in turn gets queued back into ActiveJob with
+    # these magic reserved keys, ActiveJob will throw up and error. We want to
+    # capture these and mutate the keys so we can sanely report it.
+    def filter_context(context)
+      case context
+      when Array
+        context.map { |arg| filter_context(arg) }
+      when Hash
+        Hash[context.map { |key, value| filter_context_hash(key, value) }]
+      else
+        context
+      end
     end
-  end
-else
-  Sidekiq.configure_server do |config|
-    config.error_handlers << proc do |ex, context|
-      Raven.capture_exception(ex, :extra => {
-                                :sidekiq => filter_context(context)
-                              })
+
+    def filter_context_hash(key, value)
+      (key = key[3..-1]) if key [0..3] == ACTIVEJOB_RESERVED_PREFIX
+      [key, filter_context(value)]
     end
-  end
-end
 
-def filter_context(context)
-  case context
-  when Array
-    context.map { |arg| filter_context(arg) }
-  when Hash
-    Hash[context.map { |key, value| filter_context_hash(key, value) }]
-  else
-    context
+    # this will change in the future:
+    # https://github.com/mperham/sidekiq/pull/3161
+    def culprit_from_context(context)
+      classname = (context["class"] || (context["job"] && context["job"]["class"]))
+      if classname
+        "Sidekiq/#{classname}"
+      elsif context["event"]
+        "Sidekiq/#{context['event']}"
+      else
+        "Sidekiq"
+      end
+    end
   end
 end
 
-def filter_context_hash(key, value)
-  # Strip any `_aj` prefixes from keys.
-  # These keys come from an internal serialized object from ActiveJob.
-  # Internally, there are a subset of keys that ActiveJob references, but
-  # these are declared as private, and I don't think it's wise
-  # to keep chasing what this list is. But they all use a common prefix, so
-  # we want to strip this becuase ActiveJob will complain.
-  # e.g.: _aj_globalid -> _globalid
-  (key = key[3..-1]) if key [0..3] == "_aj_"
-  [key, filter_context(value)]
+if Sidekiq::VERSION > '3'
+  Sidekiq.configure_server do |config|
+    config.error_handlers << Raven::Sidekiq.new
+  end
 end

spec/raven/integrations/sidekiq_spec.rb

@@ -0,0 +1,57 @@
+require 'spec_helper'
+require 'raven/integrations/sidekiq'
+
+describe Raven::Sidekiq do
+  before(:all) do
+    Raven.configure do |config|
+      config.dsn = 'dummy://notaserver'
+    end
+  end
+
+  let(:context) do
+    {
+      "args" => [true, true],
+      "class" => "HardWorker",
+      "created_at" => 1_474_922_824.910579,
+      "enqueued_at" => 1_474_922_824.910665,
+      "error_class" => "RuntimeError",
+      "error_message" => "a wild exception appeared",
+      "failed_at" => 1_474_922_825.158953,
+      "jid" => "701ed9cfa51c84a763d56bc4",
+      "queue" => "default",
+      "retry" => true,
+      "retry_count" => 0
+    }
+  end
+
+  it "should capture exceptions based on Sidekiq context" do
+    exception = build_exception
+    expected_options = {
+      :message => exception.message,
+      :extra => { :sidekiq => context },
+      :culprit => "Sidekiq/HardWorker"
+    }
+
+    expect(Raven).to receive(:capture_exception).with(exception, expected_options)
+
+    subject.call(exception, context)
+  end
+
+  it "filters out ActiveJob keys" do
+    exception = build_exception
+    aj_context = context
+    aj_context["_aj_globalid"] = "oh noes"
+    expected_context = aj_context
+    expected_context.delete("_aj_globalid")
+    expected_context["_globalid"] = "oh noes"
+    expected_options = {
+      :message => exception.message,
+      :extra => { :sidekiq => expected_context },
+      :culprit => "Sidekiq/HardWorker"
+    }
+
+    expect(Raven).to receive(:capture_exception).with(exception, expected_options)
+
+    subject.call(exception, aj_context)
+  end
+end