Merge pull request #2816 from newrelic/double_devon

Bugfix: don't use `return` in Redis detection

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## dev
 
-Version <dev> adds experimental OpenSearch instrumentation, updates framework detection, fixes Falcon dispatcher detection, and addresses a JRuby specific concurrency issue.
+Version <dev> adds experimental OpenSearch instrumentation, updates framework detection, fixes Falcon dispatcher detection, fixes a bug with Redis instrumentation installation, and addresses a JRuby specific concurrency issue.
 
 - **Feature: Add experimental OpenSearch instrumentation**
 
@@ -16,6 +16,10 @@ Version <dev> adds experimental OpenSearch instrumentation, updates framework de
 
   Previously, we tried to use the object space to determine whether the [Falcon web server](https://github.com/socketry/falcon) was in use. However, Falcon is not added to the object space until after the environment report is generated, resulting in a `nil` dispatcher. Now, we revert to an earlier strategy that discovered the dispatcher using `File.basename`. Thank you, [@prateeksen](https://github.com/prateeksen) for reporting this issue and researching the problem. [Issue#2778](https://github.com/newrelic/newrelic-ruby-agent/issues/2778) [PR#2795](https://github.com/newrelic/newrelic-ruby-agent/pull/2795)
 
+- **Bugfix: Fix for a Redis instrumentation error when Redis::Cluster::Client is present**
+
+  The Redis instrumentation previously contained a bug that would cause it to error out when `Redis::Cluster::Client` was present, owing to the use of a Ruby `return` outside of a method. Thanks very much to [@jdelStrother](https://github.com/jdelStrother) for not only reporting this bug but pointing us to the root cause as well. [Issue#2814](https://github.com/newrelic/newrelic-ruby-agent/issues/2814) [PR#2816](https://github.com/newrelic/newrelic-ruby-agent/pull/2816)
+
 - **Bugfix: Address JRuby concurrency issue with config hash accessing**
 
   The agent's internal configuration class maintains a hash that occassionally gets rebuilt. During the rebuild, certain previously dynamically determined instrumentation values are preserved for the benefit of the [New Relic Ruby security agent](https://github.com/newrelic/csec-ruby-agent). After reports from JRuby customers regarding concurrency issues related to the hash being accessed while being modified, two separate fixes went into the hash rebuild logic previously: a `Hash#dup` operation and a `synchronize do` block. But errors were still reported. We ourselves remain unable to reproduce these concurrency errors despite using the same exact versions of JRuby and all reported software. After confirming that the hash access code in question is only needed for the Ruby security agent (which operates only in non-production dedicated security testing environments), we have introduced a new fix for JRuby customers that will simply skip over the troublesome code when JRuby is in play but the security agent is not. [PR#2798](https://github.com/newrelic/newrelic-ruby-agent/pull/2798)

lib/new_relic/agent/instrumentation/redis.rb

@@ -36,14 +36,16 @@
       RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::Middleware)
 
       if defined?(Redis::Cluster::Client)
-        return RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::ClusterMiddleware)
+        RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::ClusterMiddleware)
       end
     end
 
-    if use_prepend?
-      prepend_instrument Redis::Client, NewRelic::Agent::Instrumentation::Redis::Prepend
-    else
-      chain_instrument NewRelic::Agent::Instrumentation::Redis::Chain
+    unless defined?(Redis::Cluster::Client)
+      if use_prepend?
+        prepend_instrument Redis::Client, NewRelic::Agent::Instrumentation::Redis::Prepend
+      else
+        chain_instrument NewRelic::Agent::Instrumentation::Redis::Chain
+      end
     end
   end
 end

test/multiverse/lib/multiverse/gem_manifest.rb

@@ -66,7 +66,7 @@ def discover_suites
 
     def gems_from_gemfile_body(body, path)
       body.split("\n").each do |line|
-        next if line.empty? || line.match?(/(?:^\s*(?:#|if|else|end))|newrelic_(?:rpm|prepender)/)
+        next if line.empty? || line.match?(/(?:^\s*(?:#|if|else|end))|newrelic_(?:rpm|prepender)|# non-gem line/)
 
         if line =~ /.*gem\s+['"]([^'"]+)['"](?:,\s+['"]([^'"]+)['"])?/
           gem = Regexp.last_match(1)

test/multiverse/suites/redis/Envfile

@@ -29,4 +29,14 @@ if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0')
     gem 'rack'
     gem 'redis-clustering'
   RB
+
+  # regression test for dependency detection bug
+  # https://github.com/newrelic/newrelic-ruby-agent/issues/2814
+  gemfile <<~GEMFILE
+    gem 'rack'
+    gem 'redis-clustering'
+
+    require 'redis'                                                 # non-gem line
+    ::Redis::Cluster.const_set(:Client, 'phony client definition')  # non-gem line
+  GEMFILE
 end

test/multiverse/suites/redis/redis_instrumentation_test.rb

@@ -468,6 +468,20 @@ def test_call_pipelined_with_tracing_uses_a_nil_db_value_if_it_must
       end
     end
 
+    # regression test for dependency detection bug
+    # https://github.com/newrelic/newrelic-ruby-agent/issues/2814
+    def test_having_a_redis_cluster_client_does_not_cause_an_error
+      skip_unless_minitest5_or_above
+      skip unless defined?(::Redis::Cluster::Client)
+
+      log_data = File.read(File.join(File.dirname(__FILE__), 'log', 'newrelic_agent.log'))
+      contains_error = log_data.match?('LocalJumpError')
+
+      refute contains_error, "Expected the agent log to be free of 'LocalJumpError' errors"
+    end
+
+    private
+
     def client
       if Gem::Version.new(Redis::VERSION).segments[0] < 4
         :client