Improve redis integration patching

lib/datadog/tracing/contrib/patcher.rb

@@ -66,14 +66,15 @@ def on_patch_error(e)
             Datadog.health_metrics.error_instrumentation_patch(1, tags: tags)
           end
 
-          private
-
           def default_tags
             ["patcher:#{patch_name}"].tap do |tags|
               tags << "target_version:#{target_version}" if respond_to?(:target_version) && !target_version.nil?
+              super.each { |t| tags << t } if defined?(super)
             end
           end
 
+          private
+
           def patch_only_once
             # NOTE: This is not thread-safe
             @patch_only_once ||= Core::Utils::OnlyOnce.new

lib/datadog/tracing/contrib/redis/integration.rb

@@ -14,19 +14,52 @@ class Integration
 
           MINIMUM_VERSION = Gem::Version.new('3.2')
 
+          # Support `Config#custom`
+          # https://github.com/redis-rb/redis-client/blob/master/CHANGELOG.md#0110
+          REDISCLIENT_MINIMUM_VERSION = Gem::Version.new('0.11.0')
+
           # @public_api Changing the integration name or integration options can cause breaking changes
           register_as :redis, auto_patch: true
 
+          # Until Redis 4, all instrumentation happened in one gem: redis.
+          # Since Redis 5, instrumentation happens in a separate gem: redis-client.
+          # Because Redis 4 does not depend on redis-client, it's possible for both gems to be installed at the same time.
+          # For example, if Sidekiq 7 and Redis 4 are installed: both redis and redis-client will be installed.
+          # If redis-client and redis > 5 are installed, than they will be in sync, and only redis-client will be installed.
           def self.version
+            redis_version || redis_client_version
+          end
+
+          def self.redis_version
             Gem.loaded_specs['redis'] && Gem.loaded_specs['redis'].version
           end
 
+          def self.redis_client_version
+            Gem.loaded_specs['redis-client'] && Gem.loaded_specs['redis-client'].version
+          end
+
           def self.loaded?
+            redis_loaded? || redis_client_loaded?
+          end
+
+          def self.redis_loaded?
             !defined?(::Redis).nil?
           end
 
+          def self.redis_client_loaded?
+            !defined?(::RedisClient).nil?
+          end
+
           def self.compatible?
-            super && version >= MINIMUM_VERSION
+            redis_compatible? || redis_client_compatible?
+          end
+
+          def self.redis_compatible?
+            !!(redis_version && redis_version >= MINIMUM_VERSION)
+          end
+
+          def self.redis_client_compatible?
+            !!(redis_client_version && redis_client_version >= REDISCLIENT_MINIMUM_VERSION)
           end
 
           def new_configuration

lib/datadog/tracing/contrib/redis/patcher.rb

@@ -49,17 +49,22 @@ def initialize(options = {})
 
           module_function
 
-          def target_version
-            Integration.version
+          def default_tags
+            [].tap do |tags|
+              tags << "target_redis_version:#{Integration.redis_version}"               if Integration.redis_version
+              tags << "target_redis_client_version:#{Integration.redis_client_version}" if Integration.redis_client_version
+            end
           end
 
           def patch
             # Redis 5+ extracts RedisClient to its own gem and provide instrumentation interface
-            if target_version >= Gem::Version.new('5.0.0')
+            if Integration.redis_client_compatible?
               require_relative 'trace_middleware'
 
               ::RedisClient.register(TraceMiddleware)
-            else
+            end
+
+            if Integration.redis_compatible? && Integration.redis_version < Gem::Version.new('5.0.0')
               require_relative 'instrumentation'
 
               ::Redis.include(InstancePatch)

spec/datadog/tracing/contrib/redis/integration_spec.rb

@@ -12,28 +12,72 @@
   describe '.version' do
     subject(:version) { described_class.version }
 
-    context 'when the "redis" gem is loaded' do
-      include_context 'loaded gems', redis: described_class::MINIMUM_VERSION
+    context 'when `redis` gem and `redis-client` are loaded' do
+      include_context 'loaded gems',
+        redis: described_class::MINIMUM_VERSION,
+        'redis-client' => described_class::REDISCLIENT_MINIMUM_VERSION
+
       it { is_expected.to be_a_kind_of(Gem::Version) }
     end
 
-    context 'when "redis" gem is not loaded' do
-      include_context 'loaded gems', redis: nil
+    context 'when `redis` gem is loaded' do
+      include_context 'loaded gems',
+        redis: described_class::MINIMUM_VERSION,
+        'redis-client' => nil
+
+      it { is_expected.to be_a_kind_of(Gem::Version) }
+    end
+
+    context 'when `redis-client` gem is loaded' do
+      include_context 'loaded gems',
+        redis: nil,
+        'redis-client' => described_class::REDISCLIENT_MINIMUM_VERSION
+
+      it { is_expected.to be_a_kind_of(Gem::Version) }
+    end
+
+    context 'when `redis` gem and `redis-client` are not loaded' do
+      include_context 'loaded gems', redis: nil, 'redis-client' => nil
+
       it { is_expected.to be nil }
     end
   end
 
   describe '.loaded?' do
     subject(:loaded?) { described_class.loaded? }
 
-    context 'when Redis is defined' do
-      before { stub_const('Redis', Class.new) }
+    context 'when `Redis` and `RedisClient` are defined' do
+      before do
+        stub_const('Redis', Class.new)
+        stub_const('RedisClient', Class.new)
+      end
 
       it { is_expected.to be true }
     end
 
-    context 'when Redis is not defined' do
-      before { hide_const('Redis') }
+    context 'when `Redis` is defined' do
+      before do
+        stub_const('Redis', Class.new)
+        hide_const('RedisClient')
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when `RedisClient` is defined' do
+      before do
+        hide_const('Redis')
+        stub_const('RedisClient', Class.new)
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when `Redis` and `RedisClient` are not defined' do
+      before do
+        hide_const('Redis')
+        hide_const('RedisClient')
+      end
 
       it { is_expected.to be false }
     end
@@ -42,20 +86,75 @@
   describe '.compatible?' do
     subject(:compatible?) { described_class.compatible? }
 
+    context 'when `redis` is compatible' do
+      before do
+        allow(described_class).to receive(:redis_compatible?).and_return(true)
+        allow(described_class).to receive(:redis_client_compatible?).and_return(false)
+      end
+      it { is_expected.to be true }
+    end
+
+    context 'when `redis-client` is compatible' do
+      before do
+        allow(described_class).to receive(:redis_compatible?).and_return(false)
+        allow(described_class).to receive(:redis_client_compatible?).and_return(true)
+      end
+      it { is_expected.to be true }
+    end
+
+    context 'when `redis` and `redis-client` are both incompatible' do
+      before do
+        allow(described_class).to receive(:redis_compatible?).and_return(false)
+        allow(described_class).to receive(:redis_client_compatible?).and_return(false)
+      end
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '.redis_compatible?' do
+    subject(:compatible?) { described_class.redis_compatible? }
+
     context 'when "redis" gem is loaded with a version' do
       context 'that is less than the minimum' do
         include_context 'loaded gems', redis: decrement_gem_version(described_class::MINIMUM_VERSION)
+
         it { is_expected.to be false }
       end
 
       context 'that meets the minimum version' do
         include_context 'loaded gems', redis: described_class::MINIMUM_VERSION
+
         it { is_expected.to be true }
       end
     end
 
     context 'when gem is not loaded' do
       include_context 'loaded gems', redis: nil
+
+      it { is_expected.to be false }
+    end
+  end
+
+  describe '.redis_client_compatible?' do
+    subject(:compatible?) { described_class.redis_client_compatible? }
+
+    context 'when "redis-client" gem is loaded with a version' do
+      context 'that is less than the minimum' do
+        include_context 'loaded gems', 'redis-client' => decrement_gem_version(described_class::REDISCLIENT_MINIMUM_VERSION)
+
+        it { is_expected.to be false }
+      end
+
+      context 'that meets the minimum version' do
+        include_context 'loaded gems', 'redis-client' => described_class::REDISCLIENT_MINIMUM_VERSION
+
+        it { is_expected.to be true }
+      end
+    end
+
+    context 'when gem is not loaded' do
+      include_context 'loaded gems', 'redis-client' => nil
+
       it { is_expected.to be false }
     end
   end

spec/datadog/tracing/contrib/redis/patcher_spec.rb

@@ -0,0 +1,23 @@
+# typed: ignore
+
+require 'datadog/tracing/contrib/support/spec_helper'
+
+require 'datadog/tracing/contrib/redis/patcher'
+
+RSpec.describe Datadog::Tracing::Contrib::Redis::Patcher do
+  describe '.default_tags' do
+    it do
+      result = described_class.default_tags
+
+      expect(result).to include(start_with('patcher:'))
+
+      if Datadog::Tracing::Contrib::Redis::Integration.redis_version
+        expect(result).to include(start_with('target_redis_version:'))
+      end
+
+      if Datadog::Tracing::Contrib::Redis::Integration.redis_client_version
+        expect(result).to include(start_with('target_redis_client_version:'))
+      end
+    end
+  end
+end