From 31dc0efe983394dd149f7106e9d33a42a4a64e73 Mon Sep 17 00:00:00 2001 From: fallwith Date: Fri, 12 May 2023 17:21:49 -0700 Subject: [PATCH] instrumentation config: use :unsatisfied With [1930](https://github.com/newrelic/newrelic-ruby-agent/pull/1930/) the agent will now automatically convert any configured instrumentation value of `:auto` into either `:prepend` or `:chain` once the determination of which approach to take has been dynamically determined. This will allow the agent's config to be inspected after the `:prepend` / `:chain` determination is made to see what `:auto` actually becomes under the hood. But 1930 left an edge case gap open in the form of not allowing the configuration to convey instrumentation with unsatisfied dependencies. If a config value is set to `:prepend` or `:chain` in the configuration (overriding the default `:auto` value) and a dependency is unsatisfied (the library to instrument is not found on the system, it is of an unsupported version, etc.) then the value would end up staying as `:prepend` or `:chain` and incorrectly convey to anyone inspecting the config that a certain method of instrumentation was used when in actuality no instrumentation was performed at all. Now, if an instrumentation's dependencies are unsatisfied, the corresponding configuration value will be updated with a value of `:unsatisfied`. --- lib/new_relic/agent/configuration/manager.rb | 5 +- lib/new_relic/dependency_detection.rb | 6 ++ .../agent/configuration/manager_test.rb | 21 +++++ .../agent/local_log_decorator_test.rb | 1 + test/new_relic/dependency_detection_test.rb | 86 ++++++++++++++++++- 5 files changed, 113 insertions(+), 6 deletions(-) diff --git a/lib/new_relic/agent/configuration/manager.rb b/lib/new_relic/agent/configuration/manager.rb index 3dc77b35ad..66ffe29398 100644 --- a/lib/new_relic/agent/configuration/manager.rb +++ b/lib/new_relic/agent/configuration/manager.rb @@ -15,7 +15,7 @@ module NewRelic module Agent module Configuration class Manager - DEPENDENCY_DETECTION_VALUES = %i[prepend chain].freeze + DEPENDENCY_DETECTION_VALUES = %i[prepend chain unsatisfied].freeze # Defining these explicitly saves object allocations that we incur # if we use Forwardable and def_delegators. @@ -366,7 +366,8 @@ def reset_cache preserved = @cache.select { |_k, v| DEPENDENCY_DETECTION_VALUES.include?(v) } new_cache - preserved.each { |k, v| @cache[k] = v unless @cache[k] && @cache[k] != 'auto' } + preserved.each { |k, v| @cache[k] = v } + @cache end diff --git a/lib/new_relic/dependency_detection.rb b/lib/new_relic/dependency_detection.rb index eb86f2e0d4..5cd93f705c 100644 --- a/lib/new_relic/dependency_detection.rb +++ b/lib/new_relic/dependency_detection.rb @@ -27,6 +27,8 @@ def detect! @items.each do |item| if item.dependencies_satisfied? item.execute + else + item.configure_as_unsatisfied unless item.disabled_configured? end end end @@ -62,6 +64,10 @@ def dependencies_satisfied? !executed and check_dependencies end + def configure_as_unsatisfied + NewRelic::Agent.config.instance_variable_get(:@cache)[config_key] = :unsatisfied + end + def source_location_for(klass, method_name) Object.instance_method(:method).bind(klass.allocate).call(method_name).source_location.to_s end diff --git a/test/new_relic/agent/configuration/manager_test.rb b/test/new_relic/agent/configuration/manager_test.rb index aa18c67456..c9648070fb 100644 --- a/test/new_relic/agent/configuration/manager_test.rb +++ b/test/new_relic/agent/configuration/manager_test.rb @@ -509,6 +509,27 @@ def test_auto_determined_values_stay_cached end end + def test_unsatisfied_values_stay_cached + name = :tears_of_the_kingdom + + dd = DependencyDetection.defer do + named(name) + + # guarantee the instrumentation's dependencies are unsatisfied + depends_on { return false } + executes { use_prepend? } + end + + key = :"instrumentation.#{name}" + with_config(key => 'prepend') do + DependencyDetection.detect! + + @manager.replace_or_add_config(ServerSource.new({})) + + assert_equal :unsatisfied, @manager.instance_variable_get(:@cache)[key] + end + end + private def assert_parsed_labels(expected) diff --git a/test/new_relic/agent/local_log_decorator_test.rb b/test/new_relic/agent/local_log_decorator_test.rb index c0b44cdbfd..6846f85c20 100644 --- a/test/new_relic/agent/local_log_decorator_test.rb +++ b/test/new_relic/agent/local_log_decorator_test.rb @@ -20,6 +20,7 @@ def setup :'instrumentation.logger' => 'auto' } NewRelic::Agent.config.add_config_for_testing(@enabled_config) + NewRelic::Agent.config.send(:new_cache) end def teardown diff --git a/test/new_relic/dependency_detection_test.rb b/test/new_relic/dependency_detection_test.rb index 341f4f2348..955be1dec7 100644 --- a/test/new_relic/dependency_detection_test.rb +++ b/test/new_relic/dependency_detection_test.rb @@ -8,6 +8,7 @@ class DependencyDetectionTest < Minitest::Test def setup @original_items = DependencyDetection.instance_variable_get(:@items) DependencyDetection.instance_variable_set(:@items, []) + NewRelic::Agent.config.send(:new_cache) end def teardown @@ -132,7 +133,7 @@ def test_config_defaults_to_auto assert_equal :auto, setting end - def test_config_disabling + def test_config_disabling_with_disabled executed = false dd = DependencyDetection.defer do @@ -149,6 +150,15 @@ def test_config_disabling refute dd.allowed_by_config? refute executed end + end + + def test_config_disabling_with_enabled + executed = false + + dd = DependencyDetection.defer do + named(:testing) + executes { executed = true } + end with_config(:'instrumentation.testing' => 'enabled') do executed = false @@ -159,8 +169,17 @@ def test_config_disabling assert_predicate dd, :allowed_by_config? assert executed end + end + + # TODO: MAJOR VERSION - Deprecated! + def test_config_disabling_with_disable_testing + executed = false + + dd = DependencyDetection.defer do + named(:testing) + executes { executed = true } + end - # TODO: MAJOR VERSION - Deprecated! with_config(:disable_testing => true) do executed = false DependencyDetection.detect! @@ -172,7 +191,7 @@ def test_config_disabling end end - def test_config_enabling + def test_config_enabling_with_enabled executed = false dd = DependencyDetection.defer do @@ -189,6 +208,15 @@ def test_config_enabling assert executed assert_predicate dd, :use_prepend? end + end + + def test_config_enabling_with_auto + executed = false + + dd = DependencyDetection.defer do + named(:testing) + executes { executed = true } + end with_config(:'instrumentation.testing' => 'auto') do DependencyDetection.detect! @@ -197,6 +225,15 @@ def test_config_enabling refute dd.deprecated_disabled_configured? assert_predicate dd, :use_prepend? end + end + + def test_config_enabling_with_prepend + executed = false + + dd = DependencyDetection.defer do + named(:testing) + executes { executed = true } + end with_config(:'instrumentation.testing' => 'prepend') do DependencyDetection.detect! @@ -205,6 +242,15 @@ def test_config_enabling refute dd.deprecated_disabled_configured? assert_predicate dd, :use_prepend? end + end + + def test_config_enabling_with_chain + executed = false + + dd = DependencyDetection.defer do + named(:testing) + executes { executed = true } + end with_config(:'instrumentation.testing' => 'chain') do DependencyDetection.detect! @@ -215,7 +261,7 @@ def test_config_enabling end end - def test_config_prepend + def test_config_prepend_with_default_config dd = DependencyDetection.defer do named(:testing) executes { true } @@ -227,6 +273,13 @@ def test_config_prepend assert_equal :auto, dd.config_value assert_predicate dd, :use_prepend? end + end + + def test_config_prepend_with_prepend + dd = DependencyDetection.defer do + named(:testing) + executes { true } + end with_config(:'instrumentation.testing' => 'prepend') do DependencyDetection.detect! @@ -234,6 +287,13 @@ def test_config_prepend assert_equal :prepend, dd.config_value assert_predicate dd, :use_prepend? end + end + + def test_config_prepend_with_disabled + dd = DependencyDetection.defer do + named(:testing) + executes { true } + end with_config(:'instrumentation.testing' => 'disabled') do DependencyDetection.detect! @@ -416,4 +476,22 @@ def test_auto_is_replaced_by_chain_when_chain_is_used assert_equal :chain, dd.config_value end end + + # confirm that :auto/:chain/:prepend becomes :unsatisfied when the detection + # logic finds that the library is either missing or known to not be supported + def test_prepend_is_replaced_by_unsatisfied_when_appropriate + name = :calm_calm_belong + + dd = DependencyDetection.defer do + named(name) + depends_on { return false } # unsatisfied + executes { use_prepend? } + end + + with_config(:"instrumentation.#{name}" => 'prepend') do + DependencyDetection.detect! + + assert_equal :unsatisfied, dd.config_value + end + end end