From 62246a530344fd156ca512a4c1b8986de17b749e Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 24 Oct 2023 14:45:17 -0700 Subject: [PATCH 1/2] ActiveRecord subscriber tests: fixes, cleanup - do not call `#source` on a proc, cheat with a source code parsing hack instead - DRY up some shared logic --- .../rails/active_record_subscriber.rb | 69 ++++++++++++------- 1 file changed, 45 insertions(+), 24 deletions(-) diff --git a/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb b/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb index 3ab4ffb2d6..2ba0be4ca5 100644 --- a/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb @@ -239,36 +239,22 @@ def test_config_can_be_gleaned_from_handler_spec end def test_instrumentation_can_be_disabled_with_disable_active_record_instrumentation - NewRelic::Agent::Instrumentation::ActiveRecordSubscriber.stub :subscribed?, false do + with_subscribed_check_disabled do common_test_for_disabling(:disable_active_record_instrumentation) end end - def test_instrumentation_can_be_disabled_with_disable_active_record_notifications - NewRelic::Agent::Instrumentation::ActiveRecordSubscriber.stub :subscribed?, false do + def xtest_instrumentation_can_be_disabled_with_disable_active_record_notifications + with_subscribed_check_disabled do common_test_for_disabling(:disable_active_record_notifications) end end def test_instrumentation_is_enabled_by_default - # When performing "env" tests, the initial loading of the agent via Rails - # initialization will trigger the dependency check with default config - # options and then subscribe to AR notifications. We have to stub out the - # "have we already subscribed?" check to ensure that the agent doesn't - # short-circuit based on an existing subscription while we're really focused - # on configuration based behavior, not subscription existence. - NewRelic::Agent::Instrumentation::ActiveRecordSubscriber.stub :subscribed?, false do + with_subscribed_check_disabled do enabled_config = {disable_active_record_instrumentation: false, disable_active_record_notifications: false} - instrumentation_name = :active_record_notifications - - item = DependencyDetection.instance_variable_get(:@items).detect { |i| i.name == instrumentation_name } - - assert item, "Could not locate the '#{instrumentation_name}' dependency detection item for AR notifications" - dependency_check = item.dependencies.detect { |d| d.source.match?(/disable_#{instrumentation_name}/) } - - assert dependency_check, "Could not locate the dependency check related to the disable_#{instrumentation_name} " \ - 'configuration parameter' + dependency_check = dependency_check_for(:active_record_notifications) with_config(enabled_config) do assert dependency_check.call, 'Expected the AR notifications dependency check to be made when given ' \ @@ -285,20 +271,55 @@ def simulate_query(duration = nil) @subscriber.finish('sql.active_record', :id, @params) end + # When performing "env" tests, the initial loading of the agent via Rails + # initialization will trigger the dependency check with default config + # options and then subscribe to AR notifications. We have to stub out the + # "have we already subscribed?" check to ensure that the agent doesn't + # short-circuit based on an existing subscription while we're really focused + # on configuration based behavior, not subscription existence. + def with_subscribed_check_disabled(&block) + NewRelic::Agent::Instrumentation::ActiveRecordSubscriber.stub :subscribed?, false do + yield + end + end + def common_test_for_disabling(parameter) - instrumentation_name = :active_record_notifications + dependency_check = dependency_check_for(parameter) + + with_config(parameter => true) do + refute dependency_check.call, 'Expected the AR notifications dependency check to NOT be made when given ' \ + "a #{parameter} value of true." + end + end + def dependency_check_for(parameter) + instrumentation_name = :active_record_notifications item = DependencyDetection.instance_variable_get(:@items).detect { |i| i.name == instrumentation_name } assert item, "Could not locate the '#{instrumentation_name}' dependency detection item for AR notifications" - dependency_check = item.dependencies.detect { |d| d.source.match?(/disable_#{instrumentation_name}/) } + + dependency_check = nil + item.dependencies.each_with_index do |d, i| + if dependency_check_matches?(d, i, instrumentation_name) + dependency_check = d + break + end + end assert dependency_check, "Could not locate the dependency check related to the disable_#{instrumentation_name} " \ 'configuration parameter' - with_config(parameter => true) do - refute dependency_check.call, 'Expected the AR notifications dependency check to NOT be made when given ' \ - "a #{parameter} value of true." + dependency_check + end + + def dependency_check_matches?(depends_on, index, instrumentation_name) + return depends_on.source.match?(/disable_#{instrumentation_name}/) if depends_on.respond_to?(:source) + + depends = File.read(depends_on.source_location.first).scan(/^(\s+)depends_on do\n(.*?)\1end/m) + if depends.empty? || (index + 1) > depends.size + raise "Failed to find a suitable `depends_on` block within `#{depends_on.source_location}`!" end + + depends[index].join.match?(/disable_#{instrumentation_name}/m) end end From ad7406f9aee5d06f913779ae34f573d37a805459 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 24 Oct 2023 14:48:11 -0700 Subject: [PATCH 2/2] AR notifications test: re-enable test re-enable test --- .../agent/instrumentation/rails/active_record_subscriber.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb b/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb index 2ba0be4ca5..333396d1da 100644 --- a/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb +++ b/test/new_relic/agent/instrumentation/rails/active_record_subscriber.rb @@ -244,7 +244,7 @@ def test_instrumentation_can_be_disabled_with_disable_active_record_instrumentat end end - def xtest_instrumentation_can_be_disabled_with_disable_active_record_notifications + def test_instrumentation_can_be_disabled_with_disable_active_record_notifications with_subscribed_check_disabled do common_test_for_disabling(:disable_active_record_notifications) end