From 5801bec234a9301822bdfec4b5c5b832ad2319d3 Mon Sep 17 00:00:00 2001 From: fallwith Date: Fri, 2 Dec 2022 15:26:46 -0800 Subject: [PATCH 1/5] Redis instrumentation: support older redis-client Redis instrumentation: * update `call_pipelined_with_tracing` to yield early (instead of crashing) if a db value cannot be obtained * rename `_nr_client` to `_nr_redis_client_config` to make it more clear that it holds a config object and not a client object, even though config getters can be called directly on the `redis` gem's client object * update `_nr_redis_client_config` to add support for `redis-client` gem versions older than v0.11 * update `_nr_redis_client_config` to raise upon failing to get at the underlying config object * minor exception handling updates to include class names * update the unit tests accordingly --- .rubocop.yml | 1 + .../instrumentation/redis/instrumentation.rb | 35 ++++++++-- .../redis/redis_instrumentation_test.rb | 64 +++++++++++++++++++ 3 files changed, 93 insertions(+), 7 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 5c02faf948..201f277524 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1341,6 +1341,7 @@ Style/MethodCallWithArgsParentheses: - skip - source - stub + - stub_const AllowedPatterns: [^assert, ^refute] Style/MethodCallWithoutArgsParentheses: diff --git a/lib/new_relic/agent/instrumentation/redis/instrumentation.rb b/lib/new_relic/agent/instrumentation/redis/instrumentation.rb index a6d15572f1..c741f3c8ef 100644 --- a/lib/new_relic/agent/instrumentation/redis/instrumentation.rb +++ b/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}") + return yield + 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 diff --git a/test/multiverse/suites/redis/redis_instrumentation_test.rb b/test/multiverse/suites/redis/redis_instrumentation_test.rb index daf7d2faa9..dc783667cc 100644 --- a/test/multiverse/suites/redis/redis_instrumentation_test.rb +++ b/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,65 @@ 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 + 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_yields_early_if_a_db_value_cannot_be_obtained + client = FakeClient.new + yield_value = 'Vegemite' + + Object.stub_const :RedisClient, nil do + # if with_tracing were to be reached, an exception will raise + client.stub :with_tracing, -> { raise 'kaboom' } do + assert_equal yield_value, client.call_pipelined_with_tracing(nil) { yield_value } + end + end + end + def client if Gem::Version.new(Redis::VERSION).segments[0] < 4 :client From 44b6617adcdc4b52838bacd310440faa44a70438 Mon Sep 17 00:00:00 2001 From: fallwith Date: Fri, 2 Dec 2022 19:25:01 -0800 Subject: [PATCH 2/5] CI: skip additional redis test on Ruby 2.2 Ruby 2.2 raises with this test as expected, but not in the way that's expected. The others in this group are skipping 2.2 already, just have this one skip it as well --- test/multiverse/suites/redis/redis_instrumentation_test.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/multiverse/suites/redis/redis_instrumentation_test.rb b/test/multiverse/suites/redis/redis_instrumentation_test.rb index dc783667cc..4d55962f59 100644 --- a/test/multiverse/suites/redis/redis_instrumentation_test.rb +++ b/test/multiverse/suites/redis/redis_instrumentation_test.rb @@ -431,6 +431,8 @@ def test__nr_redis_client_config_with_redis_client_below_v0_11 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 From 19b87c2bb8d467122e9861c47f15d8ed9431ce1d Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 6 Dec 2022 19:24:48 -0800 Subject: [PATCH 3/5] Redis instrumentation: still use db when nil The underlying `NewRelic::Agent::Transaction::DatastoreSegment` class permits `nil` for the database name argument, so it is perfectly fine to still create an instance of that class to still record all of the information we _do_ have, despite the db name value being `nil`. Thank you, @kaylareopelle --- .../agent/instrumentation/redis/instrumentation.rb | 2 +- .../suites/redis/redis_instrumentation_test.rb | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/redis/instrumentation.rb b/lib/new_relic/agent/instrumentation/redis/instrumentation.rb index c741f3c8ef..6dd7c3dbd6 100644 --- a/lib/new_relic/agent/instrumentation/redis/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/redis/instrumentation.rb @@ -31,7 +31,7 @@ def call_pipelined_with_tracing(pipeline) _nr_redis_client_config.db rescue StandardError => e NewRelic::Agent.logger.error("Failed to determine configured Redis db value: #{e.class} - #{e.message}") - return yield + nil end operation = pipeline.flatten.include?('MULTI') ? Constants::MULTI_OPERATION : Constants::PIPELINE_OPERATION diff --git a/test/multiverse/suites/redis/redis_instrumentation_test.rb b/test/multiverse/suites/redis/redis_instrumentation_test.rb index 4d55962f59..1606426a4c 100644 --- a/test/multiverse/suites/redis/redis_instrumentation_test.rb +++ b/test/multiverse/suites/redis/redis_instrumentation_test.rb @@ -442,14 +442,17 @@ def test__nr_redis_client_config_with_some_unknown_context end end - def test_call_pipelined_with_tracing_yields_early_if_a_db_value_cannot_be_obtained + def test_call_pipelined_with_tracing_uses_a_nil_db_value_if_it_must client = FakeClient.new - yield_value = 'Vegemite' + 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 - # if with_tracing were to be reached, an exception will raise - client.stub :with_tracing, -> { raise 'kaboom' } do - assert_equal yield_value, client.call_pipelined_with_tracing(nil) { yield_value } + client.stub :with_tracing, with_tracing_validator do + client.call_pipelined_with_tracing([]) { yield_value } end end end From f49331e1b96b4868fea55c5339166ed0181c2340 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 6 Dec 2022 19:39:54 -0800 Subject: [PATCH 4/5] v8.14.0 CHANGELOG entry for redis-client bugfix update CHANGELOG to reference issue 1650 and PR 1673 --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d70ab05346..1edc726368 100644 --- a/CHANGELOG.md +++ b/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,6 +12,10 @@ 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 redis-client gem, handle unknown Redis database index + + With version 8.13.0 of the agent, support was added for `redis` gem v5+ and the new `redis-client` gem. With versions of `redis-client` 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 `redis-client` 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. [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 From 772de0ad48a8e280953684ff81dba2ff1c344ea0 Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 7 Dec 2022 10:16:53 -0800 Subject: [PATCH 5/5] CHANGELOG: 8.14.0 updates for PR 1673 address PR feedback --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1edc726368..23e6b4abf3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,18 +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 redis-client gem, handle unknown Redis database index + * **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 `redis` gem v5+ and the new `redis-client` gem. With versions of `redis-client` 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 `redis-client` 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. [Issue#1650](https://github.com/newrelic/newrelic-ruby-agent/issues/1650) [PR#1673](https://github.com/newrelic/newrelic-ruby-agent/pull/1673) + 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