Merge branch 'osw-support-multiple-redis-clients-instrumentation' into 'master'

Separate Redis instrumentation per client/instance

See merge request gitlab-org/gitlab!33539

config/initializers/action_cable.rb

 # frozen_string_literal: true
 
+require 'action_cable/subscription_adapter/redis'
+
 Rails.application.configure do
   # We only mount the ActionCable engine in tests where we run it in-app
   # For other environments, we run it on a standalone Puma server
@@ -7,3 +9,11 @@ Rails.application.configure do
   config.action_cable.url = Gitlab::Utils.append_path(Gitlab.config.gitlab.relative_url_root, '/-/cable')
   config.action_cable.worker_pool_size = Gitlab.config.action_cable.worker_pool_size
 end
+
+# https://github.com/rails/rails/blob/bb5ac1623e8de08c1b7b62b1368758f0d3bb6379/actioncable/lib/action_cable/subscription_adapter/redis.rb#L18
+ActionCable::SubscriptionAdapter::Redis.redis_connector = lambda do |config|
+  args = config.except(:adapter, :channel_prefix)
+    .merge(instrumentation_class: ::Gitlab::Instrumentation::Redis::ActionCable)
+
+  ::Redis.new(args)
+end

lib/gitlab/instrumentation/redis.rb

 # frozen_string_literal: true
 
-require 'redis'
-
 module Gitlab
   module Instrumentation
-    module RedisInterceptor
-      def call(*args, &block)
-        start = Time.now
-        super(*args, &block)
-      ensure
-        duration = (Time.now - start)
-
-        if ::RequestStore.active?
-          ::Gitlab::Instrumentation::Redis.increment_request_count
-          ::Gitlab::Instrumentation::Redis.add_duration(duration)
-          ::Gitlab::Instrumentation::Redis.add_call_details(duration, args)
-        end
-      end
-    end
-
+    # Aggregates Redis measurements from different request storage sources.
     class Redis
-      REDIS_REQUEST_COUNT = :redis_request_count
-      REDIS_CALL_DURATION = :redis_call_duration
-      REDIS_CALL_DETAILS = :redis_call_details
-      REDIS_READ_BYTES = :redis_read_bytes
-      REDIS_WRITE_BYTES = :redis_write_bytes
+      ActionCable = Class.new(RedisBase)
+      Cache = Class.new(RedisBase)
+      Queues = Class.new(RedisBase)
+      SharedState = Class.new(RedisBase)
+
+      STORAGES = [ActionCable, Cache, Queues, SharedState].freeze
 
       # Milliseconds represented in seconds (from 1 to 500 milliseconds).
       QUERY_TIME_BUCKETS = [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5].freeze
 
-      def self.get_request_count
-        ::RequestStore[REDIS_REQUEST_COUNT] || 0
-      end
-
-      def self.increment_request_count
-        ::RequestStore[REDIS_REQUEST_COUNT] ||= 0
-        ::RequestStore[REDIS_REQUEST_COUNT] += 1
-      end
-
-      def self.increment_read_bytes(num_bytes)
-        ::RequestStore[REDIS_READ_BYTES] ||= 0
-        ::RequestStore[REDIS_READ_BYTES] += num_bytes
-      end
-
-      def self.increment_write_bytes(num_bytes)
-        ::RequestStore[REDIS_WRITE_BYTES] ||= 0
-        ::RequestStore[REDIS_WRITE_BYTES] += num_bytes
-      end
-
-      def self.read_bytes
-        ::RequestStore[REDIS_READ_BYTES] || 0
-      end
-
-      def self.write_bytes
-        ::RequestStore[REDIS_WRITE_BYTES] || 0
-      end
-
-      def self.detail_store
-        ::RequestStore[REDIS_CALL_DETAILS] ||= []
-      end
-
-      def self.query_time
-        query_time = ::RequestStore[REDIS_CALL_DURATION] || 0
-        query_time.round(::Gitlab::InstrumentationHelper::DURATION_PRECISION)
-      end
-
-      def self.add_duration(duration)
-        ::RequestStore[REDIS_CALL_DURATION] ||= 0
-        ::RequestStore[REDIS_CALL_DURATION] += duration
-      end
-
-      def self.add_call_details(duration, args)
-        return unless Gitlab::PerformanceBar.enabled_for_request?
-        # redis-rb passes an array (e.g. [:get, key])
-        return unless args.length == 1
+      class << self
+        def detail_store
+          STORAGES.flat_map(&:detail_store)
+        end
 
-        detail_store << {
-          cmd: args.first,
-          duration: duration,
-          backtrace: ::Gitlab::BacktraceCleaner.clean_backtrace(caller)
-        }
+        %i[get_request_count query_time read_bytes write_bytes].each do |method|
+          define_method method do
+            STORAGES.sum(&method) # rubocop:disable CodeReuse/ActiveRecord
+          end
+        end
       end
     end
   end
 end
-
-class ::Redis::Client
-  prepend ::Gitlab::Instrumentation::RedisInterceptor
-end

lib/gitlab/instrumentation/redis_base.rb

+# frozen_string_literal: true
+
+require 'redis'
+
+module Gitlab
+  module Instrumentation
+    class RedisBase
+      class << self
+        include ::Gitlab::Utils::StrongMemoize
+
+        # TODO: To be used by https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/395
+        # as a 'label' alias.
+        def storage_key
+          self.name.underscore
+        end
+
+        def add_duration(duration)
+          ::RequestStore[call_duration_key] ||= 0
+          ::RequestStore[call_duration_key] += duration
+        end
+
+        def add_call_details(duration, args)
+          return unless Gitlab::PerformanceBar.enabled_for_request?
+          # redis-rb passes an array (e.g. [[:get, key]])
+          return unless args.length == 1
+
+          # TODO: Add information about current Redis client
+          # being instrumented.
+          # https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/316.
+          detail_store << {
+            cmd: args.first,
+            duration: duration,
+            backtrace: ::Gitlab::BacktraceCleaner.clean_backtrace(caller)
+          }
+        end
+
+        def increment_request_count
+          ::RequestStore[request_count_key] ||= 0
+          ::RequestStore[request_count_key] += 1
+        end
+
+        def increment_read_bytes(num_bytes)
+          ::RequestStore[read_bytes_key] ||= 0
+          ::RequestStore[read_bytes_key] += num_bytes
+        end
+
+        def increment_write_bytes(num_bytes)
+          ::RequestStore[write_bytes_key] ||= 0
+          ::RequestStore[write_bytes_key] += num_bytes
+        end
+
+        def get_request_count
+          ::RequestStore[request_count_key] || 0
+        end
+
+        def read_bytes
+          ::RequestStore[read_bytes_key] || 0
+        end
+
+        def write_bytes
+          ::RequestStore[write_bytes_key] || 0
+        end
+
+        def detail_store
+          ::RequestStore[call_details_key] ||= []
+        end
+
+        def query_time
+          query_time = ::RequestStore[call_duration_key] || 0
+          query_time.round(::Gitlab::InstrumentationHelper::DURATION_PRECISION)
+        end
+
+        private
+
+        def request_count_key
+          strong_memoize(:request_count_key) { build_key(:redis_request_count) }
+        end
+
+        def read_bytes_key
+          strong_memoize(:read_bytes_key) { build_key(:redis_read_bytes) }
+        end
+
+        def write_bytes_key
+          strong_memoize(:write_bytes_key) { build_key(:redis_write_bytes) }
+        end
+
+        def call_duration_key
+          strong_memoize(:call_duration_key) { build_key(:redis_call_duration) }
+        end
+
+        def call_details_key
+          strong_memoize(:call_details_key) { build_key(:redis_call_details) }
+        end
+
+        def build_key(namespace)
+          "#{storage_key}_#{namespace}"
+        end
+      end
+    end
+  end
+end

lib/gitlab/instrumentation/redis_driver.rb → lib/gitlab/instrumentation/redis_interceptor.rb

@@ -4,7 +4,20 @@ require 'redis'
 
 module Gitlab
   module Instrumentation
-    class RedisDriver < ::Redis::Connection::Ruby
+    module RedisInterceptor
+      def call(*args, &block)
+        start = Time.now
+        super(*args, &block)
+      ensure
+        duration = (Time.now - start)
+
+        if ::RequestStore.active?
+          instrumentation_class.increment_request_count
+          instrumentation_class.add_duration(duration)
+          instrumentation_class.add_call_details(duration, args)
+        end
+      end
+
       def write(command)
         measure_write_size(command) if ::RequestStore.active?
         super
@@ -35,27 +48,40 @@ module Gitlab
           end
         end
 
-        ::Gitlab::Instrumentation::Redis.increment_write_bytes(size)
+        instrumentation_class.increment_write_bytes(size)
       end
 
       def measure_read_size(result)
-        # The superclass can return one of four types of results from read:
+        # The Connection::Ruby#read class can return one of four types of results from read:
         # https://github.com/redis/redis-rb/blob/f597f21a6b954b685cf939febbc638f6c803e3a7/lib/redis/connection/ruby.rb#L406
         #
         # 1. Error (exception, will not reach this line)
         # 2. Status (string)
         # 3. Integer (will be converted to string by to_s.bytesize and thrown away)
         # 4. "Binary" string (i.e. may contain zero byte)
-        # 5. Array of binary string (recurses back into read)
-
-        # Avoid double-counting array responses: the array elements themselves
-        # are retrieved with 'read'.
-        unless result.is_a? Array
-          # This count is an approximation that omits the Redis protocol overhead
-          # of type prefixes, length prefixes and line endings.
-          ::Gitlab::Instrumentation::Redis.increment_read_bytes(result.to_s.bytesize)
-        end
+        # 5. Array of binary string
+
+        size = if result.is_a? Array
+                 # This count is an approximation that omits the Redis protocol overhead
+                 # of type prefixes, length prefixes and line endings.
+                 result.inject(0) { |sum, y| sum + y.to_s.bytesize }
+               else
+                 result.to_s.bytesize
+               end
+
+        instrumentation_class.increment_read_bytes(size)
+      end
+
+      # That's required so it knows which GitLab Redis instance
+      # it's interacting with in order to categorize accordingly.
+      #
+      def instrumentation_class
+        @options[:instrumentation_class] # rubocop:disable Gitlab/ModuleWithInstanceVariables
       end
     end
   end
 end
+
+class ::Redis::Client
+  prepend ::Gitlab::Instrumentation::RedisInterceptor
+end

lib/gitlab/redis/cache.rb

@@ -27,6 +27,10 @@ module Gitlab
           # this will force use of DEFAULT_REDIS_QUEUES_URL when config file is absent
           super
         end
+
+        def instrumentation_class
+          ::Gitlab::Instrumentation::Redis::Cache
+        end
       end
     end
   end

lib/gitlab/redis/queues.rb

@@ -28,6 +28,10 @@ module Gitlab
           # this will force use of DEFAULT_REDIS_QUEUES_URL when config file is absent
           super
         end
+
+        def instrumentation_class
+          ::Gitlab::Instrumentation::Redis::Queues
+        end
       end
     end
   end

lib/gitlab/redis/shared_state.rb

@@ -30,6 +30,10 @@ module Gitlab
           # this will force use of DEFAULT_REDIS_SHARED_STATE_URL when config file is absent
           super
         end
+
+        def instrumentation_class
+          ::Gitlab::Instrumentation::Redis::SharedState
+        end
       end
     end
   end

lib/gitlab/redis/wrapper.rb

@@ -71,6 +71,10 @@ module Gitlab
           # nil will force use of DEFAULT_REDIS_URL when config file is absent
           nil
         end
+
+        def instrumentation_class
+          raise NotImplementedError
+        end
       end
 
       def initialize(rails_env = nil)
@@ -100,7 +104,7 @@ module Gitlab
         redis_url = config.delete(:url)
         redis_uri = URI.parse(redis_url)
 
-        config[:driver] ||= ::Gitlab::Instrumentation::RedisDriver
+        config[:instrumentation_class] ||= self.class.instrumentation_class
 
         if redis_uri.scheme == 'unix'
           # Redis::Store does not handle Unix sockets well, so let's do it for them

lib/peek/views/redis_detailed.rb

@@ -9,6 +9,10 @@ module Peek
         'redis'
       end
 
+      def detail_store
+        ::Gitlab::Instrumentation::Redis.detail_store
+      end
+
       private
 
       def format_call_details(call)

spec/lib/gitlab/instrumentation/redis_base_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Instrumentation::RedisBase, :request_store do
+  let(:instrumentation_class_a) do
+    stub_const('RedisInstanceA', Class.new(described_class))
+  end
+
+  let(:instrumentation_class_b) do
+    stub_const('RedisInstanceB', Class.new(described_class))
+  end
+
+  describe '.storage_key' do
+    it 'returns the class name with underscore' do
+      expect(instrumentation_class_a.storage_key).to eq('redis_instance_a')
+      expect(instrumentation_class_b.storage_key).to eq('redis_instance_b')
+    end
+  end
+
+  describe '.add_duration' do
+    it 'does not lose precision while adding' do
+      precision = 1.0 / (10**::Gitlab::InstrumentationHelper::DURATION_PRECISION)
+      2.times { instrumentation_class_a.add_duration(0.4 * precision) }
+
+      # 2 * 0.4 should be 0.8 and get rounded to 1
+      expect(instrumentation_class_a.query_time).to eq(1 * precision)
+    end
+
+    context 'storage key overlapping' do
+      it 'keys do not overlap across storages' do
+        instrumentation_class_a.add_duration(0.4)
+        instrumentation_class_b.add_duration(0.5)
+
+        expect(instrumentation_class_a.query_time).to eq(0.4)
+        expect(instrumentation_class_b.query_time).to eq(0.5)
+      end
+    end
+  end
+
+  describe '.increment_request_count' do
+    context 'storage key overlapping' do
+      it 'keys do not overlap across storages' do
+        3.times { instrumentation_class_a.increment_request_count }
+        2.times { instrumentation_class_b.increment_request_count }
+
+        expect(instrumentation_class_a.get_request_count).to eq(3)
+        expect(instrumentation_class_b.get_request_count).to eq(2)
+      end
+    end
+  end
+
+  describe '.increment_write_bytes' do
+    context 'storage key overlapping' do
+      it 'keys do not overlap across storages' do
+        2.times do
+          instrumentation_class_a.increment_write_bytes(42)
+          instrumentation_class_b.increment_write_bytes(77)
+        end
+
+        expect(instrumentation_class_a.write_bytes).to eq(42 * 2)
+        expect(instrumentation_class_b.write_bytes).to eq(77 * 2)
+      end
+    end
+  end
+
+  describe '.increment_read_bytes' do
+    context 'storage key overlapping' do
+      it 'keys do not overlap across storages' do
+        2.times do
+          instrumentation_class_a.increment_read_bytes(42)
+          instrumentation_class_b.increment_read_bytes(77)
+        end
+
+        expect(instrumentation_class_a.read_bytes).to eq(42 * 2)
+        expect(instrumentation_class_b.read_bytes).to eq(77 * 2)
+      end
+    end
+  end
+
+  describe '.add_call_details' do
+    before do
+      allow(Gitlab::PerformanceBar).to receive(:enabled_for_request?) { true }
+    end
+
+    context 'storage key overlapping' do
+      it 'keys do not overlap across storages' do
+        2.times do
+          instrumentation_class_a.add_call_details(0.3, [:set])
+          instrumentation_class_b.add_call_details(0.4, [:set])
+        end
+
+        expect(instrumentation_class_a.detail_store).to match(
+          [
+            a_hash_including(cmd: :set, duration: 0.3, backtrace: an_instance_of(Array)),
+            a_hash_including(cmd: :set, duration: 0.3, backtrace: an_instance_of(Array))
+          ]
+        )
+
+        expect(instrumentation_class_b.detail_store).to match(
+          [
+            a_hash_including(cmd: :set, duration: 0.4, backtrace: an_instance_of(Array)),
+            a_hash_including(cmd: :set, duration: 0.4, backtrace: an_instance_of(Array))
+          ]
+        )
+      end
+    end
+  end
+end

spec/lib/gitlab/instrumentation/redis_driver_spec.rb → spec/lib/gitlab/instrumentation/redis_interceptor_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require 'rspec-parameterized'
 
-describe Gitlab::Instrumentation::RedisDriver, :clean_gitlab_redis_shared_state, :request_store do
+describe Gitlab::Instrumentation::RedisInterceptor, :clean_gitlab_redis_shared_state, :request_store do
   using RSpec::Parameterized::TableSyntax
 
   describe 'read and write' do

spec/lib/gitlab/instrumentation/redis_spec.rb

@@ -3,13 +3,26 @@
 require 'spec_helper'
 
 describe Gitlab::Instrumentation::Redis do
-  describe '.add_duration', :request_store do
-    it 'does not lose precision while adding' do
-      precision = 1.0 / (10**::Gitlab::InstrumentationHelper::DURATION_PRECISION)
-      2.times { described_class.add_duration(0.4 * precision) }
+  def stub_storages(method, value)
+    described_class::STORAGES.each do |storage|
+      allow(storage).to receive(method) { value }
+    end
+  end
+
+  shared_examples 'aggregation of redis storage data' do |method|
+    describe "#{method} sum" do
+      it "sums data from all Redis storages" do
+        amount = 0.3
 
-      # 2 * 0.4 should be 0.8 and get rounded to 1
-      expect(described_class.query_time).to eq(1 * precision)
+        stub_storages(method, amount)
+
+        expect(described_class.public_send(method)).to eq(described_class::STORAGES.size * amount)
+      end
     end
   end
+
+  it_behaves_like 'aggregation of redis storage data', :get_request_count
+  it_behaves_like 'aggregation of redis storage data', :query_time
+  it_behaves_like 'aggregation of redis storage data', :read_bytes
+  it_behaves_like 'aggregation of redis storage data', :write_bytes
 end

spec/lib/gitlab/redis/wrapper_spec.rb

@@ -18,7 +18,21 @@ describe Gitlab::Redis::Wrapper do
   let(:config_env_variable_url) {"TEST_GITLAB_REDIS_URL"}
   let(:class_redis_url) { Gitlab::Redis::Wrapper::DEFAULT_REDIS_URL }
 
-  include_examples "redis_shared_examples"
+  include_examples "redis_shared_examples" do
+    before do
+      allow(described_class).to receive(:instrumentation_class) do
+        ::Gitlab::Instrumentation::Redis::Cache
+      end
+    end
+  end
+
+  describe '.instrumentation_class' do
+    it 'raises a NotImplementedError' do
+      expect(described_class).to receive(:instrumentation_class).and_call_original
+
+      expect { described_class.instrumentation_class }.to raise_error(NotImplementedError)
+    end
+  end
 
   describe '.config_file_path' do
     it 'returns the absolute path to the configuration file' do

spec/lib/peek/views/redis_detailed_spec.rb

@@ -17,7 +17,7 @@ describe Peek::Views::RedisDetailed, :request_store do
 
   with_them do
     it 'scrubs Redis commands' do
-      subject.detail_store << { cmd: cmd, duration: 1.second }
+      Gitlab::Instrumentation::Redis::SharedState.detail_store << { cmd: cmd, duration: 1.second }
 
       expect(subject.results[:details].count).to eq(1)
       expect(subject.results[:details].first)
@@ -29,11 +29,12 @@ describe Peek::Views::RedisDetailed, :request_store do
   end
 
   it 'returns aggregated results' do
-    subject.detail_store << { cmd: [:get, 'test'], duration: 0.001 }
-    subject.detail_store << { cmd: [:get, 'test'], duration: 1.second }
+    Gitlab::Instrumentation::Redis::Cache.detail_store << { cmd: [:get, 'test'], duration: 0.001 }
+    Gitlab::Instrumentation::Redis::Cache.detail_store << { cmd: [:get, 'test'], duration: 1.second }
+    Gitlab::Instrumentation::Redis::SharedState.detail_store << { cmd: [:get, 'test'], duration: 1.second }
 
-    expect(subject.results[:calls]).to eq(2)
-    expect(subject.results[:duration]).to eq('1001.00ms')
-    expect(subject.results[:details].count).to eq(2)
+    expect(subject.results[:calls]).to eq(3)
+    expect(subject.results[:duration]).to eq('2001.00ms')
+    expect(subject.results[:details].count).to eq(3)
   end
 end