Merge pull request #2720 from newrelic/redis-clustering

Add support for redis-clustering gem

CHANGELOG.md

@@ -1,5 +1,13 @@
 # New Relic Ruby Agent Release Notes
 
+## <dev>
+
+Version <dev> improves instrumentation for the `redis-clustering` gem.
+
+- **Feature: Add instrumentation for redis-clustering**
+
+  Version 5.x of the `redis` gem moved cluster behavior into a different gem, `redis-clustering`. This gem can access instrumentation registered through `RedisClient::Middleware`. Previously, the agent only instrumented the `call_pipelined` method through this approach, but now users of the `redis-clustering` gem will also have instrumentation registered for `connect` and `call` methods. In addition, the way the `database_name` attribute is set for Redis datastore spans is now compatible with all versions of Redis supported by the New Relic Ruby agent. Thank you, [@praveen-ks](https://github.com/praveen-ks) for bringing this to our attention. [Issue#2444](https://github.com/newrelic/newrelic-ruby-agent/issues/2444) [PR#2720](https://github.com/newrelic/newrelic-ruby-agent/pull/2720)
+
 ## v9.11.0
 
 Version 9.11.0 introduces instrumentation for the aws-sdk-sqs gem, fixes a bug related to expected errors not bearing a "true" value for the "expected" attribute if expected as a result of an HTTP status code match and changes the way Stripe instrumentation metrics are named to prevent high-cardinality issues.

lib/new_relic/agent/instrumentation/redis.rb

@@ -10,6 +10,7 @@
 require_relative 'redis/constants'
 require_relative 'redis/prepend'
 require_relative 'redis/middleware'
+require_relative 'redis/cluster_middleware'
 
 DependencyDetection.defer do
   # Why not :redis? newrelic-redis used that name, so avoid conflicting
@@ -33,6 +34,10 @@
     NewRelic::Agent.logger.info('Installing Redis Instrumentation')
     if NewRelic::Agent::Instrumentation::Redis::Constants::HAS_REDIS_CLIENT
       RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::Middleware)
+
+      if defined?(Redis::Cluster::Client)
+        return RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::ClusterMiddleware)
+      end
     end
 
     if use_prepend?

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

@@ -0,0 +1,26 @@
+# This file is distributed under New Relic's license terms.
+# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details.
+# frozen_string_literal: true
+
+module NewRelic::Agent::Instrumentation
+  module RedisClient
+    module ClusterMiddleware
+      include NewRelic::Agent::Instrumentation::Redis
+
+      # Until we decide to move our Redis instrumentation entirely off patches
+      # keep the middleware instrumentation for the call and connect methods
+      # limited to the redis-clustering instrumentation.
+      #
+      # Redis's middleware option does not capture errors as high in the stack
+      # as our patches. Leaving the patches for call and connect on the main
+      # Redis gem limits the feature disparity our customers experience.
+      def call(*args, &block)
+        call_with_tracing(args[0]) { super }
+      end
+
+      def connect(*args, &block)
+        connect_with_tracing { super }
+      end
+    end
+  end
+end

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

@@ -9,37 +9,30 @@ module Redis
     INSTRUMENTATION_NAME = NewRelic::Agent.base_name(name)
 
     def connect_with_tracing
-      with_tracing(Constants::CONNECT, database: db) { yield }
+      with_tracing(Constants::CONNECT, database: _nr_db) { yield }
     end
 
     def call_with_tracing(command, &block)
       operation = command[0]
       statement = ::NewRelic::Agent::Datastores::Redis.format_command(command)
 
-      with_tracing(operation, statement: statement, database: db) { yield }
+      with_tracing(operation, statement: statement, database: _nr_db) { yield }
     end
 
     # Used for Redis 4.x and 3.x
     def call_pipeline_with_tracing(pipeline)
       operation = pipeline.is_a?(::Redis::Pipeline::Multi) ? Constants::MULTI_OPERATION : Constants::PIPELINE_OPERATION
       statement = ::NewRelic::Agent::Datastores::Redis.format_pipeline_commands(pipeline.commands)
 
-      with_tracing(operation, statement: statement, database: db) { yield }
+      with_tracing(operation, statement: statement, database: _nr_db) { yield }
     end
 
     # 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: db) { yield }
+      with_tracing(operation, statement: statement, database: _nr_db) { yield }
     end
 
     private
@@ -94,5 +87,15 @@ def _nr_redis_client_config
         config
       end
     end
+
+    def _nr_db
+      # db is a method on the Redis client in versions < 5.x
+      return db if respond_to?(:db)
+      # db is accessible through the RedisClient::Config object in versions > 5.x
+      return _nr_redis_client_config.db if _nr_redis_client_config.respond_to?(:db)
+    rescue StandardError => e
+      NewRelic::Agent.logger.debug("Failed to determine configured Redis db value: #{e.class} - #{e.message}")
+      nil
+    end
   end
 end

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

@@ -6,6 +6,9 @@ module NewRelic::Agent::Instrumentation
   module RedisClient
     module Middleware
       # This module is used to instrument Redis 5.x+
+      #
+      # It only instruments call_pipelined because connect and call are accessed
+      # too late in the stack to capture all errors
       include NewRelic::Agent::Instrumentation::Redis
 
       def call_pipelined(*args, &block)

test/multiverse/suites/redis/Envfile

@@ -14,8 +14,19 @@ def gem_list(redis_version = nil)
   <<~RB
     gem 'rack'
     gem 'redis'#{redis_version}
-    
+
   RB
 end
 
 create_gemfiles(REDIS_VERSIONS)
+
+# We do not spin up a full Redis cluster in the testing environment, which
+# limits our ability to run unit tests on the redis-clustering behavior.
+# Since the testing capability is limited, only test the latest version of the
+# redis-clustering gem, which itself requires Ruby v2.7+.
+if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')
+  gemfile <<~RB
+    gem 'rack'
+    gem 'redis-clustering'
+  RB
+end

test/multiverse/suites/redis/redis_instrumentation_test.rb

@@ -59,6 +59,8 @@ def test_records_metrics_for_connect
     end
 
     def test_records_connect_tt_node_within_call_that_triggered_it
+      skip_for_redis_clustering_gem
+
       in_transaction do
         redis = Redis.new
         redis.get('foo')
@@ -277,6 +279,8 @@ def test_records_instance_parameters_on_tt_node_for_get
     end
 
     def test_records_hostname_on_tt_node_for_get_with_unix_domain_socket
+      skip_for_redis_clustering_gem
+
       redis = Redis.new
       redis.send(client).stubs(:path).returns('/tmp/redis.sock')
 
@@ -309,6 +313,8 @@ def test_records_instance_parameters_on_tt_node_for_multi
     end
 
     def test_records_hostname_on_tt_node_for_multi_with_unix_domain_socket
+      skip_for_redis_clustering_gem
+
       redis = Redis.new
       redis.send(client).stubs(:path).returns('/tmp/redis.sock')
 
@@ -327,6 +333,8 @@ def test_records_hostname_on_tt_node_for_multi_with_unix_domain_socket
     end
 
     def test_records_unknown_unknown_metric_when_error_gathering_instance_data
+      skip_for_redis_clustering_gem
+
       redis = Redis.new
       redis.send(client).stubs(:path).raises(StandardError.new)
       in_transaction do
@@ -347,6 +355,8 @@ def simulate_read_error
     end
 
     def test_noticed_error_at_segment_and_txn_on_error
+      skip_for_redis_clustering_gem
+
       txn = nil
       begin
         in_transaction do |redis_txn|
@@ -362,6 +372,8 @@ def test_noticed_error_at_segment_and_txn_on_error
     end
 
     def test_noticed_error_only_at_segment_on_error
+      skip_for_redis_clustering_gem
+
       txn = nil
       in_transaction do |redis_txn|
         begin
@@ -472,4 +484,8 @@ def either_hostname
       [NewRelic::Agent::Hostname.get, 'redis']
     end
   end
+
+  def skip_for_redis_clustering_gem
+    skip 'Incompatible with redis-clustering' if defined?(Redis::Cluster::Client)
+  end
 end