diff --git a/CHANGELOG.md b/CHANGELOG.md index a45508a20e..615ecdf53d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,9 @@ ## dev - -Version adds a configuration option to associate the AWS account ID with the DynamoDB calls from the AWS SDK, updates View Componment instrumentation to use a default metric name when one is unavaliable, and resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem. - +Version updates View Componment instrumentation to use a default metric name when one is unavailable, adds a configuration option to associate the AWS account ID with the DynamoDB calls from the AWS SDK, resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem, resolves a bug in the ruby-kafka instrumentation, and fixes a bug with Grape instrumentation. + + - **Feature: New configuration option cloud.aws.account_id** A new configuration option has been added, `cloud.aws.account_id`, that will allow New Relic to provide more details about certain calls made using the AWS SDK. Currently, the DynamoDB instrumentation is the only instrumentation that will make use of this configuration option, but this will be used in future instrumentation as well. [PR#2904](https://github.com/newrelic/newrelic-ruby-agent/pull/2904) @@ -17,6 +17,14 @@ Version adds a configuration option to associate the AWS account ID with t Due to version differences between the rdkafka gem and karafka-rdkafka gem, the agent could encounter an error when it tried to install rdkafka instrumentation. This has now been resolved. Thank you to @krisdigital for bringing this issue to our attention. [PR#2880](https://github.com/newrelic/newrelic-ruby-agent/pull/2880) +- **Bugfix: Stop calling deprecated all_specs method to check for the presence of newrelic-grape** + + In 9.14.0, we released a fix for calls to the deprecated `Bundler.rubygems.all_specs`, but the fix fell short for the agent's Grape instrumentation and deprecation warnings could still be raised. The condition has been simplified and deprecation warnings should no longer be raised. Thank you, [@excelsior](https://github.com/excelsior) for bringing this to our attention. [Issue#](https://github.com/newrelic/newrelic-ruby-agent/issues/2885) [PR#2906](https://github.com/newrelic/newrelic-ruby-agent/pull/2906) + +- **Bugfix: Instrumentation errors when using the ruby-kafka gem** + + Kafka::Consumer#each_message takes keyword arguments, while the prepended method is defined with a single splat positional argument. In Ruby >= 3.0, this signature mismatch raises an ArgumentError. Thank you [@patrickarnett](https://github.com/patrickarnett) for providing this bugfix. [PR#2915](https://github.com/newrelic/newrelic-ruby-agent/pull/2915) + ## v9.14.0 diff --git a/lib/new_relic/agent/instrumentation/grape.rb b/lib/new_relic/agent/instrumentation/grape.rb index 1424ffa06b..75bae0a0df 100644 --- a/lib/new_relic/agent/instrumentation/grape.rb +++ b/lib/new_relic/agent/instrumentation/grape.rb @@ -19,9 +19,7 @@ depends_on do begin - if defined?(Bundler) && - ((Bundler.rubygems.respond_to?(:installed_specs) && Bundler.rubygems.installed_specs.map(&:name).include?('newrelic-grape')) || - Bundler.rubygems.all_specs.map(&:name).include?('newrelic-grape')) + if NewRelic::Helper.rubygems_specs.map(&:name).include?('newrelic-grape') NewRelic::Agent.logger.info('Not installing New Relic supported Grape instrumentation because the third party newrelic-grape gem is present') false else diff --git a/lib/new_relic/agent/instrumentation/ruby_kafka/prepend.rb b/lib/new_relic/agent/instrumentation/ruby_kafka/prepend.rb index 66cec78a33..063fb6e511 100644 --- a/lib/new_relic/agent/instrumentation/ruby_kafka/prepend.rb +++ b/lib/new_relic/agent/instrumentation/ruby_kafka/prepend.rb @@ -20,10 +20,20 @@ module RubyKafkaConsumer module Prepend include NewRelic::Agent::Instrumentation::RubyKafka - def each_message(*args) - super do |message| - each_message_with_new_relic(message) do - yield(message) + if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3') + def each_message(**args) + super do |message| + each_message_with_new_relic(message) do + yield(message) + end + end + end + else + def each_message(*args) + super do |message| + each_message_with_new_relic(message) do + yield(message) + end end end end diff --git a/lib/new_relic/control/frameworks/rails4.rb b/lib/new_relic/control/frameworks/rails4.rb index 23d403dd69..b8545f92df 100644 --- a/lib/new_relic/control/frameworks/rails4.rb +++ b/lib/new_relic/control/frameworks/rails4.rb @@ -9,11 +9,7 @@ class Control module Frameworks class Rails4 < NewRelic::Control::Frameworks::Rails3 def rails_gem_list - if Bundler.rubygems.respond_to?(:installed_specs) - Bundler.rubygems.installed_specs.map { |gem| "#{gem.name} (#{gem.version})" } - else - Bundler.rubygems.all_specs.map { |gem| "#{gem.name} (#{gem.version})" } - end + NewRelic::Helper.rubygems_specs.map { |gem| "#{gem.name} (#{gem.version})" } end def append_plugin_list diff --git a/lib/new_relic/dependency_detection.rb b/lib/new_relic/dependency_detection.rb index 5cd93f705c..d9c7dff358 100644 --- a/lib/new_relic/dependency_detection.rb +++ b/lib/new_relic/dependency_detection.rb @@ -25,11 +25,9 @@ def defer(&block) def detect! @items.each do |item| - if item.dependencies_satisfied? - item.execute - else - item.configure_as_unsatisfied unless item.disabled_configured? - end + next if item.executed || item.disabled_configured? + + item.dependencies_satisfied? ? item.execute : item.configure_as_unsatisfied end end @@ -65,6 +63,13 @@ def dependencies_satisfied? end def configure_as_unsatisfied + # TODO: currently using :unsatisfied for Padrino will clobber the value + # already set for Sinatra, so skip Padrino and circle back with a + # new Padrino specific solution in the future. + # + # https://github.com/newrelic/newrelic-ruby-agent/issues/2912 + return if name == :padrino + NewRelic::Agent.config.instance_variable_get(:@cache)[config_key] = :unsatisfied end diff --git a/lib/new_relic/environment_report.rb b/lib/new_relic/environment_report.rb index 1661bebeb6..301fe83c0f 100644 --- a/lib/new_relic/environment_report.rb +++ b/lib/new_relic/environment_report.rb @@ -44,11 +44,7 @@ def self.registered_reporters=(logic) #################################### report_on('Gems') do begin - if Bundler.rubygems.respond_to?(:installed_specs) - Bundler.rubygems.installed_specs.map { |gem| "#{gem.name}(#{gem.version})" } - else - Bundler.rubygems.all_specs.map { |gem| "#{gem.name}(#{gem.version})" } - end + NewRelic::Helper.rubygems_specs.map { |gem| "#{gem.name}(#{gem.version})" } rescue # There are certain rubygem, bundler, rails combinations (e.g. gem # 1.6.2, rails 2.3, bundler 1.2.3) where the code above throws an error diff --git a/lib/new_relic/helper.rb b/lib/new_relic/helper.rb index 7d17c89908..3f89b9e36b 100644 --- a/lib/new_relic/helper.rb +++ b/lib/new_relic/helper.rb @@ -82,5 +82,20 @@ def executable_in_path?(executable) File.exist?(executable_path) && File.file?(executable_path) && File.executable?(executable_path) end end + + # Bundler version 2.5.12 deprecated all_specs and added installed_specs. + # To support newer Bundler versions, try to use installed_specs first, + # then fall back to all_specs. + # All callers expect this to be an array, so return an array if Bundler isn't defined + # @api private + def rubygems_specs + return [] unless defined?(Bundler) + + if Bundler.rubygems.respond_to?(:installed_specs) + Bundler.rubygems.installed_specs + else + Bundler.rubygems.all_specs + end + end end end diff --git a/lib/new_relic/language_support.rb b/lib/new_relic/language_support.rb index 140d2455d2..31377e831a 100644 --- a/lib/new_relic/language_support.rb +++ b/lib/new_relic/language_support.rb @@ -90,11 +90,7 @@ def snakeize(string) def bundled_gem?(gem_name) return false unless defined?(Bundler) - if Bundler.rubygems.respond_to?(:installed_specs) - Bundler.rubygems.installed_specs.map(&:name).include?(gem_name) - else - Bundler.rubygems.all_specs.map(&:name).include?(gem_name) - end + NewRelic::Helper.rubygems_specs.map(&:name).include?(gem_name) rescue => e ::NewRelic::Agent.logger.info("Could not determine if third party #{gem_name} gem is installed", e) false diff --git a/test/multiverse/suites/sinatra/sinatra_test_cases.rb b/test/multiverse/suites/sinatra/sinatra_test_cases.rb index c7a04e98fe..e3b629d801 100644 --- a/test/multiverse/suites/sinatra/sinatra_test_cases.rb +++ b/test/multiverse/suites/sinatra/sinatra_test_cases.rb @@ -128,6 +128,13 @@ def test_with_regex_pattern assert_metrics_recorded(["Controller/Sinatra/#{app_name}/#{regex_segment}"]) end + def test_that_the_actively_configured_instrumentation_is_not_marked_as_unsatsfied + get('/pass') + + assert_equal 200, last_response.status + assert_includes(%w[chain prepend], NewRelic::Agent.config[:'instrumentation.sinatra'].to_s) + end + # https://support.newrelic.com/tickets/31061 def test_precondition_not_over_called get('/precondition') diff --git a/test/new_relic/agent/configuration/manager_test.rb b/test/new_relic/agent/configuration/manager_test.rb index cc2bc78349..7d81efedba 100644 --- a/test/new_relic/agent/configuration/manager_test.rb +++ b/test/new_relic/agent/configuration/manager_test.rb @@ -492,20 +492,22 @@ def test_apply_transformations_reraises_errors end def test_auto_determined_values_stay_cached - name = :knockbreck_manse + with_config(:'security.agent.enabled' => true) do + name = :knockbreck_manse - DependencyDetection.defer do - named(name) - executes { use_prepend? } - end + DependencyDetection.defer do + named(name) + executes { use_prepend? } + end - key = :"instrumentation.#{name}" - with_config(key => 'auto') do - DependencyDetection.detect! + key = :"instrumentation.#{name}" + with_config(key => 'auto') do + DependencyDetection.detect! - @manager.replace_or_add_config(ServerSource.new({})) + @manager.replace_or_add_config(ServerSource.new({})) - assert_equal :prepend, @manager.instance_variable_get(:@cache)[key] + assert_equal :prepend, @manager.instance_variable_get(:@cache)[key] + end end end diff --git a/test/new_relic/dependency_detection_test.rb b/test/new_relic/dependency_detection_test.rb index b4223340a5..d60a0cbadc 100644 --- a/test/new_relic/dependency_detection_test.rb +++ b/test/new_relic/dependency_detection_test.rb @@ -488,4 +488,21 @@ def test_prepend_is_replaced_by_unsatisfied_when_appropriate assert_equal :unsatisfied, dd.config_value end end + + def test_already_executed_items_are_not_executed_again + unexecuted = Minitest::Mock.new + unexecuted.expect :executed, false + unexecuted.expect :dependencies_satisfied?, true + unexecuted.expect :disabled_configured?, false + unexecuted.expect :execute, -> { execution_took_place = true } + executed = Minitest::Mock.new + executed.expect :executed, true + unexecuted.expect :disabled_configured?, false + + DependencyDetection.instance_variable_set(:@items, [unexecuted, executed]) + DependencyDetection.detect! + + unexecuted.verify + executed.verify + end end diff --git a/test/new_relic/helper_test.rb b/test/new_relic/helper_test.rb index e47a838a40..2ec6dcef94 100644 --- a/test/new_relic/helper_test.rb +++ b/test/new_relic/helper_test.rb @@ -77,4 +77,28 @@ def test_run_command_sad_exception NewRelic::Helper.run_command('executable that existed at detection time but is not there now') end end + + # + # rubygems_specs + # + def test_rubygems_specs_returns_empty_array_without_bundler + stub(:defined?, nil, ['Bundler']) do + result = NewRelic::Helper.rubygems_specs + + assert_instance_of Array, result + assert_empty Array, result + end + end + + def test_rubygems_specs_works_with_all_specs_when_installed_specs_is_absent + Bundler.rubygems.stub(:respond_to?, nil) do + assert_equal Bundler.rubygems.all_specs, NewRelic::Helper.rubygems_specs + end + end + + def test_rubygems_specs_works_with_installed_specs + skip 'running a version of Bundler that has not defined installed_specs' unless Bundler.rubygems.respond_to?(:installed_specs) + + assert_equal Bundler.rubygems.installed_specs, NewRelic::Helper.rubygems_specs + end end