Merge branch 'metrics-tuning' into 'master'

Tuning of metrics data to store

This removes data we don't really need, as well as making sure we don't overload any cache stores or databases.

See merge request !2265

8 files changed, 59 insertions, 112 deletions

diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb
index 2d266ccfe9e..ee88ab34d6c 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,18 +31,13 @@ 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
       @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
@@ -85,16 +85,14 @@ 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?
-      @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)
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})
diff --git a/lib/gitlab/metrics/metric.rb b/lib/gitlab/metrics/metric.rb
index 753008df99a..7ea9555cc8c 100644
--- a/lib/gitlab/metrics/metric.rb
+++ b/lib/gitlab/metrics/metric.rb
@@ -17,11 +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,
-            process_type: Sidekiq.server? ? 'sidekiq' : 'rails'
-          ),
+          series:    @series,
+          tags:      @tags,
           values:    @values,
           timestamp: @created_at.to_i * 1_000_000_000
         }
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
-        | \/.+?(?<!\\)\/ # regexps (including escaped slashes)
-      /x
-
-      MYSQL_REPLACEMENTS = /
-        ".+?" # double quoted strings
-      /x
-
-      # Regex to replace consecutive placeholders with a single one indicating
-      # the length. This can be useful when a "IN" statement uses thousands of
-      # IDs (storing this would just be a waste of space).
-      CONSECUTIVE = /(\?(\s*,\s*)?){2,}/
-
-      # sql - The raw SQL query as a String.
-      def initialize(sql)
-        @sql = sql
-      end
-
-      # Returns a new, obfuscated SQL query.
-      def to_s
-        regex = REPLACEMENT
-
-        if Gitlab::Database.mysql?
-          regex = Regexp.union(regex, MYSQL_REPLACEMENTS)
-        end
-
-        sql = @sql.gsub(regex, '?').gsub(CONSECUTIVE) do |match|
-          "#{match.count(',') + 1} values"
-        end
-
-        # InfluxDB escapes double quotes upon output, so lets get rid of them
-        # whenever we can.
-        if Gitlab::Database.postgresql?
-          sql = sql.delete('"')
-        end
-
-        sql.tr("\n", ' ')
-      end
-    end
-  end
-end
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
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
index d947c128ce2..8008b3bc895 100644
--- a/lib/gitlab/metrics/subscribers/active_record.rb
+++ b/lib/gitlab/metrics/subscribers/active_record.rb
@@ -1,44 +1,18 @@
 module Gitlab
   module Metrics
     module Subscribers
-      # Class for tracking raw SQL queries.
-      #
-      # Queries are obfuscated before being logged to ensure no private data is
-      # exposed via InfluxDB/Grafana.
+      # Class for tracking the total query duration of a transaction.
       class ActiveRecord < ActiveSupport::Subscriber
         attach_to :active_record
 
-        SERIES = 'sql_queries'
-
         def sql(event)
           return unless current_transaction
 
-          values = values_for(event)
-          tags   = tags_for(event)
-
-          current_transaction.add_metric(SERIES, values, tags)
+          current_transaction.increment(:sql_duration, event.duration)
         end
 
         private
 
-        def values_for(event)
-          { duration: event.duration }
-        end
-
-        def tags_for(event)
-          sql  = ObfuscatedSQL.new(event.payload[:sql]).to_s
-          tags = { sql: sql }
-
-          file, line = Metrics.last_relative_application_frame
-
-          if file and line
-            tags[:file] = file
-            tags[:line] = line
-          end
-
-          tags
-        end
-
         def current_transaction
           Transaction.current
         end
diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb
index a61dbd989e7..68b86de0655 100644
--- a/lib/gitlab/metrics/transaction.rb
+++ b/lib/gitlab/metrics/transaction.rb
@@ -4,15 +4,12 @@ module Gitlab
     class Transaction
       THREAD_KEY = :_gitlab_metrics_transaction
 
-      SERIES = 'transactions'
-
       attr_reader :uuid, :tags
 
       def self.current
         Thread.current[THREAD_KEY]
       end
 
-      # name - The name of this transaction as a String.
       def initialize
         @metrics = []
         @uuid    = SecureRandom.uuid
@@ -20,7 +17,8 @@ module Gitlab
         @started_at  = nil
         @finished_at = nil
 
-        @tags = {}
+        @values = Hash.new(0)
+        @tags   = {}
       end
 
       def duration
@@ -40,9 +38,14 @@ 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("#{prefix}#{series}", values, tags)
+      end
 
-        @metrics << Metric.new(series, values, tags)
+      def increment(name, value)
+        @values[name] += value
       end
 
       def add_tag(key, value)
@@ -55,12 +58,22 @@ 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('transactions', values, @tags)
       end
 
       def submit
         Metrics.submit_metrics(@metrics.map(&:to_hash))
       end
+
+      def sidekiq?
+        Sidekiq.server?
+      end
     end
   end
 end