Merge pull request #3317 from newrelic/sidekiq_retry_backup

Add `sidekiq.ignore_retry_errors` to defer error reporting

Author: hannahramadan

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## dev
 
+- **Feature: Add sidekiq.ignore_retry_errors configuration option**
+
+  A new configuration option, `sidekiq.ignore_retry_errors`, has been added to control if Sidekiq job retries are captured. Retry errors are captured by default, but now if `sidekiq.ignore_retry_errors` is set to `true`, the agent will ignore exceptions raised during Sidekiq's retry attempts and will only report the error if the job permanently fails. Thank you [DonGiulio](https://github.com/DonGiulio) for recognizing this improvement and contributing a solution. [PR#3317](https://github.com/newrelic/newrelic-ruby-agent/pull/3317)
+
 - **Feature: Deprecation notice for recording deployments using Capistrano**
 
   Sending application deployment information using a Capistrano recipe is deprecated and will be removed in agent version 10.0.0. For recording deployments, please see our guide to [Change Tracking](https://docs.newrelic.com/docs/change-tracking/change-tracking-introduction/) for a list of available options.

lib/new_relic/agent/configuration/default_source.rb

@@ -2140,6 +2140,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
             argument array elements and job argument scalars will be excluded.
           SIDEKIQ_ARGS_EXCLUDE
         },
+        :'sidekiq.ignore_retry_errors' => {
+          :default => false,
+          :public => true,
+          :type => Boolean,
+          :allowed_from_server => false,
+          :description => %Q(If `true`, the agent will ignore exceptions raised during Sidekiq's retry attempts and will only report the error if the job permanently fails.)
+        },
         # Slow SQL
         :'slow_sql.enabled' => {
           :default => value_of(:'transaction_tracer.enabled'),

lib/new_relic/agent/instrumentation/sidekiq.rb

@@ -40,13 +40,19 @@
         end
       end
 
-      if config.respond_to?(:error_handlers)
+      if config.respond_to?(:error_handlers) && !NewRelic::Agent.config[:'sidekiq.ignore_retry_errors']
         # Sidekiq 3.0.0 - 7.1.4 expect error_handlers to have 2 arguments
         # Sidekiq 7.1.5+ expect error_handlers to have 3 arguments
         config.error_handlers << proc do |error, _ctx, *_|
           NewRelic::Agent.notice_error(error)
         end
       end
+
+      if config.respond_to?(:death_handlers) && NewRelic::Agent.config[:'sidekiq.ignore_retry_errors']
+        config.death_handlers << proc do |_, error|
+          NewRelic::Agent.notice_error(error)
+        end
+      end
     end
   end
 end

newrelic.yml

@@ -331,6 +331,10 @@ common: &default_settings
   # If true, disables Sidekiq instrumentation.
   # disable_sidekiq: false
 
+  # 'If true, the agent will ignore exceptions raised during Sidekiq's retry attempts and will only report the 
+  # error if the job permanently fails.
+  # sidekiq.ignore_retry_errors: false
+
   # If true, disables agent middleware for Sinatra. This middleware is responsible
   # for advanced feature support such as cross application tracing, page load
   # timing, and error collection.

test/multiverse/lib/multiverse/runner.rb

@@ -110,7 +110,7 @@ def execute_suites(filter, opts)
 
       # these need services running in github actions, so they are separated
       'services_1' => %w[mongo bunny],
-      'services_2' => %w[redis sidekiq sidekiq_delay_extensions memcache],
+      'services_2' => %w[redis sidekiq sidekiq_delay_extensions sidekiq_ignore_retry_errors_enabled memcache],
       'services_kafka' => %w[rdkafka ruby_kafka],
       'services_elasticsearch' => %w[elasticsearch],
       'services_mysql_pg' => %w[active_record active_record_pg],

test/multiverse/suites/sidekiq/sidekiq_ignore_retry_errors_disabled_test.rb

@@ -0,0 +1,79 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require_relative 'sidekiq_test_helpers'
+
+# On startup, Sidekiq instrumentation registers error and death handlers
+# based on the value of the 'sidekiq.ignore_retry_errors'. Because of this,
+# we need to have separate enabled/disabled test suites to test both cases.
+class SidekiqIgnoreRetryErrorsTest < Minitest::Test
+  include SidekiqTestHelpers
+
+  def test_sidekiq_ignore_retry_errors_default_value
+    refute NewRelic::Agent.config[:'sidekiq.ignore_retry_errors'],
+      'Expected sidekiq.ignore_retry_errors default to be false'
+  end
+
+  def test_sidekiq_ignore_retry_errors_configuration_can_be_set_to_true
+    with_config(:'sidekiq.ignore_retry_errors' => true) do
+      assert NewRelic::Agent.config[:'sidekiq.ignore_retry_errors'],
+        'Expected sidekiq.ignore_retry_errors to be true when configured'
+    end
+  end
+
+  def test_sidekiq_ignore_retry_errors_configuration_can_be_set_to_false
+    with_config(:'sidekiq.ignore_retry_errors' => false) do
+      refute NewRelic::Agent.config[:'sidekiq.ignore_retry_errors'],
+        'Expected sidekiq.ignore_retry_errors to be false when configured'
+    end
+  end
+
+  def test_error_handlers_registered_when_sidekiq_ignore_retry_errors_is_false
+    # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported
+    skip 'Test requires Sidekiq v6+' unless Sidekiq::VERSION.split('.').first.to_i >= 6
+
+    config = if Sidekiq::VERSION.split('.').first.to_i >= 7
+      Sidekiq.default_configuration
+    else
+      Sidekiq
+    end
+
+    error_handlers = if config.respond_to?(:error_handlers)
+      config.error_handlers
+    else
+      config[:error_handlers] || []
+    end
+
+    nr_error_handler_found = error_handlers.any? do |handler|
+      handler.is_a?(Proc) && handler.source_location&.first&.include?('newrelic')
+    end
+
+    assert nr_error_handler_found,
+      'Expected NewRelic error_handler to be registered when sidekiq.ignore_retry_errors is false'
+  end
+
+  def test_death_handlers_not_registered_when_sidekiq_ignore_retry_errors_is_false
+    # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported
+    skip 'Test requires Sidekiq v6+' unless Sidekiq::VERSION.split('.').first.to_i >= 6
+
+    config = if Sidekiq::VERSION.split('.').first.to_i >= 7
+      Sidekiq.default_configuration
+    else
+      Sidekiq
+    end
+
+    death_handlers = if config.respond_to?(:death_handlers)
+      config.death_handlers
+    else
+      config[:death_handlers] || []
+    end
+
+    nr_death_handler_found = death_handlers.any? do |handler|
+      handler.is_a?(Proc) && handler.source_location&.first&.include?('newrelic')
+    end
+
+    refute nr_death_handler_found,
+      'Expected NewRelic death_handler to NOT be registered when sidekiq.ignore_retry_errors is false'
+  end
+end

test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb

@@ -70,4 +70,8 @@ def test_works_with_perform_inline
       assert_equal 1, segments.size, "Expected to find a single Sidekiq job segment, found #{segments.size}"
     end
   end
+
+  def test_sidekiq_ignore_retry_errors_default_is_false
+    refute NewRelic::Agent.config[:'sidekiq.ignore_retry_errors'], 'Expected default value for sidekiq.ignore_retry_errors to be false'
+  end
 end

test/multiverse/suites/sidekiq_ignore_retry_errors_enabled/Envfile

@@ -0,0 +1,19 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+SIDEKIQ_VERSIONS = [
+  [nil, 3.2],
+  ['7.3.9', 2.7],
+  ['6.4.0', 2.5],
+  ['5.0.3', 2.4, 2.5]
+]
+
+def gem_list(sidekiq_version = nil)
+  <<-RB
+    gem 'sidekiq'#{sidekiq_version}
+    gem 'newrelic_rpm', :require => false, :path => File.expand_path('../../../../')
+  RB
+end
+
+create_gemfiles(SIDEKIQ_VERSIONS)

test/multiverse/suites/sidekiq_ignore_retry_errors_enabled/config/newrelic.yml

@@ -0,0 +1,21 @@
+---
+development:
+  error_collector:
+    enabled: true
+  apdex_t: 0.5
+  monitor_mode: true
+  license_key: bootstrap_newrelic_admin_license_key_000
+  app_name: test
+  ca_bundle_path: ../../../config/test.cert.crt
+  app_name: test
+  host: localhost
+  api_host: localhost
+  port: <%= $collector && $collector.port %>
+  transaction_tracer:
+    record_sql: obfuscated
+    enabled: true
+    stack_trace_threshold: 0.5
+    transaction_threshold: 1.0
+  capture_params: false
+  log_file_path: log
+  sidekiq.ignore_retry_errors: true

test/multiverse/suites/sidekiq_ignore_retry_errors_enabled/sidekiq_ignore_retry_errors_enabled_test.rb

@@ -0,0 +1,68 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+require_relative 'sidekiq_test_helpers'
+
+# On startup, Sidekiq instrumentation registers error and death handlers
+# based on the value of the 'sidekiq.ignore_retry_errors'. Because of this,
+# we need to have separate enabled/disabled test suites to test both cases.
+class SidekiqIgnoreRetryErrorEnabledTest < Minitest::Test
+  include SidekiqTestHelpers
+
+  def test_error_handlers_not_registered_when_sidekiq_ignore_retry_errors_is_true
+    # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported
+    skip 'Test requires Sidekiq v6+' unless Sidekiq::VERSION.split('.').first.to_i >= 6
+
+    config = if Sidekiq::VERSION.split('.').first.to_i >= 7
+      Sidekiq.default_configuration
+    else
+      Sidekiq
+    end
+
+    error_handlers = if config.respond_to?(:error_handlers)
+      config.error_handlers
+    else
+      config[:error_handlers] || []
+    end
+
+    nr_error_handler_found = error_handlers.any? do |handler|
+      handler.is_a?(Proc) && handler.source_location&.first&.include?('newrelic')
+    end
+
+    refute nr_error_handler_found,
+      'Expected NewRelic error_handler to NOT be registered when sidekiq.ignore_retry_errors is true'
+  end
+
+  def test_death_handlers_registered_when_sidekiq_ignore_retry_errors_is_true
+    # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported
+    skip 'Test requires Sidekiq v6+' unless Sidekiq::VERSION.split('.').first.to_i >= 6
+
+    config = if Sidekiq::VERSION.split('.').first.to_i >= 7
+      Sidekiq.default_configuration
+    else
+      Sidekiq
+    end
+
+    death_handlers = if config.respond_to?(:death_handlers)
+      config.death_handlers
+    else
+      config[:death_handlers] || []
+    end
+
+    nr_death_handler_found = death_handlers.any? do |handler|
+      handler.is_a?(Proc) && handler.source_location&.first&.include?('newrelic')
+    end
+
+    assert nr_death_handler_found,
+      'Expected NewRelic death_handler to be registered when sidekiq.ignore_retry_errors is true'
+  end
+
+  def test_basic_job_execution_still_works
+    segment = run_job
+
+    assert_predicate segment, :finished?
+    assert_predicate segment, :record_metrics?
+    assert_kind_of Float, segment.duration
+  end
+end