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/CHANGELOG.md b/CHANGELOG.md index d70ab05346..23e6b4abf3 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,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 diff --git a/lib/new_relic/agent/instrumentation/redis/instrumentation.rb b/lib/new_relic/agent/instrumentation/redis/instrumentation.rb index a6d15572f1..6dd7c3dbd6 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}") + 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 diff --git a/test/multiverse/suites/redis/redis_instrumentation_test.rb b/test/multiverse/suites/redis/redis_instrumentation_test.rb index daf7d2faa9..1606426a4c 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,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