Merge pull request #2699 from newrelic/dev

Release 9.10.2

Author: kaylareopelle

.github/workflows/ci.yml

@@ -357,4 +357,4 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
           resultPath: lib/coverage_results/.last_run.json
           failedThreshold: 93.5
-          failedThresholdBranch: 71.5
+          failedThresholdBranch: 50

CHANGELOG.md

@@ -1,5 +1,23 @@
 # New Relic Ruby Agent Release Notes
 
+## v9.10.2
+
+Version 9.10.2 fixes a bug related to the new DynamoDB instrumentation and removes `Rails::Command::RakeCommand` from the default list of denylisted constants.
+
+- **Bugfix: DynamoDB instrumentation logging errors when trying to get account_id**
+
+    When trying to access data needed to add the `account_id` to the DynamoDB span, the agent encountered an error when certain credentials classes were used. This has been fixed. Thanks to [@kichik](https://github.com/kichik) for bringing this to our attention. [PR#2864](https://github.com/newrelic/newrelic-ruby-agent/pull/2684)
+
+- **Bugfix: Remove Rails::Command::RakeCommand from the default list of autostart.denylisted_constants**
+
+  The default value for the `autostart.denylisted_constants` configuration was changed in 9.10.0 to include `Rails::Command::RunnerCommand` and `Rails::Command::RakeCommand`. The inclusion of `Rails::Command::RakeCommand` prevented the agent from starting automatically when Solid Queue was started using `bin/rails solid_queue:start`. We recognize there are many commands nested within `Rails::Command::RakeCommand` and have decided to remove it from the default list. We encourage users who do not want the agent to run on `Rails::Command::RakeCommand` to add the constant to their configuration. This can be accomplished by adding the following to your `newrelic.yml` file:
+
+  ```yaml
+    autostart.denylisted_constants: "Rails::Command::ConsoleCommand,Rails::Command::CredentialsCommand,Rails::Command::Db::System::ChangeCommand,Rails::Command::DbConsoleCommand,Rails::Command::DestroyCommand,Rails::Command::DevCommand,Rails::Command::EncryptedCommand,Rails::Command::GenerateCommand,Rails::Command::InitializersCommand,Rails::Command::NotesCommand,Rails::Command::RakeCommand,Rails::Command::RoutesCommand,Rails::Command::RunnerCommand,Rails::Command::SecretsCommand,Rails::Console,Rails::DBConsole"
+  ```
+
+  Thank you, [@edariedl](https://github.com/edariedl), for reporting this issue. [Issue#2677](https://github.com/newrelic/newrelic-ruby-agent/issues/2677) [PR#2694](https://github.com/newrelic/newrelic-ruby-agent/pull/2694)
+
 ## v9.10.1
 
 - **Bugfix: Incompatibility with Bootstrap**
@@ -11,7 +29,7 @@ Version 9.10.1 fixes an incompatibility between the agent and the [Bootstrap](ht
 Version 9.10.0 introduces instrumentation for DynamoDB, adds a new feature to automatically apply nonces from the Rails content security policy, fixes a bug that would cause an expected error to negatively impact a transaction's Apdex, and fixes the agent's autostart logic so that by default `rails runner` and `rails db` commands will not cause the agent to start.
 
 - **Feature: Add instrumentation for DynamoDB**
-  
+
     The agent has added instrumentation for the aws-sdk-dynamodb gem. The agent will now record datastore spans for DynamoDB client calls made with the aws-sdk-dynamodb gem.  [PR#2642](https://github.com/newrelic/newrelic-ruby-agent/pull/2642)
 
 - **Feature: Automatically apply nonces from the Rails content security policy**

lib/new_relic/agent/aws.rb

@@ -8,15 +8,21 @@ module Aws
       CHARACTERS = %w[A B C D E F G H I J K L M N O P Q R S T U V W X Y Z 2 3 4 5 6 7].freeze
       HEX_MASK = '7fffffffff80'
 
-      def self.create_arn(service, resource, config)
-        region = config.region
-        account_id = NewRelic::Agent::Aws.convert_access_key_to_account_id(config.credentials.access_key_id)
-
+      def self.create_arn(service, resource, region, account_id)
         "arn:aws:#{service}:#{region}:#{account_id}:#{resource}"
       rescue => e
         NewRelic::Agent.logger.warn("Failed to create ARN: #{e}")
       end
 
+      def self.get_account_id(config)
+        access_key_id = config.credentials.credentials.access_key_id if config&.credentials&.credentials&.respond_to?(:access_key_id)
+        return unless access_key_id
+
+        NewRelic::Agent::Aws.convert_access_key_to_account_id(access_key_id)
+      rescue => e
+        NewRelic::Agent.logger.debug("Failed to create account id: #{e}")
+      end
+
       def self.convert_access_key_to_account_id(access_key)
         decoded_key = Integer(decode_to_hex(access_key[4..-1]), 16)
         mask = Integer(HEX_MASK, 16)

lib/new_relic/agent/configuration/default_source.rb

@@ -480,7 +480,12 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :type => Boolean,
           :allowed_from_server => false,
           :description => <<~DESC
-            Forces the exit handler that sends all cached data to collector before shutting down to be installed regardless of detecting scenarios where it generally should not be. Known use-case for this option is where Sinatra is running as an embedded service within another framework and the agent is detecting the Sinatra app and skipping the `at_exit` handler as a result. Sinatra classically runs the entire application in an `at_exit` block and would otherwise misbehave if the Agent's `at_exit` handler was also installed in those circumstances. Note: `send_data_on_exit` should also be set to `true` in  tandem with this setting."
+            The exit handler that sends all cached data to the collector before shutting down is forcibly installed. \
+            This is true even when it detects scenarios where it generally should not be. The known use case for this \
+            option is when Sinatra runs as an embedded service within another framework. The agent detects the Sinatra \
+            app and skips the `at_exit` handler as a result. Sinatra classically runs the entire application in an \
+            `at_exit` block and would otherwise misbehave if the agent's `at_exit` handler was also installed in those \
+            circumstances. Note: `send_data_on_exit` should also be set to `true` in tandem with this setting.
           DESC
         },
         :high_security => {
@@ -1077,7 +1082,6 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
             Rails::Command::GenerateCommand
             Rails::Command::InitializersCommand
             Rails::Command::NotesCommand
-            Rails::Command::RakeCommand
             Rails::Command::RoutesCommand
             Rails::Command::RunnerCommand
             Rails::Command::SecretsCommand
@@ -1144,8 +1148,12 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :public => true,
           :type => Integer,
           :allowed_from_server => true,
-          :description => 'Specify a maximum number of custom events to buffer in memory at a time.',
-          :dynamic_name => true
+          :dynamic_name => true,
+          :description => <<~DESC
+            * Specify a maximum number of custom events to buffer in memory at a time.'
+            * When configuring the agent for [AI monitoring](/docs/ai-monitoring/intro-to-ai-monitoring), \
+            set to max value `100000`. This ensures the agent captures the maximum amount of LLM events.
+          DESC
         },
         # Datastore tracer
         :'datastore_tracer.database_name_reporting.enabled' => {
@@ -1983,8 +1991,8 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :allowed_from_server => true,
           :description => <<~DESC
             * Defines the maximum number of span events reported from a single harvest. Any Integer between `1` and `10000` is valid.'
-              * When configuring the agent for [AI monitoring](/docs/ai-monitoring/intro-to-ai-monitoring), set to max value `10000`.\
-              This ensures that the agent captures the maximum amount of distributed traces.
+            * When configuring the agent for [AI monitoring](/docs/ai-monitoring/intro-to-ai-monitoring), set to max value `10000`.\
+            This ensures the agent captures the maximum amount of distributed traces.
           DESC
         },
         # Strip exception messages

lib/new_relic/agent/instrumentation/dynamodb/instrumentation.rb

@@ -49,10 +49,16 @@ def build_request_with_new_relic(*args)
       @nr_captured_request = yield
     end
 
+    def nr_account_id
+      return @nr_account_id if defined?(@nr_account_id)
+
+      @nr_account_id = NewRelic::Agent::Aws.get_account_id(config)
+    end
+
     def get_arn(params)
-      return unless params[:table_name]
+      return unless params[:table_name] && nr_account_id
 
-      NewRelic::Agent::Aws.create_arn(PRODUCT.downcase, "table/#{params[:table_name]}", config)
+      NewRelic::Agent::Aws.create_arn(PRODUCT.downcase, "table/#{params[:table_name]}", config&.region, nr_account_id)
     end
   end
 end

lib/new_relic/version.rb

@@ -7,7 +7,7 @@ module NewRelic
   module VERSION # :nodoc:
     MAJOR = 9
     MINOR = 10
-    TINY = 1
+    TINY = 2
 
     STRING = "#{MAJOR}.#{MINOR}.#{TINY}"
   end

lib/tasks/instrumentation_generator/instrumentation.thor

@@ -31,15 +31,16 @@ class Instrumentation < Thor
 
   def scaffold(name)
     @name = name
+    @snake_name = snake_name(@name)
     @method = options[:method] if options[:method]
     @args = options[:args] if options[:args]
     @class_name = ::NewRelic::LanguageSupport.camelize(name)
-    base_path = "#{INSTRUMENTATION_ROOT}#{name.downcase}"
+    base_path = "#{INSTRUMENTATION_ROOT}#{@snake_name}"
 
     empty_directory(base_path)
     create_instrumentation_files(base_path)
-    append_to_default_source(name)
-    append_to_newrelic_yml(name)
+    append_to_default_source(@name, @snake_name)
+    # append_to_newrelic_yml(@name, @snake_name) # This is now done on release, we don't need it anymore, but leaving it to be sure.
     create_tests(name)
   end
 
@@ -69,53 +70,60 @@ class Instrumentation < Thor
   def create_tests(name)
     @name = name
     @instrumentation_method_global_erb_snippet = '<%= $instrumentation_method %>'
-    base_path = "#{MULTIVERSE_SUITE_ROOT}#{@name.downcase}"
+    @snake_name = snake_name(@name)
+    base_path = "#{MULTIVERSE_SUITE_ROOT}#{@snake_name}"
     empty_directory(base_path)
     template('templates/Envfile.tt', "#{base_path}/Envfile")
-    template('templates/test.tt', "#{base_path}/#{@name.downcase}_instrumentation_test.rb")
+    template('templates/test.tt', "#{base_path}/#{@snake_name}_instrumentation_test.rb")
 
     empty_directory("#{base_path}/config")
     template('templates/newrelic.yml.tt', "#{base_path}/config/newrelic.yml")
   end
 
-  def append_to_default_source(name)
+  def append_to_default_source(name, snake_name)
     insert_into_file(
       DEFAULT_SOURCE_LOCATION,
-      config_block(name.downcase),
+      config_block(name, snake_name),
       after: ":description => 'Controls auto-instrumentation of bunny at start-up. May be one of: `auto`, `prepend`, `chain`, `disabled`.'
         },\n"
     )
   end
 
-  def append_to_newrelic_yml(name)
+  def append_to_newrelic_yml(name, snake_name)
     insert_into_file(
       NEWRELIC_YML_LOCATION,
-      yaml_block(name),
+      yaml_block(name, snake_name),
       after: "# instrumentation.bunny: auto\n"
     )
   end
 
-  def config_block(name)
-    <<~CONFIG
-      :'instrumentation.#{name.downcase}' => {
-        :default => 'auto',
-        :public => true,
-        :type => String,
-        :dynamic_name => true,
-        :allowed_from_server => false,
-        :description => 'Controls auto-instrumentation of the #{name} library at start-up. May be one of [auto|prepend|chain|disabled].'
-      },
+  def config_block(name, snake_name)
+    # Don't change to <<~
+    # We want to preserve the whitespace so the config is correctly indented
+    <<-CONFIG
+        :'instrumentation.#{snake_name}' => {
+          :default => 'auto',
+          :public => true,
+          :type => String,
+          :dynamic_name => true,
+          :allowed_from_server => false,
+          :description => 'Controls auto-instrumentation of the #{name} library at start-up. May be one of `auto`, `prepend`, `chain`, `disabled`.'
+        },
     CONFIG
   end
 
-  def yaml_block(name)
+  def yaml_block(name, snake_name)
     <<~HEREDOC
 
       # Controls auto-instrumentation of #{name} at start-up.
       # May be one of [auto|prepend|chain|disabled]
-      # instrumentation.#{name.downcase}: auto
+      # instrumentation.#{snake_name}: auto
     HEREDOC
   end
+
+  def snake_name(name)
+    name.downcase.tr('-', '_')
+  end
 end
 
 Instrumentation.start(ARGV)

lib/tasks/instrumentation_generator/templates/dependency_detection.tt

@@ -2,12 +2,12 @@
 # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
 # frozen_string_literal: true
 
-require_relative '<%= @name.downcase %>/instrumentation'
-require_relative '<%= @name.downcase %>/chain'
-require_relative '<%= @name.downcase %>/prepend'
+require_relative '<%= @snake_name.downcase %>/instrumentation'
+require_relative '<%= @snake_name.downcase %>/chain'
+require_relative '<%= @snake_name.downcase %>/prepend'
 
 DependencyDetection.defer do
-  named :<%= @name.match?(/\-|\_/) ? "'#{@name.downcase}'" : @name.downcase %>
+  named :<%= @name.match?(/\-|\_/) ? "'#{@snake_name}'" : @name.downcase %>
 
   depends_on do
     # The class that needs to be defined to prepend/chain onto. This can be used

lib/tasks/instrumentation_generator/templates/newrelic.yml.tt

@@ -6,7 +6,7 @@ development:
   monitor_mode: true
   license_key: bootstrap_newrelic_admin_license_key_000
   instrumentation:
-    <%= @name.downcase %>: <%= @instrumentation_method_global_erb_snippet %>
+    <%= @snake_name %>: <%= @instrumentation_method_global_erb_snippet %>
   app_name: test
   log_level: debug
   host: 127.0.0.1

newrelic.yml

@@ -116,7 +116,7 @@ common: &default_settings
   # Specify a list of constants that should prevent the agent from starting
   # automatically. Separate individual constants with a comma ,. For example,
   # "Rails::Console,UninstrumentedBackgroundJob".
-  # autostart.denylisted_constants: Rails::Command::ConsoleCommand,Rails::Command::CredentialsCommand,Rails::Command::Db::System::ChangeCommand,Rails::Command::DbConsoleCommand,Rails::Command::DestroyCommand,Rails::Command::DevCommand,Rails::Command::EncryptedCommand,Rails::Command::GenerateCommand,Rails::Command::InitializersCommand,Rails::Command::NotesCommand,Rails::Command::RakeCommand,Rails::Command::RoutesCommand,Rails::Command::RunnerCommand,Rails::Command::SecretsCommand,Rails::Console,Rails::DBConsole
+  # autostart.denylisted_constants: Rails::Command::ConsoleCommand,Rails::Command::CredentialsCommand,Rails::Command::Db::System::ChangeCommand,Rails::Command::DbConsoleCommand,Rails::Command::DestroyCommand,Rails::Command::DevCommand,Rails::Command::EncryptedCommand,Rails::Command::GenerateCommand,Rails::Command::InitializersCommand,Rails::Command::NotesCommand,Rails::Command::RoutesCommand,Rails::Command::RunnerCommand,Rails::Command::SecretsCommand,Rails::Console,Rails::DBConsole
 
   # Defines a comma-delimited list of executables that the agent should not
   # instrument. For example, "rake,my_ruby_script.rb".
@@ -188,7 +188,9 @@ common: &default_settings
   # If true, the agent captures custom events.
   # custom_insights_events.enabled: true
 
-  # Specify a maximum number of custom events to buffer in memory at a time.
+  # * Specify a maximum number of custom events to buffer in memory at a time.'
+  # * When configuring the agent for AI monitoring, set to max value 100000. This
+  # ensures the agent captures the maximum amount of LLM events.
   # custom_insights_events.max_samples_stored: 3000
 
   # If false, the agent will not add database_name parameter to transaction or slow
@@ -354,14 +356,14 @@ common: &default_settings
   # requests.
   # exclude_newrelic_header: false
 
-  # Forces the exit handler that sends all cached data to collector before shutting
-  # down to be installed regardless of detecting scenarios where it generally should
-  # not be. Known use-case for this option is where Sinatra is running as an
-  # embedded service within another framework and the agent is detecting the Sinatra
-  # app and skipping the at_exit handler as a result. Sinatra classically runs the
+  # The exit handler that sends all cached data to the collector before shutting
+  # down is forcibly installed. This is true even when it detects scenarios where it
+  # generally should not be. The known use case for this option is when Sinatra runs
+  # as an embedded service within another framework. The agent detects the Sinatra
+  # app and skips the at_exit handler as a result. Sinatra classically runs the
   # entire application in an at_exit block and would otherwise misbehave if the
-  # Agent's at_exit handler was also installed in those circumstances. Note:
-  # send_data_on_exit should also be set to true in  tandem with this setting."
+  # agent's at_exit handler was also installed in those circumstances. Note:
+  # send_data_on_exit should also be set to true in tandem with this setting.
   # force_install_exit_handler: false
 
   # Ordinarily the agent reports dyno names with a trailing dot and process ID (for
@@ -699,8 +701,8 @@ common: &default_settings
 
   # * Defines the maximum number of span events reported from a single harvest. Any
   # Integer between 1 and 10000 is valid.'
-  # * When configuring the agent for AI monitoring, set to max value 10000.  This
-  # ensures that the agent captures the maximum amount of distributed traces.
+  # * When configuring the agent for AI monitoring, set to max value 10000.This
+  # ensures the agent captures the maximum amount of distributed traces.
   # span_events.max_samples_stored: 2000
 
   # Sets the maximum number of span events to buffer when streaming to the trace

test/multiverse/suites/dynamodb/dynamodb_instrumentation_test.rb

@@ -7,6 +7,8 @@
 class DynamodbInstrumentationTest < Minitest::Test
   def setup
     Aws.config.update(stub_responses: true)
+    NewRelic::Agent::Aws.stubs(:create_arn).returns('test-arn')
+    NewRelic::Agent::Aws.stubs(:get_account_id).returns('123456789')
     @stats_engine = NewRelic::Agent.instance.stats_engine
   end
 
@@ -22,7 +24,6 @@ def create_client
   def test_all_attributes_added_to_segment
     client = create_client
     Seahorse::Client::Http::Response.any_instance.stubs(:headers).returns({'x-amzn-requestid' => '1234321'})
-    NewRelic::Agent::Aws.stubs(:create_arn).returns('test-arn')
 
     in_transaction do |txn|
       client.query({

test/new_relic/agent/aws_test.rb

@@ -6,18 +6,26 @@
 
 class AwsTest < Minitest::Test
   def test_create_arn
+    service = 'test-service'
+    region = 'us-test-region-1'
+    account_id = '123456789'
+    resource = 'test/test-resource'
+    arn = NewRelic::Agent::Aws.create_arn(service, resource, region, account_id)
+
+    expected = 'arn:aws:test-service:us-test-region-1:123456789:test/test-resource'
+
+    assert_equal expected, arn
+  end
+
+  def test_get_account_id
     config = mock
-    config.stubs(:region).returns('us-test-region-1')
     mock_credentials = mock
+    mock_credentials.stubs(:credentials).returns(mock_credentials)
     mock_credentials.stubs(:access_key_id).returns('AKIAIOSFODNN7EXAMPLE') # this is a fake access key id from aws docs
     config.stubs(:credentials).returns(mock_credentials)
 
-    service = 'test-service'
-    resource = 'test/test-resource'
-    arn = NewRelic::Agent::Aws.create_arn(service, resource, config)
+    account_id = NewRelic::Agent::Aws.get_account_id(config)
 
-    expected = 'arn:aws:test-service:us-test-region-1:36315003739:test/test-resource'
-
-    assert_equal expected, arn
+    assert_equal 36315003739, account_id
   end
 end