Merge branch '330643-primary-replica-db-duration' into 'master'

Expose load balancing DB metrics in Sidekiq logs

See merge request gitlab-org/gitlab!61996

ee/lib/ee/gitlab/metrics/subscribers/active_record.rb

@@ -19,11 +19,6 @@ module EE
           class_methods do
             extend ::Gitlab::Utils::Override
 
-            override :known_payload_keys
-            def known_payload_keys
-              super + DB_LOAD_BALANCING_COUNTERS
-            end
-
             override :db_counter_payload
             def db_counter_payload
               super.tap do |payload|

ee/spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb

@@ -75,10 +75,12 @@ RSpec.describe Gitlab::SidekiqLogging::StructuredLogger do
             'db_write_count' => 0,
             'db_replica_count' => 0,
             'db_replica_cached_count' => 0,
+            'db_replica_wal_count' => 0,
+            'db_replica_duration_s' => a_value >= 0,
             'db_primary_count' => a_value >= 1,
             'db_primary_cached_count' => 0,
             'db_primary_wal_count' => 0,
-            'db_replica_wal_count' => 0
+            'db_primary_duration_s' => a_value > 0
           )
         end
 
@@ -95,10 +97,12 @@ RSpec.describe Gitlab::SidekiqLogging::StructuredLogger do
             'db_write_count' => 0,
             'db_replica_count' => 0,
             'db_replica_cached_count' => 0,
+            'db_replica_wal_count' => 0,
+            'db_replica_duration_s' => 0,
             'db_primary_count' => 0,
             'db_primary_cached_count' => 0,
             'db_primary_wal_count' => 0,
-            'db_replica_wal_count' => 0
+            'db_primary_duration_s' => 0
           )
         end
 

ee/spec/lib/gitlab/sidekiq_middleware/instrumentation_logger_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::SidekiqMiddleware::InstrumentationLogger do
-  describe '.keys' do
-    it 'contains load balancer keys' do
-      expected_keys = [
-        :db_replica_count,
-        :db_replica_cached_count,
-        :db_primary_count,
-        :db_primary_cached_count,
-        :db_primary_wal_count,
-        :db_replica_wal_count
-      ]
-
-      expect(described_class.keys).to include(*expected_keys)
-    end
-  end
-end

lib/gitlab/instrumentation/redis.rb

@@ -21,10 +21,6 @@ module Gitlab
           nil
         end
 
-        def known_payload_keys
-          super + STORAGES.flat_map(&:known_payload_keys)
-        end
-
         def payload
           super.merge(*STORAGES.flat_map(&:payload))
         end

lib/gitlab/instrumentation/redis_payload.rb

@@ -5,12 +5,6 @@ module Gitlab
     module RedisPayload
       include ::Gitlab::Utils::StrongMemoize
 
-      # Fetches payload keys from the lazy payload (this avoids
-      # unnecessary processing of the values).
-      def known_payload_keys
-        to_lazy_payload.keys
-      end
-
       def payload
         to_lazy_payload.transform_values do |value|
           result = value.call

lib/gitlab/metrics/subscribers/active_record.rb

@@ -51,10 +51,6 @@ module Gitlab
           payload
         end
 
-        def self.known_payload_keys
-          DB_COUNTERS
-        end
-
         private
 
         def ignored_query?(payload)

lib/gitlab/metrics/subscribers/external_http.rb

@@ -14,8 +14,6 @@ module Gitlab
         COUNTER = :external_http_count
         DURATION = :external_http_duration_s
 
-        KNOWN_PAYLOAD_KEYS = [COUNTER, DURATION].freeze
-
         def self.detail_store
           ::Gitlab::SafeRequestStore[DETAIL_STORE] ||= []
         end

lib/gitlab/sidekiq_middleware/instrumentation_logger.rb

@@ -3,24 +3,6 @@
 module Gitlab
   module SidekiqMiddleware
     class InstrumentationLogger
-      def self.keys
-        @keys ||= [
-          :cpu_s,
-          :gitaly_calls,
-          :gitaly_duration_s,
-          :rugged_calls,
-          :rugged_duration_s,
-          :elasticsearch_calls,
-          :elasticsearch_duration_s,
-          :elasticsearch_timed_out_count,
-          *::Gitlab::Memory::Instrumentation::KEY_MAPPING.values,
-          *::Gitlab::Instrumentation::Redis.known_payload_keys,
-          *::Gitlab::Metrics::Subscribers::ActiveRecord.known_payload_keys,
-          *::Gitlab::Metrics::Subscribers::ExternalHttp::KNOWN_PAYLOAD_KEYS,
-          *::Gitlab::Metrics::Subscribers::RackAttack::PAYLOAD_KEYS
-        ]
-      end
-
       def call(worker, job, queue)
         ::Gitlab::InstrumentationHelper.init_instrumentation_data
 
@@ -37,7 +19,6 @@ module Gitlab
         # https://github.com/mperham/sidekiq/blob/53bd529a0c3f901879925b8390353129c465b1f2/lib/sidekiq/processor.rb#L115-L118
         job[:instrumentation] = {}.tap do |instrumentation_values|
           ::Gitlab::InstrumentationHelper.add_instrumentation_data(instrumentation_values)
-          instrumentation_values.slice!(*self.class.keys)
         end
       end
     end

spec/lib/gitlab/instrumentation/redis_base_spec.rb

@@ -18,24 +18,6 @@ RSpec.describe Gitlab::Instrumentation::RedisBase, :request_store do
     end
   end
 
-  describe '.known_payload_keys' do
-    it 'returns generated payload keys' do
-      expect(instrumentation_class_a.known_payload_keys).to eq([:redis_instance_a_calls,
-                                                                :redis_instance_a_duration_s,
-                                                                :redis_instance_a_read_bytes,
-                                                                :redis_instance_a_write_bytes])
-    end
-
-    it 'does not call calculation methods' do
-      expect(instrumentation_class_a).not_to receive(:get_request_count)
-      expect(instrumentation_class_a).not_to receive(:query_time)
-      expect(instrumentation_class_a).not_to receive(:read_bytes)
-      expect(instrumentation_class_a).not_to receive(:write_bytes)
-
-      instrumentation_class_a.known_payload_keys
-    end
-  end
-
   describe '.payload' do
     it 'returns values that are higher than 0' do
       allow(instrumentation_class_a).to receive(:get_request_count) { 1 }

spec/lib/gitlab/instrumentation/redis_spec.rb

@@ -26,46 +26,6 @@ RSpec.describe Gitlab::Instrumentation::Redis do
   it_behaves_like 'aggregation of redis storage data', :read_bytes
   it_behaves_like 'aggregation of redis storage data', :write_bytes
 
-  describe '.known_payload_keys' do
-    it 'returns all known payload keys' do
-      expected_keys = [
-        :redis_calls,
-        :redis_duration_s,
-        :redis_read_bytes,
-        :redis_write_bytes,
-        :redis_action_cable_calls,
-        :redis_action_cable_duration_s,
-        :redis_action_cable_read_bytes,
-        :redis_action_cable_write_bytes,
-        :redis_cache_calls,
-        :redis_cache_duration_s,
-        :redis_cache_read_bytes,
-        :redis_cache_write_bytes,
-        :redis_queues_calls,
-        :redis_queues_duration_s,
-        :redis_queues_read_bytes,
-        :redis_queues_write_bytes,
-        :redis_shared_state_calls,
-        :redis_shared_state_duration_s,
-        :redis_shared_state_read_bytes,
-        :redis_shared_state_write_bytes
-      ]
-
-      expect(described_class.known_payload_keys).to eq(expected_keys)
-    end
-
-    it 'does not call storage calculation methods' do
-      described_class::STORAGES.each do |storage|
-        expect(storage).not_to receive(:get_request_count)
-        expect(storage).not_to receive(:query_time)
-        expect(storage).not_to receive(:read_bytes)
-        expect(storage).not_to receive(:write_bytes)
-      end
-
-      described_class.known_payload_keys
-    end
-  end
-
   describe '.payload', :request_store do
     before do
       Gitlab::Redis::Cache.with { |redis| redis.set('cache-test', 321) }

spec/lib/gitlab/sidekiq_middleware/instrumentation_logger_spec.rb

@@ -24,58 +24,10 @@ RSpec.describe Gitlab::SidekiqMiddleware::InstrumentationLogger do
     stub_const('TestWorker', worker)
   end
 
-  describe '.keys' do
-    it 'returns all available payload keys' do
-      expected_keys = [
-        :cpu_s,
-        :gitaly_calls,
-        :gitaly_duration_s,
-        :rugged_calls,
-        :rugged_duration_s,
-        :elasticsearch_calls,
-        :elasticsearch_duration_s,
-        :elasticsearch_timed_out_count,
-        :mem_objects,
-        :mem_bytes,
-        :mem_mallocs,
-        :redis_calls,
-        :redis_duration_s,
-        :redis_read_bytes,
-        :redis_write_bytes,
-        :redis_action_cable_calls,
-        :redis_action_cable_duration_s,
-        :redis_action_cable_read_bytes,
-        :redis_action_cable_write_bytes,
-        :redis_cache_calls,
-        :redis_cache_duration_s,
-        :redis_cache_read_bytes,
-        :redis_cache_write_bytes,
-        :redis_queues_calls,
-        :redis_queues_duration_s,
-        :redis_queues_read_bytes,
-        :redis_queues_write_bytes,
-        :redis_shared_state_calls,
-        :redis_shared_state_duration_s,
-        :redis_shared_state_read_bytes,
-        :redis_shared_state_write_bytes,
-        :db_count,
-        :db_write_count,
-        :db_cached_count,
-        :external_http_count,
-        :external_http_duration_s,
-        :rack_attack_redis_count,
-        :rack_attack_redis_duration_s
-      ]
-
-      expect(described_class.keys).to include(*expected_keys)
-    end
-  end
-
   describe '#call', :request_store do
     let(:instrumentation_values) do
       {
         cpu_s: 10,
-        unknown_attribute: 123,
         db_count: 0,
         db_cached_count: 0,
         db_write_count: 0,
@@ -90,12 +42,10 @@ RSpec.describe Gitlab::SidekiqMiddleware::InstrumentationLogger do
       end
     end
 
-    it 'merges correct instrumentation data in the job' do
+    it 'merges all instrumentation data in the job' do
       expect { |b| subject.call(worker, job, queue, &b) }.to yield_control
 
-      expected_values = instrumentation_values.except(:unknown_attribute)
-
-      expect(job[:instrumentation]).to eq(expected_values)
+      expect(job[:instrumentation]).to eq(instrumentation_values)
     end
   end
 end