From 7d716cc849ca5435cd5c038bdf924d5a995b6b93 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 19 Jan 2018 18:12:19 +0100 Subject: [PATCH] Convert InfluxDB to concern. Fix uninitialized metrics when metrics code is inherited. --- lib/gitlab/metrics.rb | 2 +- lib/gitlab/metrics/concern.rb | 9 +- lib/gitlab/metrics/influx_db.rb | 316 +++++++++--------- lib/gitlab/metrics/prometheus.rb | 3 +- .../metrics/subscribers/rails_cache_spec.rb | 7 +- 5 files changed, 172 insertions(+), 165 deletions(-) diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb index ba849892f72..7d63ca5627d 100644 --- a/lib/gitlab/metrics.rb +++ b/lib/gitlab/metrics.rb @@ -1,6 +1,6 @@ module Gitlab module Metrics - extend Gitlab::Metrics::InfluxDb + include Gitlab::Metrics::InfluxDb include Gitlab::Metrics::Prometheus def self.enabled? diff --git a/lib/gitlab/metrics/concern.rb b/lib/gitlab/metrics/concern.rb index 451dd45c722..d9d69d8026e 100644 --- a/lib/gitlab/metrics/concern.rb +++ b/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 diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb index 32bf441663a..1fbe030681e 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -1,184 +1,188 @@ 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 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 - - 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) - end - end - - new_hash - 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 influx_metrics_enabled? + settings[:enabled] || false end - real_duration_seconds.observe(trans.labels, real_time) + # Prometheus histogram buckets used for arbitrary code measurements - 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" + 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 - cpu_duration_seconds.observe(trans.labels, cpu_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) + def mri? + RUBY_ENGINE == 'ruby' + end - retval - 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 - # Sets the action of the current transaction (if any) - # - # action - The name of the action. - def action=(action) - trans = current_transaction + def submit_metrics(metrics) + prepared = prepare_metrics(metrics) - trans&.action = action - end - - # Tracks an event. - # - # See `Gitlab::Metrics::Transaction#add_event` for more details. - def add_event(*args) - trans = current_transaction - - trans&.add_event(*args) - 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 }) + pool&.with do |connection| + prepared.each_slice(settings[:packet_size]) do |slice| + begin + connection.write_points(slice) + rescue StandardError end end end - - @pool + 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 + + 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 + + new_hash + 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 + end + + real_duration_seconds.observe(trans.labels, real_time) + + 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) + + # 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) + + retval + end + + # Sets the action of the current transaction (if any) + # + # action - The name of the action. + def action=(action) + trans = current_transaction + + trans&.action = action + end + + # Tracks an event. + # + # See `Gitlab::Metrics::Transaction#add_event` for more details. + def add_event(*args) + trans = current_transaction + + trans&.add_event(*args) + 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 }) + end + end + end + + @pool + end + end + # rubocop:enable Gitlab/ModuleWithInstanceVariables end - # rubocop:enable Gitlab/ModuleWithInstanceVariables end end end diff --git a/lib/gitlab/metrics/prometheus.rb b/lib/gitlab/metrics/prometheus.rb index 3fd8d0e1414..9c645ad6af4 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/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? diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb index 58e28592cf9..6795c1ab56b 100644 --- a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb +++ b/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