From 6719a92417bf24851098f0fea5b03944a0241635 Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 15 Aug 2024 17:21:04 -0700 Subject: [PATCH 1/2] Bugfix: don't use `return` in Redis detection - Bugfix for https://github.com/newrelic/newrelic-ruby-agent/issues/2814 - Added relevant regression test (with logic taking place in `Gemfile` before the instrumentation is loaded) resolves #2814 --- CHANGELOG.md | 6 +++++- lib/new_relic/agent/instrumentation/redis.rb | 12 +++++++----- test/multiverse/suites/redis/Envfile | 10 ++++++++++ .../suites/redis/redis_instrumentation_test.rb | 14 ++++++++++++++ 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7268e81719..3278acf530 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## dev -Version adds experimental OpenSearch instrumentation, updates framework detection, fixes Falcon dispatcher detection, and addresses a JRuby specific concurrency issue. +Version 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 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) diff --git a/lib/new_relic/agent/instrumentation/redis.rb b/lib/new_relic/agent/instrumentation/redis.rb index 691c8e87c3..fa435855c3 100644 --- a/lib/new_relic/agent/instrumentation/redis.rb +++ b/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 diff --git a/test/multiverse/suites/redis/Envfile b/test/multiverse/suites/redis/Envfile index 04d39db434..46fec9230f 100644 --- a/test/multiverse/suites/redis/Envfile +++ b/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' + ::Redis::Cluster.const_set(:Client, 'phony client definition') + GEMFILE end diff --git a/test/multiverse/suites/redis/redis_instrumentation_test.rb b/test/multiverse/suites/redis/redis_instrumentation_test.rb index 0d5a6a7420..1a5128db4f 100644 --- a/test/multiverse/suites/redis/redis_instrumentation_test.rb +++ b/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 From 4326957664e8037e836bcf218bc609b5de446d39 Mon Sep 17 00:00:00 2001 From: fallwith Date: Thu, 15 Aug 2024 18:15:02 -0700 Subject: [PATCH 2/2] CI: permit non 'gem' lines in Envfile use a new comment system to permit non 'gem' lines in `Envfile` files --- test/multiverse/lib/multiverse/gem_manifest.rb | 2 +- test/multiverse/suites/redis/Envfile | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/multiverse/lib/multiverse/gem_manifest.rb b/test/multiverse/lib/multiverse/gem_manifest.rb index d8e8b2d708..c217ece836 100755 --- a/test/multiverse/lib/multiverse/gem_manifest.rb +++ b/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) diff --git a/test/multiverse/suites/redis/Envfile b/test/multiverse/suites/redis/Envfile index 46fec9230f..a8121081f5 100644 --- a/test/multiverse/suites/redis/Envfile +++ b/test/multiverse/suites/redis/Envfile @@ -36,7 +36,7 @@ if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0') gem 'rack' gem 'redis-clustering' - require 'redis' - ::Redis::Cluster.const_set(:Client, 'phony client definition') + require 'redis' # non-gem line + ::Redis::Cluster.const_set(:Client, 'phony client definition') # non-gem line GEMFILE end