From a6c60127e3e2966b2f29fa6e6e79503b130c2b02 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 31 Dec 2015 17:14:02 +0100 Subject: Removed tracking of raw SQL queries This particular setup had 3 problems: 1. Storing SQL queries as tags is very inefficient as InfluxDB ends up indexing every query (and they can get pretty large). Storing these as values instead means we can't always display the SQL as easily. 2. We already instrument ActiveRecord query methods, thus we already have timing information about database queries. 3. SQL obfuscation is difficult to get right and I'd rather not expose sensitive data by accident. --- lib/gitlab/metrics/obfuscated_sql.rb | 47 ------------------------ lib/gitlab/metrics/subscribers/active_record.rb | 48 ------------------------- 2 files changed, 95 deletions(-) delete mode 100644 lib/gitlab/metrics/obfuscated_sql.rb delete mode 100644 lib/gitlab/metrics/subscribers/active_record.rb (limited to 'lib') diff --git a/lib/gitlab/metrics/obfuscated_sql.rb b/lib/gitlab/metrics/obfuscated_sql.rb deleted file mode 100644 index fe97d7a0534..00000000000 --- a/lib/gitlab/metrics/obfuscated_sql.rb +++ /dev/null @@ -1,47 +0,0 @@ -module Gitlab - module Metrics - # Class for producing SQL queries with sensitive data stripped out. - class ObfuscatedSQL - REPLACEMENT = / - \d+(\.\d+)? # integers, floats - | '.+?' # single quoted strings - | \/.+?(? Date: Thu, 31 Dec 2015 17:47:07 +0100 Subject: Cache InfluxDB settings after the first use This ensures we don't need to load anything from either PostgreSQL or the Rails cache whenever creating new InfluxDB connections. --- lib/gitlab/metrics.rb | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb index 2d266ccfe9e..c5b98a0115e 100644 --- a/lib/gitlab/metrics.rb +++ b/lib/gitlab/metrics.rb @@ -6,16 +6,21 @@ module Gitlab METRICS_ROOT = Rails.root.join('lib', 'gitlab', 'metrics').to_s PATH_REGEX = /^#{RAILS_ROOT}\/?/ - def self.pool_size - current_application_settings[:metrics_pool_size] || 16 - end - - def self.timeout - current_application_settings[:metrics_timeout] || 10 + def self.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], + username: current_application_settings[:metrics_username], + password: current_application_settings[:metrics_password], + port: current_application_settings[:metrics_port] + } end def self.enabled? - current_application_settings[:metrics_enabled] || false + settings[:enabled] || false end def self.mri? @@ -26,8 +31,7 @@ module Gitlab # 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. - @method_call_threshold ||= - (current_application_settings[:metrics_method_call_threshold] || 10) + @method_call_threshold ||= settings[:method_call_threshold] end def self.pool @@ -90,11 +94,11 @@ module Gitlab # When enabled this should be set before being used as the usual pattern # "@foo ||= bar" is _not_ thread-safe. if enabled? - @pool = ConnectionPool.new(size: pool_size, timeout: timeout) do - host = current_application_settings[:metrics_host] - user = current_application_settings[:metrics_username] - pw = current_application_settings[:metrics_password] - port = current_application_settings[:metrics_port] + @pool = ConnectionPool.new(size: settings[:pool_size], timeout: settings[:timeout]) do + host = settings[:host] + user = settings[:username] + pw = settings[:password] + port = settings[:port] InfluxDB::Client. new(udp: { host: host, port: port }, username: user, password: pw) -- cgit v1.2.1 From bd9f86bb8abb4759a0c72f94fb0492b1ff8619b5 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 31 Dec 2015 17:51:12 +0100 Subject: Use separate series for Rails/Sidekiq transactions This removes the need for tagging all metrics with a "process_type" tag. --- lib/gitlab/metrics/metric.rb | 3 +-- lib/gitlab/metrics/rack_middleware.rb | 4 +++- lib/gitlab/metrics/sidekiq_middleware.rb | 4 +++- lib/gitlab/metrics/transaction.rb | 7 +++---- 4 files changed, 10 insertions(+), 8 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics/metric.rb b/lib/gitlab/metrics/metric.rb index 753008df99a..8319e628a40 100644 --- a/lib/gitlab/metrics/metric.rb +++ b/lib/gitlab/metrics/metric.rb @@ -19,8 +19,7 @@ module Gitlab { series: @series, tags: @tags.merge( - hostname: Metrics.hostname, - process_type: Sidekiq.server? ? 'sidekiq' : 'rails' + hostname: Metrics.hostname ), values: @values, timestamp: @created_at.to_i * 1_000_000_000 diff --git a/lib/gitlab/metrics/rack_middleware.rb b/lib/gitlab/metrics/rack_middleware.rb index 5c0587c4c51..bb9e4fcb918 100644 --- a/lib/gitlab/metrics/rack_middleware.rb +++ b/lib/gitlab/metrics/rack_middleware.rb @@ -4,6 +4,8 @@ module Gitlab class RackMiddleware CONTROLLER_KEY = 'action_controller.instance' + SERIES = 'rails_transactions' + def initialize(app) @app = app end @@ -30,7 +32,7 @@ module Gitlab end def transaction_from_env(env) - trans = Transaction.new + trans = Transaction.new(SERIES) trans.add_tag(:request_method, env['REQUEST_METHOD']) trans.add_tag(:request_uri, env['REQUEST_URI']) diff --git a/lib/gitlab/metrics/sidekiq_middleware.rb b/lib/gitlab/metrics/sidekiq_middleware.rb index ad441decfa2..6e804dd2562 100644 --- a/lib/gitlab/metrics/sidekiq_middleware.rb +++ b/lib/gitlab/metrics/sidekiq_middleware.rb @@ -4,8 +4,10 @@ module Gitlab # # This middleware is intended to be used as a server-side middleware. class SidekiqMiddleware + SERIES = 'sidekiq_transactions' + def call(worker, message, queue) - trans = Transaction.new + trans = Transaction.new(SERIES) begin trans.run { yield } diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index a61dbd989e7..43a7dab5323 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -4,8 +4,6 @@ module Gitlab class Transaction THREAD_KEY = :_gitlab_metrics_transaction - SERIES = 'transactions' - attr_reader :uuid, :tags def self.current @@ -13,7 +11,8 @@ module Gitlab end # name - The name of this transaction as a String. - def initialize + def initialize(series) + @series = series @metrics = [] @uuid = SecureRandom.uuid @@ -55,7 +54,7 @@ module Gitlab end def track_self - add_metric(SERIES, { duration: duration }, @tags) + add_metric(@series, { duration: duration }, @tags) end def submit -- cgit v1.2.1 From cafc784ee1d5d0a0279077272af8ee435bb110e4 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 31 Dec 2015 17:55:10 +0100 Subject: Removed tracking of hostnames for metrics This isn't hugely useful and mostly wastes InfluxDB space. We can re-add this whenever needed (but only once we really need it). --- lib/gitlab/metrics.rb | 6 ------ lib/gitlab/metrics/metric.rb | 6 ++---- 2 files changed, 2 insertions(+), 10 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb index c5b98a0115e..ee88ab34d6c 100644 --- a/lib/gitlab/metrics.rb +++ b/lib/gitlab/metrics.rb @@ -38,10 +38,6 @@ module Gitlab @pool end - def self.hostname - @hostname - end - # Returns a relative path and line number based on the last application call # frame. def self.last_relative_application_frame @@ -89,8 +85,6 @@ module Gitlab value.to_s.gsub('=', '\\=') end - @hostname = Socket.gethostname - # When enabled this should be set before being used as the usual pattern # "@foo ||= bar" is _not_ thread-safe. if enabled? diff --git a/lib/gitlab/metrics/metric.rb b/lib/gitlab/metrics/metric.rb index 8319e628a40..7ea9555cc8c 100644 --- a/lib/gitlab/metrics/metric.rb +++ b/lib/gitlab/metrics/metric.rb @@ -17,10 +17,8 @@ module Gitlab # Returns a Hash in a format that can be directly written to InfluxDB. def to_hash { - series: @series, - tags: @tags.merge( - hostname: Metrics.hostname - ), + series: @series, + tags: @tags, values: @values, timestamp: @created_at.to_i * 1_000_000_000 } -- cgit v1.2.1 From 96075be6f4688a59335130dc796132ad4f232442 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 4 Jan 2016 11:37:46 +0100 Subject: Ability to increment custom transaction values This will be used to store/increment the total query/view rendering timings on a per transaction basis. This in turn can greatly reduce the amount of metrics stored. --- lib/gitlab/metrics/transaction.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index 43a7dab5323..0aaebf262d4 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -19,7 +19,8 @@ module Gitlab @started_at = nil @finished_at = nil - @tags = {} + @values = Hash.new(0) + @tags = {} end def duration @@ -44,6 +45,10 @@ module Gitlab @metrics << Metric.new(series, values, tags) end + def increment(name, value) + @values[name] += value + end + def add_tag(key, value) @tags[key] = value end @@ -54,7 +59,13 @@ module Gitlab end def track_self - add_metric(@series, { duration: duration }, @tags) + values = { duration: duration } + + @values.each do |name, value| + values[name] = value + end + + add_metric(@series, values, @tags) end def submit -- cgit v1.2.1 From 66a997a91403eef62ffd9fb789e899619d021a26 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 4 Jan 2016 12:14:36 +0100 Subject: Track total query/view timings in transactions --- lib/gitlab/metrics/subscribers/action_view.rb | 1 + lib/gitlab/metrics/subscribers/active_record.rb | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 lib/gitlab/metrics/subscribers/active_record.rb (limited to 'lib') diff --git a/lib/gitlab/metrics/subscribers/action_view.rb b/lib/gitlab/metrics/subscribers/action_view.rb index 7e0dcf99d92..7c0105d543a 100644 --- a/lib/gitlab/metrics/subscribers/action_view.rb +++ b/lib/gitlab/metrics/subscribers/action_view.rb @@ -19,6 +19,7 @@ module Gitlab values = values_for(event) tags = tags_for(event) + current_transaction.increment(:view_duration, event.duration) current_transaction.add_metric(SERIES, values, tags) end diff --git a/lib/gitlab/metrics/subscribers/active_record.rb b/lib/gitlab/metrics/subscribers/active_record.rb new file mode 100644 index 00000000000..8008b3bc895 --- /dev/null +++ b/lib/gitlab/metrics/subscribers/active_record.rb @@ -0,0 +1,22 @@ +module Gitlab + module Metrics + module Subscribers + # Class for tracking the total query duration of a transaction. + class ActiveRecord < ActiveSupport::Subscriber + attach_to :active_record + + def sql(event) + return unless current_transaction + + current_transaction.increment(:sql_duration, event.duration) + end + + private + + def current_transaction + Transaction.current + end + end + end + end +end -- cgit v1.2.1 From 825b46f8a3eb620f99192217d414b72dffe597d7 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 4 Jan 2016 12:19:45 +0100 Subject: Track total method call times per transaction This makes it easier to see where time is spent without having to aggregate all the individual points in the method_calls series. --- lib/gitlab/metrics/instrumentation.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/metrics/instrumentation.rb b/lib/gitlab/metrics/instrumentation.rb index 06fc2f25948..d9fce2e6758 100644 --- a/lib/gitlab/metrics/instrumentation.rb +++ b/lib/gitlab/metrics/instrumentation.rb @@ -123,6 +123,8 @@ module Gitlab duration = (Time.now - start) * 1000.0 if duration >= Gitlab::Metrics.method_call_threshold + trans.increment(:method_duration, duration) + trans.add_metric(Gitlab::Metrics::Instrumentation::SERIES, { duration: duration }, method: #{label.inspect}) -- cgit v1.2.1 From 2ea464bb272fa52ff34a188a921f0bc90811ca45 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 4 Jan 2016 12:45:31 +0100 Subject: Use separate series for Rails/Sidekiq sample stats This removes the need for any tags to differentiate between Sidekiq and Rails statistics while still being able to separate the two. --- lib/gitlab/metrics/sampler.rb | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics/sampler.rb b/lib/gitlab/metrics/sampler.rb index 998578e1c0a..1ea425bc904 100644 --- a/lib/gitlab/metrics/sampler.rb +++ b/lib/gitlab/metrics/sampler.rb @@ -50,12 +50,11 @@ module Gitlab end def sample_memory_usage - @metrics << Metric.new('memory_usage', value: System.memory_usage) + add_metric('memory_usage', value: System.memory_usage) end def sample_file_descriptors - @metrics << Metric. - new('file_descriptors', value: System.file_descriptor_count) + add_metric('file_descriptors', value: System.file_descriptor_count) end if Metrics.mri? @@ -69,7 +68,7 @@ module Gitlab counts['Symbol'] = Symbol.all_symbols.length counts.each do |name, count| - @metrics << Metric.new('object_counts', { count: count }, type: name) + add_metric('object_counts', { count: count }, type: name) end end else @@ -91,7 +90,17 @@ module Gitlab stats[:count] = stats[:minor_gc_count] + stats[:major_gc_count] - @metrics << Metric.new('gc_statistics', stats) + add_metric('gc_statistics', stats) + end + + def add_metric(series, values, tags = {}) + prefix = sidekiq? ? 'sidekiq_' : 'rails_' + + @metrics << Metric.new("#{prefix}#{series}", values, tags) + end + + def sidekiq? + Sidekiq.server? end end end -- cgit v1.2.1 From 2ee8f555996baca6b470d223ffad65419b730398 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 4 Jan 2016 13:17:02 +0100 Subject: Automatically prefix transaction series names This ensures Rails and Sidekiq transactions are split into the series "rails_transactions" and "sidekiq_transactions" respectively. --- lib/gitlab/metrics/rack_middleware.rb | 4 +--- lib/gitlab/metrics/sidekiq_middleware.rb | 4 +--- lib/gitlab/metrics/transaction.rb | 15 +++++++++------ 3 files changed, 11 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/metrics/rack_middleware.rb b/lib/gitlab/metrics/rack_middleware.rb index bb9e4fcb918..5c0587c4c51 100644 --- a/lib/gitlab/metrics/rack_middleware.rb +++ b/lib/gitlab/metrics/rack_middleware.rb @@ -4,8 +4,6 @@ module Gitlab class RackMiddleware CONTROLLER_KEY = 'action_controller.instance' - SERIES = 'rails_transactions' - def initialize(app) @app = app end @@ -32,7 +30,7 @@ module Gitlab end def transaction_from_env(env) - trans = Transaction.new(SERIES) + trans = Transaction.new trans.add_tag(:request_method, env['REQUEST_METHOD']) trans.add_tag(:request_uri, env['REQUEST_URI']) diff --git a/lib/gitlab/metrics/sidekiq_middleware.rb b/lib/gitlab/metrics/sidekiq_middleware.rb index 6e804dd2562..ad441decfa2 100644 --- a/lib/gitlab/metrics/sidekiq_middleware.rb +++ b/lib/gitlab/metrics/sidekiq_middleware.rb @@ -4,10 +4,8 @@ module Gitlab # # This middleware is intended to be used as a server-side middleware. class SidekiqMiddleware - SERIES = 'sidekiq_transactions' - def call(worker, message, queue) - trans = Transaction.new(SERIES) + trans = Transaction.new begin trans.run { yield } diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index 0aaebf262d4..68b86de0655 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -10,9 +10,7 @@ module Gitlab Thread.current[THREAD_KEY] end - # name - The name of this transaction as a String. - def initialize(series) - @series = series + def initialize @metrics = [] @uuid = SecureRandom.uuid @@ -40,9 +38,10 @@ module Gitlab end def add_metric(series, values, tags = {}) - tags = tags.merge(transaction_id: @uuid) + tags = tags.merge(transaction_id: @uuid) + prefix = sidekiq? ? 'sidekiq_' : 'rails_' - @metrics << Metric.new(series, values, tags) + @metrics << Metric.new("#{prefix}#{series}", values, tags) end def increment(name, value) @@ -65,12 +64,16 @@ module Gitlab values[name] = value end - add_metric(@series, values, @tags) + add_metric('transactions', values, @tags) end def submit Metrics.submit_metrics(@metrics.map(&:to_hash)) end + + def sidekiq? + Sidekiq.server? + end end end end -- cgit v1.2.1