Convert InfluxDB to concern. Fix uninitialized metrics when metrics code is inherited.

lib/gitlab/metrics.rb

 module Gitlab
   module Metrics
-    extend Gitlab::Metrics::InfluxDb
+    include Gitlab::Metrics::InfluxDb
     include Gitlab::Metrics::Prometheus
 
     def self.enabled?

lib/gitlab/metrics/concern.rb

@@ -6,7 +6,7 @@ module Gitlab
       extend ActiveSupport::Concern
 
       included do
-        @_metric_provider_mutex = Mutex.new
+        @@_metric_provider_mutex = Mutex.new
         @_metrics_provider_cache = {}
       end
 
@@ -23,12 +23,12 @@ module Gitlab
           end
 
           define_singleton_method(name) do
-            @_metrics_provider_cache[name] || init_metric(type, name, opts, &block)
+            @_metrics_provider_cache&.[](name) || init_metric(type, name, opts, &block)
           end
         end
 
         def fetch_metric(type, name, opts = {}, &block)
-          @_metrics_provider_cache[name] || init_metric(type, name, opts, &block)
+          @_metrics_provider_cache&.[](name) || init_metric(type, name, opts, &block)
         end
 
         def init_metric(type, name, opts = {}, &block)
@@ -43,7 +43,8 @@ module Gitlab
         end
 
         def synchronized_cache_fill(key)
-          @_metric_provider_mutex.synchronize do
+          @@_metric_provider_mutex.synchronize do
+            @_metrics_provider_cache ||= {}
             @_metrics_provider_cache[key] ||= yield
           end
         end

lib/gitlab/metrics/influx_db.rb

 module Gitlab
   module Metrics
     module InfluxDb
+      extend ActiveSupport::Concern
       include Gitlab::Metrics::Concern
-      include Gitlab::CurrentSettings
-      extend self
 
-      MUTEX = Mutex.new
-      private_constant :MUTEX
-
-      def influx_metrics_enabled?
-        settings[:enabled] || false
-      end
-
-      # Prometheus histogram buckets used for arbitrary code measurements
       EXECUTION_MEASUREMENT_BUCKETS = [0.001, 0.01, 0.1, 1].freeze
       RAILS_ROOT = Rails.root.to_s
       METRICS_ROOT = Rails.root.join('lib', 'gitlab', 'metrics').to_s
       PATH_REGEX = /^#{RAILS_ROOT}\/?/
 
-      def settings
-        @settings ||= {
-          enabled: current_application_settings[:metrics_enabled],
-          pool_size: current_application_settings[:metrics_pool_size],
-          timeout: current_application_settings[:metrics_timeout],
-          method_call_threshold: current_application_settings[:metrics_method_call_threshold],
-          host: current_application_settings[:metrics_host],
-          port: current_application_settings[:metrics_port],
-          sample_interval: current_application_settings[:metrics_sample_interval] || 15,
-          packet_size: current_application_settings[:metrics_packet_size] || 1
-        }
-      end
+      MUTEX = Mutex.new
+      private_constant :MUTEX
 
-      def mri?
-        RUBY_ENGINE == 'ruby'
-      end
+      class_methods do
+        include Gitlab::CurrentSettings
 
-      def method_call_threshold
-        # This is memoized since this method is called for every instrumented
-        # method. Loading data from an external cache on every method call slows
-        # things down too much.
-        # in milliseconds
-        @method_call_threshold ||= settings[:method_call_threshold]
-      end
+        def influx_metrics_enabled?
+          settings[:enabled] || false
+        end
 
-      def submit_metrics(metrics)
-        prepared = prepare_metrics(metrics)
+        # Prometheus histogram buckets used for arbitrary code measurements
+
+        def settings
+          @settings ||= {
+            enabled: current_application_settings[:metrics_enabled],
+            pool_size: current_application_settings[:metrics_pool_size],
+            timeout: current_application_settings[:metrics_timeout],
+            method_call_threshold: current_application_settings[:metrics_method_call_threshold],
+            host: current_application_settings[:metrics_host],
+            port: current_application_settings[:metrics_port],
+            sample_interval: current_application_settings[:metrics_sample_interval] || 15,
+            packet_size: current_application_settings[:metrics_packet_size] || 1
+          }
+        end
+
+        def mri?
+          RUBY_ENGINE == 'ruby'
+        end
+
+        def method_call_threshold
+          # This is memoized since this method is called for every instrumented
+          # method. Loading data from an external cache on every method call slows
+          # things down too much.
+          # in milliseconds
+          @method_call_threshold ||= settings[:method_call_threshold]
+        end
 
-        pool&.with do |connection|
-          prepared.each_slice(settings[:packet_size]) do |slice|
-            begin
-              connection.write_points(slice)
-            rescue StandardError
+        def submit_metrics(metrics)
+          prepared = prepare_metrics(metrics)
+
+          pool&.with do |connection|
+            prepared.each_slice(settings[:packet_size]) do |slice|
+              begin
+                connection.write_points(slice)
+              rescue StandardError
+              end
             end
           end
+        rescue Errno::EADDRNOTAVAIL, SocketError => ex
+          Gitlab::EnvironmentLogger.error('Cannot resolve InfluxDB address. GitLab Performance Monitoring will not work.')
+          Gitlab::EnvironmentLogger.error(ex)
         end
-      rescue Errno::EADDRNOTAVAIL, SocketError => ex
-        Gitlab::EnvironmentLogger.error('Cannot resolve InfluxDB address. GitLab Performance Monitoring will not work.')
-        Gitlab::EnvironmentLogger.error(ex)
-      end
 
-      def prepare_metrics(metrics)
-        metrics.map do |hash|
-          new_hash = hash.symbolize_keys
+        def prepare_metrics(metrics)
+          metrics.map do |hash|
+            new_hash = hash.symbolize_keys
 
-          new_hash[:tags].each do |key, value|
-            if value.blank?
-              new_hash[:tags].delete(key)
-            else
-              new_hash[:tags][key] = escape_value(value)
+            new_hash[:tags].each do |key, value|
+              if value.blank?
+                new_hash[:tags].delete(key)
+              else
+                new_hash[:tags][key] = escape_value(value)
+              end
             end
-          end
 
-          new_hash
+            new_hash
+          end
         end
-      end
 
-      def escape_value(value)
-        value.to_s.gsub('=', '\\=')
-      end
-
-      # Measures the execution time of a block.
-      #
-      # Example:
-      #
-      #     Gitlab::Metrics.measure(:find_by_username_duration) do
-      #       User.find_by_username(some_username)
-      #     end
-      #
-      # name - The name of the field to store the execution time in.
-      #
-      # Returns the value yielded by the supplied block.
-      def measure(name)
-        trans = current_transaction
-
-        return yield unless trans
-
-        real_start = Time.now.to_f
-        cpu_start = System.cpu_time
-
-        retval = yield
-
-        cpu_stop = System.cpu_time
-        real_stop = Time.now.to_f
-
-        real_time = (real_stop - real_start)
-        cpu_time = cpu_stop - cpu_start
-
-        real_duration_seconds = fetch_histogram("gitlab_#{name}_real_duration_seconds".to_sym) do
-          docstring "Measure #{name}"
-          base_labels Transaction::BASE_LABELS
-          buckets EXECUTION_MEASUREMENT_BUCKETS
+        def escape_value(value)
+          value.to_s.gsub('=', '\\=')
         end
 
-        real_duration_seconds.observe(trans.labels, real_time)
+        # Measures the execution time of a block.
+        #
+        # Example:
+        #
+        #     Gitlab::Metrics.measure(:find_by_username_duration) do
+        #       User.find_by_username(some_username)
+        #     end
+        #
+        # name - The name of the field to store the execution time in.
+        #
+        # Returns the value yielded by the supplied block.
+        def measure(name)
+          trans = current_transaction
+
+          return yield unless trans
+
+          real_start = Time.now.to_f
+          cpu_start = System.cpu_time
+
+          retval = yield
+
+          cpu_stop = System.cpu_time
+          real_stop = Time.now.to_f
+
+          real_time = (real_stop - real_start)
+          cpu_time = cpu_stop - cpu_start
+
+          real_duration_seconds = fetch_histogram("gitlab_#{name}_real_duration_seconds".to_sym) do
+            docstring "Measure #{name}"
+            base_labels Transaction::BASE_LABELS
+            buckets EXECUTION_MEASUREMENT_BUCKETS
+          end
 
-        cpu_duration_seconds = fetch_histogram("gitlab_#{name}_cpu_duration_seconds".to_sym) do
-          docstring "Measure #{name}"
-          base_labels Transaction::BASE_LABELS
-          buckets EXECUTION_MEASUREMENT_BUCKETS
-          # with_feature "prometheus_metrics_measure_#{name}_cpu_duration"
-        end
-        cpu_duration_seconds.observe(trans.labels, cpu_time)
+          real_duration_seconds.observe(trans.labels, real_time)
 
-        # InfluxDB stores the _real_time and _cpu_time time values as milliseconds
-        trans.increment("#{name}_real_time", real_time.in_milliseconds, false)
-        trans.increment("#{name}_cpu_time", cpu_time.in_milliseconds, false)
-        trans.increment("#{name}_call_count", 1, false)
+          cpu_duration_seconds = fetch_histogram("gitlab_#{name}_cpu_duration_seconds".to_sym) do
+            docstring "Measure #{name}"
+            base_labels Transaction::BASE_LABELS
+            buckets EXECUTION_MEASUREMENT_BUCKETS
+            # with_feature "prometheus_metrics_measure_#{name}_cpu_duration"
+          end
+          cpu_duration_seconds.observe(trans.labels, cpu_time)
 
-        retval
-      end
+          # InfluxDB stores the _real_time and _cpu_time time values as milliseconds
+          trans.increment("#{name}_real_time", real_time.in_milliseconds, false)
+          trans.increment("#{name}_cpu_time", cpu_time.in_milliseconds, false)
+          trans.increment("#{name}_call_count", 1, false)
 
-      # Sets the action of the current transaction (if any)
-      #
-      # action - The name of the action.
-      def action=(action)
-        trans = current_transaction
+          retval
+        end
 
-        trans&.action = action
-      end
+        # Sets the action of the current transaction (if any)
+        #
+        # action - The name of the action.
+        def action=(action)
+          trans = current_transaction
 
-      # Tracks an event.
-      #
-      # See `Gitlab::Metrics::Transaction#add_event` for more details.
-      def add_event(*args)
-        trans = current_transaction
+          trans&.action = action
+        end
 
-        trans&.add_event(*args)
-      end
+        # Tracks an event.
+        #
+        # See `Gitlab::Metrics::Transaction#add_event` for more details.
+        def add_event(*args)
+          trans = current_transaction
 
-      # Returns the prefix to use for the name of a series.
-      def series_prefix
-        @series_prefix ||= Sidekiq.server? ? 'sidekiq_' : 'rails_'
-      end
+          trans&.add_event(*args)
+        end
 
-      # Allow access from other metrics related middlewares
-      def current_transaction
-        Transaction.current
-      end
+        # Returns the prefix to use for the name of a series.
+        def series_prefix
+          @series_prefix ||= Sidekiq.server? ? 'sidekiq_' : 'rails_'
+        end
+
+        # Allow access from other metrics related middlewares
+        def current_transaction
+          Transaction.current
+        end
 
-      # When enabled this should be set before being used as the usual pattern
-      # "@foo ||= bar" is _not_ thread-safe.
-      # rubocop:disable Gitlab/ModuleWithInstanceVariables
-      def pool
-        if influx_metrics_enabled?
-          if @pool.nil?
-            MUTEX.synchronize do
-              @pool ||= ConnectionPool.new(size: settings[:pool_size], timeout: settings[:timeout]) do
-                host = settings[:host]
-                port = settings[:port]
-
-                InfluxDB::Client
-                  .new(udp: { host: host, port: port })
+        # When enabled this should be set before being used as the usual pattern
+        # "@foo ||= bar" is _not_ thread-safe.
+        # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        def pool
+          if influx_metrics_enabled?
+            if @pool.nil?
+              MUTEX.synchronize do
+                @pool ||= ConnectionPool.new(size: settings[:pool_size], timeout: settings[:timeout]) do
+                  host = settings[:host]
+                  port = settings[:port]
+
+                  InfluxDB::Client
+                    .new(udp: { host: host, port: port })
+                end
               end
             end
-          end
 
-          @pool
+            @pool
+          end
         end
+        # rubocop:enable Gitlab/ModuleWithInstanceVariables
       end
-      # rubocop:enable Gitlab/ModuleWithInstanceVariables
     end
   end
 end

lib/gitlab/metrics/prometheus.rb

@@ -4,13 +4,12 @@ module Gitlab
   module Metrics
     module Prometheus
       extend ActiveSupport::Concern
-      include Gitlab::Metrics::Concern
-      include Gitlab::CurrentSettings
 
       REGISTRY_MUTEX = Mutex.new
       PROVIDER_MUTEX = Mutex.new
 
       class_methods do
+        include Gitlab::CurrentSettings
         include Gitlab::Utils::StrongMemoize
 
         def metrics_folder_present?

spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb

@@ -144,7 +144,10 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
     end
 
     context 'with a transaction' do
+      let(:metric_cache_misses_total) { double('metric_cache_misses_total', increment: nil) }
+
       before do
+        allow(subscriber).to receive(:metric_cache_misses_total).and_return(metric_cache_misses_total)
         allow(subscriber).to receive(:current_transaction)
                                .and_return(transaction)
       end
@@ -157,9 +160,9 @@ describe Gitlab::Metrics::Subscribers::RailsCache do
       end
 
       it 'increments the cache_read_miss total' do
-        expect(subscriber.send(:metric_cache_misses_total)).to receive(:increment).with({})
-
         subscriber.cache_generate(event)
+
+        expect(metric_cache_misses_total).to have_received(:increment).with({})
       end
     end
   end