Use prepended method instead of around_perform for ActiveJob integration (#1631)

st0012 · web-flow · commit 9303b9ade03e · 2021-12-02T01:30:36.000+08:00
* use prepended method instead of around_perform for activejob integration since the sdk registers its around_perform as the "first" callback, it needs to call all error handlers manually (which naturally should happen later) to see if it should report the exception. usually, this approach works well because if an exception will be handled later, handling it ealier doesn't make much of a difference. however, as described in #956, if there's another exception happened inside one of the error handlers, it'll re-enter the error handler again in here: https://github.com/rails/rails/blob/38cb5610b1/activejob/lib/active_job/execution.rb#l50-l51 Of course, it may be possible to handle such case by adding more rescue block inside the `capture_and_reraise_with_sentry` method. But it'll force the entire exception handling flow to be performed inside the first `around_perform` block. And that's something we should avoid. * Refactor ActiveJob integration * Fix SendEventJob's rescue_from callback * Update changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,10 @@
+## Unreleased
+
+### Bug Fixes
+
+- Use prepended method instead of `around_perform` for `ActiveJob` integration [#1631](https://github.com/getsentry/sentry-ruby/pull/1631)
+  - Fixes [#956](https://github.com/getsentry/sentry-ruby/issues/956) and [#1629](https://github.com/getsentry/sentry-ruby/issues/1629)
+
 ## 4.8.1
 
 ### Bug Fixes
diff --git a/sentry-rails/app/jobs/sentry/send_event_job.rb b/sentry-rails/app/jobs/sentry/send_event_job.rb
@@ -16,8 +16,8 @@ class SendEventJob < parent_job
         discard_on ActiveJob::DeserializationError
       else
         # mimic what discard_on does for Rails 5.0
-        rescue_from ActiveJob::DeserializationError do
-          logger.error "Discarded #{self.class} due to a #{exception}. The original exception was #{error.cause.inspect}."
+        rescue_from ActiveJob::DeserializationError do |exception|
+          logger.error "Discarded #{self.class} due to a #{exception}. The original exception was #{exception.cause.inspect}."
         end
       end
 
diff --git a/sentry-rails/lib/sentry/rails/active_job.rb b/sentry-rails/lib/sentry/rails/active_job.rb
@@ -1,28 +1,22 @@
 module Sentry
   module Rails
     module ActiveJobExtensions
-      def self.included(base)
-        base.class_eval do
-          around_perform do |job, block|
-            if Sentry.initialized?
-              if already_supported_by_specific_integration?(job)
-                block.call
-              else
-                Sentry.with_scope do |scope|
-                  capture_and_reraise_with_sentry(job, scope, block)
-                end
-              end
-            else
-              block.call
+      def perform_now
+        if !Sentry.initialized? || already_supported_by_sentry_integration?
+          super
+        else
+          Sentry.with_scope do |scope|
+            capture_and_reraise_with_sentry(scope) do
+              super
             end
           end
         end
       end
 
-      def capture_and_reraise_with_sentry(job, scope, block)
-        scope.set_transaction_name(job.class.name)
+      def capture_and_reraise_with_sentry(scope, &block)
+        scope.set_transaction_name(self.class.name)
         transaction =
-          if job.is_a?(::Sentry::SendEventJob)
+          if is_a?(::Sentry::SendEventJob)
             nil
           else
             Sentry.start_transaction(name: scope.transaction_name, op: "active_job")
@@ -32,42 +26,40 @@ def capture_and_reraise_with_sentry(job, scope, block)
 
         block.call
 
-        finish_transaction(transaction, 200)
+        finish_sentry_transaction(transaction, 200)
       rescue Exception => e # rubocop:disable Lint/RescueException
-        rescue_handler_result = rescue_with_handler(e)
-        finish_transaction(transaction, 500)
-        return rescue_handler_result if rescue_handler_result
+        finish_sentry_transaction(transaction, 500)
 
         Sentry::Rails.capture_exception(
           e,
-          extra: sentry_context(job),
+          extra: sentry_context,
           tags: {
-            job_id: job.job_id,
-            provider_job_id: job.provider_job_id
+            job_id: job_id,
+            provider_job_id:provider_job_id
           }
         )
         raise e
       end
 
-      def finish_transaction(transaction, status)
+      def finish_sentry_transaction(transaction, status)
         return unless transaction
 
         transaction.set_http_status(status)
         transaction.finish
       end
 
-      def already_supported_by_specific_integration?(job)
-        Sentry.configuration.rails.skippable_job_adapters.include?(job.class.queue_adapter.class.to_s)
+      def already_supported_by_sentry_integration?
+        Sentry.configuration.rails.skippable_job_adapters.include?(self.class.queue_adapter.class.to_s)
       end
 
-      def sentry_context(job)
+      def sentry_context
         {
-          active_job: job.class.name,
-          arguments: job.arguments,
-          scheduled_at: job.scheduled_at,
-          job_id: job.job_id,
-          provider_job_id: job.provider_job_id,
-          locale: job.locale
+          active_job: self.class.name,
+          arguments: arguments,
+          scheduled_at: scheduled_at,
+          job_id: job_id,
+          provider_job_id: provider_job_id,
+          locale: locale
         }
       end
     end
diff --git a/sentry-rails/spec/sentry/rails/activejob_spec.rb b/sentry-rails/spec/sentry/rails/activejob_spec.rb
@@ -35,6 +35,14 @@ class RescuedActiveJob < FailedWithExtraJob
   def rescue_callback(error); end
 end
 
+class ProblematicRescuedActiveJob < FailedWithExtraJob
+  rescue_from TestError, with: :rescue_callback
+
+  def rescue_callback(error)
+    raise "foo"
+  end
+end
+
 RSpec.describe "without Sentry initialized" do
   it "runs job" do
     expect { FailedJob.perform_now }.to raise_error(FailedJob::TestError)
@@ -190,10 +198,29 @@ def perform(event, hint)
 
   context 'using rescue_from' do
     it 'does not trigger Sentry' do
+      expect_any_instance_of(RescuedActiveJob).to receive(:rescue_callback).once.and_call_original
+
       expect { RescuedActiveJob.perform_now }.not_to raise_error
 
       expect(transport.events.size).to eq(0)
     end
+
+    context "with exception in rescue_from" do
+      it "reports both the original error and callback error" do
+        expect_any_instance_of(ProblematicRescuedActiveJob).to receive(:rescue_callback).once.and_call_original
+
+        expect { ProblematicRescuedActiveJob.perform_now }.to raise_error(RuntimeError)
+
+        expect(transport.events.size).to eq(1)
+
+        event = transport.events.first
+        exceptions_data = event.exception.to_hash[:values]
+
+        expect(exceptions_data.count).to eq(2)
+        expect(exceptions_data[0][:type]).to eq("FailedJob::TestError")
+        expect(exceptions_data[1][:type]).to eq("RuntimeError")
+      end
+    end
   end
 
   context "when we are using an adapter which has a specific integration" do
