Merge pull request #1673 from newrelic/holiday_spiced_sider

Redis instrumentation: support older redis-client

.rubocop.yml

@@ -1341,6 +1341,7 @@ Style/MethodCallWithArgsParentheses:
     - skip
     - source
     - stub
+    - stub_const
   AllowedPatterns: [^assert, ^refute]
 
 Style/MethodCallWithoutArgsParentheses:

CHANGELOG.md

@@ -2,7 +2,7 @@
 
   ## v8.14.0
 
-  Version 8.14.0 of the agent restores desired Capistrano-based changelog lookup functionalty when a deployment is performed and delivers support for instrumenting Rails custom event notifications.
+  Version 8.14.0 of the agent restores desired Capistrano-based changelog lookup functionalty when a deployment is performed, delivers support for instrumenting Rails custom event notifications, and fixes potential compatibility issues with Redis gems.
 
   * **Deployment Recipe: Restore desired Capistrano-based changelog lookup behavior**
 
@@ -12,14 +12,18 @@
 
     When the new `active_support_custom_events_names` configuration parameter is set equal to an array of custom event names to subscribe to, the agent will now subscribe to each of the names specified and report instrumentation for the events when they take place. [Creating custom events](https://guides.rubyonrails.org/active_support_instrumentation.html#creating-custom-events) is simple and now reporting instrumentation for them to New Relic is simple as well. [PR#1659](https://github.com/newrelic/newrelic-ruby-agent/pull/1659)
 
+  * **Bugfix: Support older versions of the RedisClient gem, handle unknown Redis database index
+
+    With version 8.13.0 of the agent, support was added for the [redis-rb](https://github.com/redis/redis-rb) gem v5+ and the new [RedisClient](https://rubygems.org/gems/redis-client) gem. With versions of RedisClient older than v0.11, the agent could cause the monitored application to crash when attempting to determine the Redis database index. Version 8.14.0 adds two related improvements. Firstly, support for RedisClient versions older than v0.11 has been added to get at the database index value. Secondly, the agent will no longer crash or impact the monitored application in the event that the database index cannot be obtained. Thank you very much to our community members [@mbsmartee](https://github.com/mbsmartee) and [@patatepartie](https://github.com/patatepartie) for bringing this issue to our attention, for helping us determine how to best reproduce it, and for testing out the update. We appreciate your help! [Issue#1650](https://github.com/newrelic/newrelic-ruby-agent/issues/1650) [PR#1673](https://github.com/newrelic/newrelic-ruby-agent/pull/1673)
+
 
   ## v8.13.1
 
   Version 8.13.1 of the agent provides a bugfix for Redis v5.0 instrumentation.
 
-  * **Fix NoMethodError when using Sidekiq v7.0 with Redis Client v0.11**
+  * **Fix NoMethodError when using Sidekiq v7.0 with RedisClient v0.11**
 
-    In some cases, the `RedisClient` object cannot directly access methods like db, port, or path. These methods are always available on the `client.config` object. This raised a `NoMethodError` in environments that used Sidekiq v7.0 and [Redis Client](https://rubygems.org/gems/redis-client) v0.11. Thank you to [fcheung](https://github.com/fcheung) and [@stevenou](https://github.com/stevenou) for bringing this to our attention! [Issue#1639](https://github.com/newrelic/newrelic-ruby-agent/issues/1639)
+    In some cases, the `RedisClient` object cannot directly access methods like db, port, or path. These methods are always available on the `client.config` object. This raised a `NoMethodError` in environments that used Sidekiq v7.0 and [RedisClient](https://rubygems.org/gems/redis-client) v0.11. Thank you to [fcheung](https://github.com/fcheung) and [@stevenou](https://github.com/stevenou) for bringing this to our attention! [Issue#1639](https://github.com/newrelic/newrelic-ruby-agent/issues/1639)
 
 
   ## v8.13.0

lib/new_relic/agent/instrumentation/redis/instrumentation.rb

@@ -27,10 +27,17 @@ def call_pipeline_with_tracing(pipeline)
 
     # Used for Redis 5.x+
     def call_pipelined_with_tracing(pipeline)
+      db = begin
+        _nr_redis_client_config.db
+      rescue StandardError => e
+        NewRelic::Agent.logger.error("Failed to determine configured Redis db value: #{e.class} - #{e.message}")
+        nil
+      end
+
       operation = pipeline.flatten.include?('MULTI') ? Constants::MULTI_OPERATION : Constants::PIPELINE_OPERATION
       statement = ::NewRelic::Agent::Datastores::Redis.format_pipeline_commands(pipeline)
 
-      with_tracing(operation, statement: statement, database: client.config.db) { yield }
+      with_tracing(operation, statement: statement, database: db) { yield }
     end
 
     private
@@ -52,21 +59,35 @@ def with_tracing(operation, statement: nil, database: nil)
     end
 
     def _nr_hostname
-      _nr_client.path ? Constants::LOCALHOST : _nr_client.host
+      _nr_redis_client_config.path ? Constants::LOCALHOST : _nr_redis_client_config.host
     rescue => e
-      NewRelic::Agent.logger.debug("Failed to retrieve Redis host: #{e}")
+      NewRelic::Agent.logger.debug("Failed to retrieve Redis host: #{e.class} - #{e.message}")
       Constants::UNKNOWN
     end
 
     def _nr_port_path_or_id
-      _nr_client.path || _nr_client.port
+      _nr_redis_client_config.path || _nr_redis_client_config.port
     rescue => e
-      NewRelic::Agent.logger.debug("Failed to retrieve Redis port_path_or_id: #{e}")
+      NewRelic::Agent.logger.debug("Failed to retrieve Redis port_path_or_id: #{e.class} - #{e.message}")
       Constants::UNKNOWN
     end
 
-    def _nr_client
-      @nr_client ||= self.is_a?(::Redis::Client) ? self : client.config
+    def _nr_redis_client_config
+      @nr_config ||= begin
+        # redis gem
+        config = if defined?(::Redis::Client) && self.is_a?(::Redis::Client)
+          self
+        # redis-client gem v0.11+ (self is a RedisClient::Middlewares)
+        elsif respond_to?(:client)
+          client && client.config
+        # redis-client gem <0.11 (self is a RedisClient::Middlewares)
+        elsif defined?(::RedisClient)
+          ::RedisClient.config if ::RedisClient.respond_to?(:config)
+        end
+        raise 'Unable to locate the underlying Redis client configuration.' unless config
+
+        config
+      end
     end
   end
 end

test/multiverse/suites/redis/redis_instrumentation_test.rb

@@ -4,12 +4,17 @@
 
 require 'redis'
 require_relative '../../../helpers/docker'
+require_relative '../../../helpers/misc'
 
 if NewRelic::Agent::Datastores::Redis.is_supported_version?
   class NewRelic::Agent::Instrumentation::RedisInstrumentationTest < Minitest::Test
     include MultiverseHelpers
     setup_and_teardown_agent
 
+    class FakeClient
+      include ::NewRelic::Agent::Instrumentation::Redis
+    end
+
     def after_setup
       super
       # Default timeout is 5 secs; a flushall takes longer on a busy box (i.e. CI)
@@ -388,6 +393,70 @@ def test_instrumentation_returns_expected_values
       assert_equal 2, @redis.del('foo', 'baz')
     end
 
+    def test__nr_redis_client_config_with_redis
+      skip_unless_minitest5_or_above
+
+      client = FakeClient.new
+
+      client.stub :is_a?, true, [::Redis::Client] do
+        assert_equal client, client.send(:_nr_redis_client_config)
+      end
+    end
+
+    def test__nr_redis_client_config_with_redis_client_v0_11
+      skip_unless_minitest5_or_above
+
+      client = FakeClient.new
+      config = 'the config'
+      mock_client = MiniTest::Mock.new
+      mock_client.expect :config, config
+      client.stub :respond_to?, true, [:client] do
+        client.stub :client, mock_client do
+          assert_equal config, client.send(:_nr_redis_client_config)
+        end
+      end
+    end
+
+    def test__nr_redis_client_config_with_redis_client_below_v0_11
+      skip_unless_minitest5_or_above
+
+      client = FakeClient.new
+      config = 'the config'
+      mock_client = MiniTest::Mock.new
+      mock_client.expect :config, config
+
+      Object.stub_const :RedisClient, mock_client do
+        assert_equal config, client.send(:_nr_redis_client_config)
+      end
+    end
+
+    def test__nr_redis_client_config_with_some_unknown_context
+      skip_unless_minitest5_or_above
+
+      client = FakeClient.new
+
+      Object.stub_const :RedisClient, nil do
+        assert_raises StandardError do
+          client.send(:_nr_redis_client_config)
+        end
+      end
+    end
+
+    def test_call_pipelined_with_tracing_uses_a_nil_db_value_if_it_must
+      client = FakeClient.new
+      with_tracing_validator = proc do |*args|
+        assert_equal 2, args.size
+        assert args.last.key?(:database)
+        refute args.last[:database]
+      end
+
+      Object.stub_const :RedisClient, nil do
+        client.stub :with_tracing, with_tracing_validator do
+          client.call_pipelined_with_tracing([]) { yield_value }
+        end
+      end
+    end
+
     def client
       if Gem::Version.new(Redis::VERSION).segments[0] < 4
         :client