From 5877d7a1f561b41c83877e5a6acb464bae49f12b Mon Sep 17 00:00:00 2001 From: Giulio Giraldi Date: Thu, 16 Oct 2025 15:17:32 +0200 Subject: [PATCH 1/3] add sidekiq_notice_only_once configuration to ensure failing sidekiq jobs are notified only once. --- lib/new_relic/agent/instrumentation/sidekiq.rb | 8 +++++++- newrelic.yml | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/new_relic/agent/instrumentation/sidekiq.rb b/lib/new_relic/agent/instrumentation/sidekiq.rb index 8070e31797..b904d3510d 100644 --- a/lib/new_relic/agent/instrumentation/sidekiq.rb +++ b/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_notice_only_once] # 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_notice_only_once] + config.death_handlers << proc do |_, error| + NewRelic::Agent.notice_error(error) + end + end end end end diff --git a/newrelic.yml b/newrelic.yml index c87a33c1f4..e77a2f1b56 100644 --- a/newrelic.yml +++ b/newrelic.yml @@ -331,6 +331,10 @@ common: &default_settings # If true, disables Sidekiq instrumentation. # disable_sidekiq: false + # If true, when a sidekiq job fails only notify it after the last failing retry before going to the dead queue + # i.e. skip failures that will be retried + # sidekiq_notice_only_once: 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. From 72f7f321f6a28cd7b0bb6ee6131280f301fe3cfe Mon Sep 17 00:00:00 2001 From: Giulio Giraldi Date: Thu, 16 Oct 2025 16:55:33 +0200 Subject: [PATCH 2/3] default_source --- lib/new_relic/agent/configuration/default_source.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index dc1b49e3a6..5756088e38 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1411,6 +1411,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => 'If `true`, disables [Sidekiq instrumentation](/docs/agents/ruby-agent/background-jobs/sidekiq-instrumentation).' }, + :sidekiq_notice_only_once => { + :default => false, + :public => true, + :type => Boolean, + :allowed_from_server => false, + :description => 'If `true`, enables reporting only on last sidekiq retry [Sidekiq instrumentation](/docs/agents/ruby-agent/background-jobs/sidekiq-instrumentation).' + }, :disable_roda_auto_middleware => { :default => false, :public => true, From 246a0df1a1a64ccff1653dc1591d17b8c317ad1f Mon Sep 17 00:00:00 2001 From: Giulio Giraldi Date: Thu, 23 Oct 2025 13:46:00 +0200 Subject: [PATCH 3/3] adding specs --- .../sidekiq/sidekiq_instrumentation_test.rb | 4 + .../sidekiq/sidekiq_notice_only_once_test.rb | 76 ++++++++++++++++ .../suites/sidekiq_notice_only_once/Envfile | 18 ++++ .../config/newrelic.yml | 22 +++++ .../sidekiq_notice_only_once_enabled_test.rb | 70 +++++++++++++++ .../sidekiq_test_helpers.rb | 90 +++++++++++++++++++ 6 files changed, 280 insertions(+) create mode 100644 test/multiverse/suites/sidekiq/sidekiq_notice_only_once_test.rb create mode 100644 test/multiverse/suites/sidekiq_notice_only_once/Envfile create mode 100644 test/multiverse/suites/sidekiq_notice_only_once/config/newrelic.yml create mode 100644 test/multiverse/suites/sidekiq_notice_only_once/sidekiq_notice_only_once_enabled_test.rb create mode 100644 test/multiverse/suites/sidekiq_notice_only_once/sidekiq_test_helpers.rb diff --git a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb b/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb index 70076bcebe..4ba0e22b49 100644 --- a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb +++ b/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_notice_only_once_default_is_false + assert_equal false, NewRelic::Agent.config[:sidekiq_notice_only_once], 'Expected default value for sidekiq_notice_only_once to be false' + end end diff --git a/test/multiverse/suites/sidekiq/sidekiq_notice_only_once_test.rb b/test/multiverse/suites/sidekiq/sidekiq_notice_only_once_test.rb new file mode 100644 index 0000000000..d77c4de245 --- /dev/null +++ b/test/multiverse/suites/sidekiq/sidekiq_notice_only_once_test.rb @@ -0,0 +1,76 @@ +# 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' + +class SidekiqNoticeOnlyOnceTest < Minitest::Test + include SidekiqTestHelpers + + def test_sidekiq_notice_only_once_default_value + assert_equal false, NewRelic::Agent.config[:sidekiq_notice_only_once], + 'Expected sidekiq_notice_only_once default to be false' + end + + def test_sidekiq_notice_only_once_configuration_can_be_set_to_true + with_config(sidekiq_notice_only_once: true) do + assert NewRelic::Agent.config[:sidekiq_notice_only_once], + 'Expected sidekiq_notice_only_once to be true when configured' + end + end + + def test_sidekiq_notice_only_once_configuration_can_be_set_to_false + with_config(sidekiq_notice_only_once: false) do + refute NewRelic::Agent.config[:sidekiq_notice_only_once], + 'Expected sidekiq_notice_only_once to be false when configured' + end + end + + def test_error_handlers_registered_when_sidekiq_notice_only_once_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_notice_only_once is false' + end + + def test_death_handlers_not_registered_when_sidekiq_notice_only_once_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_notice_only_once is false' + end +end \ No newline at end of file diff --git a/test/multiverse/suites/sidekiq_notice_only_once/Envfile b/test/multiverse/suites/sidekiq_notice_only_once/Envfile new file mode 100644 index 0000000000..8c95f32e7a --- /dev/null +++ b/test/multiverse/suites/sidekiq_notice_only_once/Envfile @@ -0,0 +1,18 @@ +# 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] +] + +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) diff --git a/test/multiverse/suites/sidekiq_notice_only_once/config/newrelic.yml b/test/multiverse/suites/sidekiq_notice_only_once/config/newrelic.yml new file mode 100644 index 0000000000..b87048edf4 --- /dev/null +++ b/test/multiverse/suites/sidekiq_notice_only_once/config/newrelic.yml @@ -0,0 +1,22 @@ +--- +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 + # Enable sidekiq_notice_only_once to test death_handlers registration + sidekiq_notice_only_once: true diff --git a/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_notice_only_once_enabled_test.rb b/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_notice_only_once_enabled_test.rb new file mode 100644 index 0000000000..32d7b26ee3 --- /dev/null +++ b/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_notice_only_once_enabled_test.rb @@ -0,0 +1,70 @@ +# 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' + +class SidekiqNoticeOnlyOnceEnabledTest < Minitest::Test + include SidekiqTestHelpers + + def test_sidekiq_notice_only_once_is_enabled + assert NewRelic::Agent.config[:sidekiq_notice_only_once], + 'Expected sidekiq_notice_only_once to be true based on newrelic.yml configuration' + end + + def test_death_handlers_registered_when_sidekiq_notice_only_once_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_notice_only_once is true' + end + + def test_error_handlers_not_registered_when_sidekiq_notice_only_once_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_notice_only_once is true' + end + + def test_basic_job_execution_still_works + segment = run_job + + assert_predicate segment, :finished? + assert_predicate segment, :record_metrics? + assert segment.duration.is_a?(Float) + end +end diff --git a/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_test_helpers.rb b/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_test_helpers.rb new file mode 100644 index 0000000000..0d05468530 --- /dev/null +++ b/test/multiverse/suites/sidekiq_notice_only_once/sidekiq_test_helpers.rb @@ -0,0 +1,90 @@ +# 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 'sidekiq' +require 'sidekiq/cli' +require 'newrelic_rpm' + +class NRDeadEndJob + # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported + if Sidekiq::VERSION.split('.').first.to_i >= 6 + include Sidekiq::Job + else + include Sidekiq::Worker + end + + sidekiq_options retry: 5 + + COMPLETION_VAR = :@@nr_job_complete + ERROR_MESSAGE = 'kaboom' + + def perform(*args) + raise ERROR_MESSAGE if args.first.is_a?(Hash) && args.first['raise_error'] + ensure + self.class.class_variable_set(COMPLETION_VAR, true) + end +end + +module SidekiqTestHelpers + def run_job(*args) + segments = nil + in_transaction do |txn| + NRDeadEndJob.perform_async(*args) + process_queued_jobs + segments = txn.segments.select { |s| s.name.eql?('Nested/OtherTransaction/SidekiqJob/NRDeadEndJob/perform') } + end + + assert_equal 1, segments.size, "Expected to find a single Sidekiq job segment, found #{segments.size}" + segments.first + end + + def run_job_and_get_attributes(*args) + run_job(*args).attributes.agent_attributes_for(NewRelic::Agent::AttributeFilter::DST_TRANSACTION_TRACER) + end + + def process_queued_jobs + NRDeadEndJob.class_variable_set(NRDeadEndJob::COMPLETION_VAR, false) + config = cli.instance_variable_defined?(:@config) ? cli.instance_variable_get(:@config) : Sidekiq.options + + # TODO: MAJOR VERSION - remove this when Sidekiq v5 is no longer supported + require 'sidekiq/launcher' if Sidekiq::VERSION.split('.').first.to_i < 6 + + launcher = Sidekiq::Launcher.new(config) + launcher.run + Timeout.timeout(5) do + sleep 0.01 until NRDeadEndJob.class_variable_get(NRDeadEndJob::COMPLETION_VAR) + end + + # TODO: MAJOR VERSION - Sidekiq v7 is fine with launcher.stop, but v5 and v6 + # need the Manager#quiet call + if launcher.instance_variable_defined?(:@manager) + launcher.instance_variable_get(:@manager).quiet + else + launcher.stop + end + end + + def cli + @@cli ||= begin + cli = Sidekiq::CLI.instance + cli.parse(['--require', File.absolute_path(__FILE__), '--queue', 'default,1']) + cli.logger.instance_variable_get(:@logdev).instance_variable_set(:@dev, File.new('/dev/null', 'w')) + ensure_sidekiq_config(cli) + cli + end + end + + def ensure_sidekiq_config(cli) + return unless Sidekiq::VERSION.split('.').first.to_i >= 7 + return unless cli.respond_to?(:config) + return unless cli.config.nil? + + require 'sidekiq/config' + cli.instance_variable_set(:@config, ::Sidekiq::Config.new) + end + + def flatten(object) + NewRelic::Agent::AttributeProcessing.flatten_and_coerce(object, 'job.sidekiq.args') + end +end