Merge remote-tracking branch 'upstream/master' into ce-to-ee-2018-01-30

# Conflicts:
#	lib/gitlab/metrics/influx_db.rb

[ci skip]

GITALY_SERVER_VERSION

-0.74.0
+0.75.0

Gemfile

@@ -142,7 +142,7 @@ gem 'asciidoctor-plantuml', '0.0.7'
 gem 'rouge', '~> 2.0'
 gem 'truncato', '~> 0.7.9'
 gem 'bootstrap_form', '~> 2.7.0'
-gem 'nokogiri', '~> 1.8.1'
+gem 'nokogiri', '~> 1.8.2'
 
 # Diffs
 gem 'diffy', '~> 3.1.0'
@@ -422,7 +422,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.78.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.82.0', require: 'gitaly'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 

Gemfile.lock

@@ -309,7 +309,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.78.0)
+    gitaly-proto (0.82.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -542,7 +542,7 @@ GEM
     net-ntp (2.1.3)
     net-ssh (4.1.0)
     netrc (0.11.0)
-    nokogiri (1.8.1)
+    nokogiri (1.8.2)
       mini_portile2 (~> 2.3.0)
     numerizer (0.1.1)
     oauth (0.5.1)
@@ -1091,7 +1091,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.78.0)
+  gitaly-proto (~> 0.82.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-license (~> 1.0)
@@ -1138,7 +1138,7 @@ DEPENDENCIES
   net-ldap
   net-ntp
   net-ssh (~> 4.1.0)
-  nokogiri (~> 1.8.1)
+  nokogiri (~> 1.8.2)
   oauth2 (~> 1.4)
   octokit (~> 4.6.2)
   oj (~> 2.17.4)

app/assets/javascripts/users/activity_calendar.js

@@ -98,7 +98,7 @@ export default class ActivityCalendar {
     const secondLastColMonth = this.timestampsTmp[group - 2][0].date.getMonth();
 
     if (lastColMonth !== secondLastColMonth) {
-      extraWidthPadding = 3;
+      extraWidthPadding = 6;
     }
 
     return extraWidthPadding;

app/models/concerns/discussion_on_diff.rb

@@ -37,6 +37,8 @@ module DiscussionOnDiff
 
   # Returns an array of at most 16 highlighted lines above a diff note
   def truncated_diff_lines(highlight: true)
+    return [] if diff_line.nil? && first_note.is_a?(LegacyDiffNote)
+
     lines = highlight ? highlighted_diff_lines : diff_lines
 
     initial_line_index = [diff_line.index - NUMBER_OF_TRUNCATED_DIFF_LINES + 1, 0].max

app/models/project_wiki.rb

@@ -139,6 +139,12 @@ class ProjectWiki
     update_project_activity
   end
 
+  def page_formatted_data(page)
+    page_title, page_dir = page_title_and_dir(page.title)
+
+    wiki.page_formatted_data(title: page_title, dir: page_dir, version: page.version)
+  end
+
   def page_title_and_dir(title)
     title_array = title.split("/")
     title = title_array.pop

app/models/repository.rb

@@ -262,6 +262,8 @@ class Repository
 
     # This will still fail if the file is corrupted (e.g. 0 bytes)
     raw_repository.write_ref(keep_around_ref_name(sha), sha, shell: false)
+  rescue Gitlab::Git::CommandError => ex
+    Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}"
   end
 
   def kept_around?(sha)

app/models/wiki_page.rb

@@ -107,7 +107,10 @@ class WikiPage
 
   # The processed/formatted content of this page.
   def formatted_content
-    @attributes[:formatted_content] ||= @page&.formatted_data
+    # Assuming @page exists, nil formatted_data means we didn't load it
+    # before hand (i.e. page was fetched by Gitaly), so we fetch it separately.
+    # If the page was fetched by Gollum, formatted_data would've been a String.
+    @attributes[:formatted_content] ||= @page&.formatted_data || @wiki.page_formatted_data(@page)
   end
 
   # The markup format for the page.

changelogs/unreleased/21554-mark-new-user-as-external.yml

+---
+title: Login via OAuth now only marks new users as external
+merge_request: 16672
+author:
+type: fixed

changelogs/unreleased/41771-reduce-cardinality-of-metrics.yml

+---
+title: Reduce the number of Prometheus metrics
+merge_request: 16443
+author:
+type: performance

changelogs/unreleased/42160-error-500-loading-merge-request-undefined-method-index-for-nil-nilclass.yml

+---
+title: Fix 500 error when loading a merge request with an invalid comment
+merge_request: 16795
+author:
+type: fixed

changelogs/unreleased/42591-update-nokogiri.yml

+---
+title: Update nokogiri to 1.8.2
+merge_request: 16807
+author:
+type: security

changelogs/unreleased/contribution_calendar_label_cut_off.yml

+---
+title: Contribution calendar label was cut off
+merge_request:
+author: Branka Martinovic
+type: fixed

lib/gitlab/ee_compat_check.rb

@@ -5,7 +5,7 @@ module Gitlab
     DEFAULT_CE_PROJECT_URL = 'https://gitlab.com/gitlab-org/gitlab-ce'.freeze
     EE_REPO_URL = 'https://gitlab.com/gitlab-org/gitlab-ee.git'.freeze
     CHECK_DIR = Rails.root.join('ee_compat_check')
-    IGNORED_FILES_REGEX = /(VERSION|CHANGELOG\.md:\d+)/.freeze
+    IGNORED_FILES_REGEX = %r{VERSION|CHANGELOG\.md|db/schema\.rb}i.freeze
     PLEASE_READ_THIS_BANNER = %Q{
       ============================================================
       ===================== PLEASE READ THIS =====================

lib/gitlab/git/repository.rb

@@ -1106,10 +1106,14 @@ module Gitlab
       end
 
       def write_ref(ref_path, ref, old_ref: nil, shell: true)
-        if shell
-          shell_write_ref(ref_path, ref, old_ref)
-        else
-          rugged_write_ref(ref_path, ref)
+        ref_path = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{ref_path}" unless ref_path.start_with?("refs/") || ref_path == "HEAD"
+
+        gitaly_migrate(:write_ref) do |is_enabled|
+          if is_enabled
+            gitaly_repository_client.write_ref(ref_path, ref, old_ref, shell)
+          else
+            local_write_ref(ref_path, ref, old_ref: old_ref, shell: shell)
+          end
         end
       end
 
@@ -1433,6 +1437,14 @@ module Gitlab
 
       private
 
+      def local_write_ref(ref_path, ref, old_ref: nil, shell: true)
+        if shell
+          shell_write_ref(ref_path, ref, old_ref)
+        else
+          rugged_write_ref(ref_path, ref)
+        end
+      end
+
       def shell_write_ref(ref_path, ref, old_ref)
         raise ArgumentError, "invalid ref_path #{ref_path.inspect}" if ref_path.include?(' ')
         raise ArgumentError, "invalid ref #{ref.inspect}" if ref.include?("\x00")

lib/gitlab/git/wiki.rb

@@ -117,6 +117,20 @@ module Gitlab
         page.url_path
       end
 
+      def page_formatted_data(title:, dir: nil, version: nil)
+        version = version&.id
+
+        @repository.gitaly_migrate(:wiki_page_formatted_data) do |is_enabled|
+          if is_enabled
+            gitaly_wiki_client.get_formatted_data(title: title, dir: dir, version: version)
+          else
+            # We don't use #page because if wiki_find_page feature is enabled, we would
+            # get a page without formatted_data.
+            gollum_find_page(title: title, dir: dir, version: version)&.formatted_data
+          end
+        end
+      end
+
       private
 
       # options:

lib/gitlab/gitaly_client.rb

@@ -6,6 +6,7 @@ require 'grpc/health/v1/health_services_pb'
 
 module Gitlab
   module GitalyClient
+    include Gitlab::Metrics::Methods
     module MigrationStatus
       DISABLED = 1
       OPT_IN = 2
@@ -33,8 +34,6 @@ module Gitlab
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
 
     MUTEX = Mutex.new
-    METRICS_MUTEX = Mutex.new
-    private_constant :MUTEX, :METRICS_MUTEX
 
     class << self
       attr_accessor :query_time
@@ -42,28 +41,14 @@ module Gitlab
 
     self.query_time = 0
 
-    def self.migrate_histogram
-      @migrate_histogram ||=
-        METRICS_MUTEX.synchronize do
-          # If a thread was blocked on the mutex, the value was set already
-          return @migrate_histogram if @migrate_histogram
-
-          Gitlab::Metrics.histogram(:gitaly_migrate_call_duration_seconds,
-                                    "Gitaly migration call execution timings",
-                                    gitaly_enabled: nil, feature: nil)
-        end
+    define_histogram :gitaly_migrate_call_duration_seconds do
+      docstring "Gitaly migration call execution timings"
+      base_labels gitaly_enabled: nil, feature: nil
     end
 
-    def self.gitaly_call_histogram
-      @gitaly_call_histogram ||=
-        METRICS_MUTEX.synchronize do
-          # If a thread was blocked on the mutex, the value was set already
-          return @gitaly_call_histogram if @gitaly_call_histogram
-
-          Gitlab::Metrics.histogram(:gitaly_controller_action_duration_seconds,
-                                    "Gitaly endpoint histogram by controller and action combination",
-                                    Gitlab::Metrics::Transaction::BASE_LABELS.merge(gitaly_service: nil, rpc: nil))
-        end
+    define_histogram :gitaly_controller_action_duration_seconds do
+      docstring "Gitaly endpoint histogram by controller and action combination"
+      base_labels Gitlab::Metrics::Transaction::BASE_LABELS.merge(gitaly_service: nil, rpc: nil)
     end
 
     def self.stub(name, storage)
@@ -145,7 +130,7 @@ module Gitlab
 
       # Keep track, seperately, for the performance bar
       self.query_time += duration
-      gitaly_call_histogram.observe(
+      gitaly_controller_action_duration_seconds.observe(
         current_transaction_labels.merge(gitaly_service: service.to_s, rpc: rpc.to_s),
         duration)
     end
@@ -247,7 +232,7 @@ module Gitlab
             yield is_enabled
           ensure
             total_time = Gitlab::Metrics::System.monotonic_time - start
-            migrate_histogram.observe({ gitaly_enabled: is_enabled, feature: feature }, total_time)
+            gitaly_migrate_call_duration_seconds.observe({ gitaly_enabled: is_enabled, feature: feature }, total_time)
             feature_stack.shift
             Thread.current[:gitaly_feature_stack] = nil if feature_stack.empty?
           end

lib/gitlab/gitaly_client/repository_service.rb

@@ -203,6 +203,22 @@ module Gitlab
           timeout: GitalyClient.default_timeout
         )
       end
+
+      def write_ref(ref_path, ref, old_ref, shell)
+        request = Gitaly::WriteRefRequest.new(
+          repository: @gitaly_repo,
+          ref: ref_path.b,
+          revision: ref.b,
+          shell: shell
+        )
+        request.old_revision = old_ref.b unless old_ref.nil?
+
+        response = GitalyClient.call(@storage, :repository_service, :write_ref, request)
+
+        raise Gitlab::Git::CommandError, encode!(response.error) if response.error.present?
+
+        true
+      end
     end
   end
 end

lib/gitlab/gitaly_client/wiki_service.rb

@@ -127,6 +127,18 @@ module Gitlab
         wiki_file
       end
 
+      def get_formatted_data(title:, dir: nil, version: nil)
+        request = Gitaly::WikiGetFormattedDataRequest.new(
+          repository: @gitaly_repo,
+          title: encode_binary(title),
+          revision: encode_binary(version),
+          directory: encode_binary(dir)
+        )
+
+        response = GitalyClient.call(@repository.storage, :wiki_service, :wiki_get_formatted_data, request)
+        response.reduce("") { |memo, msg| memo << msg.data }
+      end
+
       private
 
       # If a block is given and the yielded value is true, iteration will be

lib/gitlab/metrics.rb

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

lib/gitlab/metrics/method_call.rb

@@ -4,26 +4,15 @@ module Gitlab
   module Metrics
     # Class for tracking timing information about method calls
     class MethodCall
-      @@measurement_enabled_cache = Concurrent::AtomicBoolean.new(false)
-      @@measurement_enabled_cache_expires_at = Concurrent::AtomicReference.new(Time.now.to_i)
-      MUTEX = Mutex.new
+      include Gitlab::Metrics::Methods
       BASE_LABELS = { module: nil, method: nil }.freeze
       attr_reader :real_time, :cpu_time, :call_count, :labels
 
-      def self.call_duration_histogram
-        return @call_duration_histogram if @call_duration_histogram
-
-        MUTEX.synchronize do
-          @call_duration_histogram ||= Gitlab::Metrics.histogram(
-            :gitlab_method_call_duration_seconds,
-            'Method calls real duration',
-            Transaction::BASE_LABELS.merge(BASE_LABELS),
-            [0.01, 0.05, 0.1, 0.5, 1])
-        end
-      end
-
-      def self.measurement_enabled_cache_expires_at
-        @@measurement_enabled_cache_expires_at
+      define_histogram :gitlab_method_call_duration_seconds do
+        docstring 'Method calls real duration'
+        base_labels Transaction::BASE_LABELS.merge(BASE_LABELS)
+        buckets [0.01, 0.05, 0.1, 0.5, 1]
+        with_feature :prometheus_metrics_method_instrumentation
       end
 
       # name - The full name of the method (including namespace) such as
@@ -53,8 +42,8 @@ module Gitlab
         @cpu_time += cpu_time
         @call_count += 1
 
-        if call_measurement_enabled? && above_threshold?
-          self.class.call_duration_histogram.observe(@transaction.labels.merge(labels), real_time)
+        if above_threshold?
+          self.class.gitlab_method_call_duration_seconds.observe(@transaction.labels.merge(labels), real_time)
         end
 
         retval
@@ -78,17 +67,6 @@ module Gitlab
       def above_threshold?
         real_time.in_milliseconds >= Metrics.method_call_threshold
       end
-
-      def call_measurement_enabled?
-        expires_at = @@measurement_enabled_cache_expires_at.value
-        if expires_at < Time.now.to_i
-          if @@measurement_enabled_cache_expires_at.compare_and_set(expires_at, 1.minute.from_now.to_i)
-            @@measurement_enabled_cache.value = Feature.get(:prometheus_metrics_method_instrumentation).enabled?
-          end
-        end
-
-        @@measurement_enabled_cache.value
-      end
     end
   end
 end

lib/gitlab/metrics/methods.rb

+# rubocop:disable Style/ClassVars
+
+module Gitlab
+  module Metrics
+    module Methods
+      extend ActiveSupport::Concern
+
+      included do
+        @@_metric_provider_mutex ||= Mutex.new
+        @@_metrics_provider_cache = {}
+      end
+
+      class_methods do
+        def reload_metric!(name)
+          @@_metrics_provider_cache.delete(name)
+        end
+
+        private
+
+        def define_metric(type, name, opts = {}, &block)
+          if respond_to?(name)
+            raise ArgumentError, "method #{name} already exists"
+          end
+
+          define_singleton_method(name) do
+            # inlining fetch_metric method to avoid method call overhead when instrumenting hot spots
+            @@_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)
+        end
+
+        def init_metric(type, name, opts = {}, &block)
+          options = MetricOptions.new(opts)
+          options.evaluate(&block)
+
+          if disabled_by_feature(options)
+            synchronized_cache_fill(name) { NullMetric.instance }
+          else
+            synchronized_cache_fill(name) { build_metric!(type, name, options) }
+          end
+        end
+
+        def synchronized_cache_fill(key)
+          @@_metric_provider_mutex.synchronize do
+            @@_metrics_provider_cache[key] ||= yield
+          end
+        end
+
+        def disabled_by_feature(options)
+          options.with_feature && !Feature.get(options.with_feature).enabled?
+        end
+
+        def build_metric!(type, name, options)
+          case type
+          when :gauge
+            Gitlab::Metrics.gauge(name, options.docstring, options.base_labels, options.multiprocess_mode)
+          when :counter
+            Gitlab::Metrics.counter(name, options.docstring, options.base_labels)
+          when :histogram
+            Gitlab::Metrics.histogram(name, options.docstring, options.base_labels, options.buckets)
+          when :summary
+            raise NotImplementedError, "summary metrics are not currently supported"
+          else
+            raise ArgumentError, "uknown metric type #{type}"
+          end
+        end
+
+        # Fetch and/or initialize counter metric
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def fetch_counter(name, opts = {}, &block)
+          fetch_metric(:counter, name, opts, &block)
+        end
+
+        # Fetch and/or initialize gauge metric
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def fetch_gauge(name, opts = {}, &block)
+          fetch_metric(:gauge, name, opts, &block)
+        end
+
+        # Fetch and/or initialize histogram metric
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def fetch_histogram(name, opts = {}, &block)
+          fetch_metric(:histogram, name, opts, &block)
+        end
+
+        # Fetch and/or initialize summary metric
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def fetch_summary(name, opts = {}, &block)
+          fetch_metric(:summary, name, opts, &block)
+        end
+
+        # Define metric accessor method for a Counter
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def define_counter(name, opts = {}, &block)
+          define_metric(:counter, name, opts, &block)
+        end
+
+        # Define metric accessor method for a Gauge
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def define_gauge(name, opts = {}, &block)
+          define_metric(:gauge, name, opts, &block)
+        end
+
+        # Define metric accessor method for a Histogram
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def define_histogram(name, opts = {}, &block)
+          define_metric(:histogram, name, opts, &block)
+        end
+
+        # Define metric accessor method for a Summary
+        # @param [Symbol] name
+        # @param [Hash] opts
+        def define_summary(name, opts = {}, &block)
+          define_metric(:summary, name, opts, &block)
+        end
+      end
+    end
+  end
+end

lib/gitlab/metrics/methods/metric_options.rb

+module Gitlab
+  module Metrics
+    module Methods
+      class MetricOptions
+        SMALL_NETWORK_BUCKETS = [0.005, 0.01, 0.1, 1, 10].freeze
+
+        def initialize(options = {})
+          @multiprocess_mode = options[:multiprocess_mode] || :all
+          @buckets = options[:buckets] || SMALL_NETWORK_BUCKETS
+          @base_labels = options[:base_labels] || {}
+          @docstring = options[:docstring]
+          @with_feature = options[:with_feature]
+        end
+
+        # Documentation describing metric in metrics endpoint '/-/metrics'
+        def docstring(docstring = nil)
+          @docstring = docstring unless docstring.nil?
+
+          @docstring
+        end
+
+        # Gauge aggregation mode for multiprocess metrics
+        # - :all (default) returns each gauge for every process
+        # - :livesum all process'es gauges summed up
+        # - :max maximum value of per process gauges
+        # - :min minimum value of per process gauges
+        def multiprocess_mode(mode = nil)
+          @multiprocess_mode = mode unless mode.nil?
+
+          @multiprocess_mode
+        end
+
+        # Measurement buckets for histograms
+        def buckets(buckets = nil)
+          @buckets = buckets unless buckets.nil?
+
+          @buckets
+        end
+
+        # Base labels are merged with per metric labels
+        def base_labels(base_labels = nil)
+          @base_labels = base_labels unless base_labels.nil?
+
+          @base_labels
+        end
+
+        # Use feature toggle to control whether certain metric is enabled/disabled
+        def with_feature(name = nil)
+          @with_feature = name unless name.nil?
+
+          @with_feature
+        end
+
+        def evaluate(&block)
+          instance_eval(&block) if block_given?
+          self
+        end
+      end
+    end
+  end
+end

lib/gitlab/metrics/null_metric.rb

@@ -2,6 +2,8 @@ module Gitlab
   module Metrics
     # Mocks ::Prometheus::Client::Metric and all derived metrics
     class NullMetric
+      include Singleton
+
       def method_missing(name, *args, &block)
         nil
       end

lib/gitlab/metrics/prometheus.rb

@@ -3,73 +3,77 @@ require 'prometheus/client'
 module Gitlab
   module Metrics
     module Prometheus
-      include Gitlab::CurrentSettings
-      include Gitlab::Utils::StrongMemoize
+      extend ActiveSupport::Concern
 
       REGISTRY_MUTEX = Mutex.new
       PROVIDER_MUTEX = Mutex.new
 
-      def metrics_folder_present?
-        multiprocess_files_dir = ::Prometheus::Client.configuration.multiprocess_files_dir
+      class_methods do
+        include Gitlab::Utils::StrongMemoize
 
-        multiprocess_files_dir &&
-          ::Dir.exist?(multiprocess_files_dir) &&
-          ::File.writable?(multiprocess_files_dir)
-      end
+        def metrics_folder_present?
+          multiprocess_files_dir = ::Prometheus::Client.configuration.multiprocess_files_dir
 
-      def prometheus_metrics_enabled?
-        strong_memoize(:prometheus_metrics_enabled) do
-          prometheus_metrics_enabled_unmemoized
+          multiprocess_files_dir &&
+            ::Dir.exist?(multiprocess_files_dir) &&
+            ::File.writable?(multiprocess_files_dir)
+        end
+
+        def prometheus_metrics_enabled?
+          strong_memoize(:prometheus_metrics_enabled) do
+            prometheus_metrics_enabled_unmemoized
+          end
         end
-      end
 
-      def registry
-        strong_memoize(:registry) do
-          REGISTRY_MUTEX.synchronize do
-            strong_memoize(:registry) do
-              ::Prometheus::Client.registry
+        def registry
+          strong_memoize(:registry) do
+            REGISTRY_MUTEX.synchronize do
+              strong_memoize(:registry) do
+                ::Prometheus::Client.registry
+              end
             end
           end
         end
-      end
 
-      def counter(name, docstring, base_labels = {})
-        safe_provide_metric(:counter, name, docstring, base_labels)
-      end
+        def counter(name, docstring, base_labels = {})
+          safe_provide_metric(:counter, name, docstring, base_labels)
+        end
 
-      def summary(name, docstring, base_labels = {})
-        safe_provide_metric(:summary, name, docstring, base_labels)
-      end
+        def summary(name, docstring, base_labels = {})
+          safe_provide_metric(:summary, name, docstring, base_labels)
+        end
 
-      def gauge(name, docstring, base_labels = {}, multiprocess_mode = :all)
-        safe_provide_metric(:gauge, name, docstring, base_labels, multiprocess_mode)
-      end
+        def gauge(name, docstring, base_labels = {}, multiprocess_mode = :all)
+          safe_provide_metric(:gauge, name, docstring, base_labels, multiprocess_mode)
+        end
 
-      def histogram(name, docstring, base_labels = {}, buckets = ::Prometheus::Client::Histogram::DEFAULT_BUCKETS)
-        safe_provide_metric(:histogram, name, docstring, base_labels, buckets)
-      end
+        def histogram(name, docstring, base_labels = {}, buckets = ::Prometheus::Client::Histogram::DEFAULT_BUCKETS)
+          safe_provide_metric(:histogram, name, docstring, base_labels, buckets)
+        end
 
-      private
+        private
 
-      def safe_provide_metric(method, name, *args)
-        metric = provide_metric(name)
-        return metric if metric
+        def safe_provide_metric(method, name, *args)
+          metric = provide_metric(name)
+          return metric if metric
 
-        PROVIDER_MUTEX.synchronize do
-          provide_metric(name) || registry.method(method).call(name, *args)
+          PROVIDER_MUTEX.synchronize do
+            provide_metric(name) || registry.method(method).call(name, *args)
+          end
         end
-      end
 
-      def provide_metric(name)
-        if prometheus_metrics_enabled?
-          registry.get(name)
-        else
-          NullMetric.new
+        def provide_metric(name)
+          if prometheus_metrics_enabled?
+            registry.get(name)
+          else
+            NullMetric.instance
+          end
         end
-      end
 
-      def prometheus_metrics_enabled_unmemoized
-        metrics_folder_present? && current_application_settings[:prometheus_metrics_enabled] || false
+        def prometheus_metrics_enabled_unmemoized
+          metrics_folder_present? &&
+            Gitlab::CurrentSettings.current_application_settings[:prometheus_metrics_enabled] || false
+        end
       end
     end
   end

lib/gitlab/metrics/subscribers/action_view.rb

@@ -3,6 +3,14 @@ module Gitlab
     module Subscribers
       # Class for tracking the rendering timings of views.
       class ActionView < ActiveSupport::Subscriber
+        include Gitlab::Metrics::Methods
+        define_histogram :gitlab_view_rendering_duration_seconds do
+          docstring 'View rendering time'
+          base_labels Transaction::BASE_LABELS.merge({ path: nil })
+          buckets [0.001, 0.01, 0.1, 1, 10.0]
+          with_feature :prometheus_metrics_view_instrumentation
+        end
+
         attach_to :action_view
 
         SERIES = 'views'.freeze
@@ -15,23 +23,11 @@ module Gitlab
 
         private
 
-        def metric_view_rendering_duration_seconds
-          @metric_view_rendering_duration_seconds ||= Gitlab::Metrics.histogram(
-            :gitlab_view_rendering_duration_seconds,
-            'View rendering time',
-            Transaction::BASE_LABELS.merge({ path: nil }),
-            [0.001, 0.002, 0.005, 0.01, 0.02, 0.05, 0.1, 0.500, 2.0, 10.0]
-          )
-        end
-
         def track(event)
           values = values_for(event)
           tags   = tags_for(event)
 
-          metric_view_rendering_duration_seconds.observe(
-            current_transaction.labels.merge(tags),
-            event.duration
-          )
+          self.class.gitlab_view_rendering_duration_seconds.observe(current_transaction.labels.merge(tags), event.duration)
 
           current_transaction.increment(:view_duration, event.duration)
           current_transaction.add_metric(SERIES, values, tags)

lib/gitlab/metrics/subscribers/active_record.rb

@@ -3,12 +3,13 @@ module Gitlab
     module Subscribers
       # Class for tracking the total query duration of a transaction.
       class ActiveRecord < ActiveSupport::Subscriber
+        include Gitlab::Metrics::Methods
         attach_to :active_record
 
         def sql(event)
           return unless current_transaction
 
-          metric_sql_duration_seconds.observe(current_transaction.labels, event.duration / 1000.0)
+          self.class.gitlab_sql_duration_seconds.observe(current_transaction.labels, event.duration / 1000.0)
 
           current_transaction.increment(:sql_duration, event.duration, false)
           current_transaction.increment(:sql_count, 1, false)
@@ -16,17 +17,14 @@ module Gitlab
 
         private
 
-        def current_transaction
-          Transaction.current
+        define_histogram :gitlab_sql_duration_seconds do
+          docstring 'SQL time'
+          base_labels Transaction::BASE_LABELS
+          buckets [0.001, 0.01, 0.1, 1.0, 10.0]
         end
 
-        def metric_sql_duration_seconds
-          @metric_sql_duration_seconds ||= Gitlab::Metrics.histogram(
-            :gitlab_sql_duration_seconds,
-            'SQL time',
-            Transaction::BASE_LABELS,
-            [0.001, 0.002, 0.005, 0.01, 0.02, 0.05, 0.1, 0.500, 2.0, 10.0]
-          )
+        def current_transaction
+          Transaction.current
         end
       end
     end

lib/gitlab/metrics/transaction.rb

@@ -2,11 +2,12 @@ module Gitlab
   module Metrics
     # Class for storing metrics information of a single transaction.
     class Transaction
+      include Gitlab::Metrics::Methods
+
       # base labels shared among all transactions
       BASE_LABELS = { controller: nil, action: nil }.freeze
 
       THREAD_KEY = :_gitlab_metrics_transaction
-      METRICS_MUTEX = Mutex.new
 
       # The series to store events (e.g. Git pushes) in.
       EVENT_SERIES = 'events'.freeze
@@ -54,8 +55,8 @@ module Gitlab
         @memory_after = System.memory_usage
         @finished_at = System.monotonic_time
 
-        self.class.metric_transaction_duration_seconds.observe(labels, duration)
-        self.class.metric_transaction_allocated_memory_bytes.observe(labels, allocated_memory * 1024.0)
+        self.class.gitlab_transaction_duration_seconds.observe(labels, duration)
+        self.class.gitlab_transaction_allocated_memory_bytes.observe(labels, allocated_memory * 1024.0)
 
         Thread.current[THREAD_KEY] = nil
       end
@@ -72,7 +73,7 @@ module Gitlab
       # event_name - The name of the event (e.g. "git_push").
       # tags - A set of tags to attach to the event.
       def add_event(event_name, tags = {})
-        self.class.metric_event_counter(event_name, tags).increment(tags.merge(labels))
+        self.class.transaction_metric(event_name, :counter, prefix: 'event_', tags: tags).increment(tags.merge(labels))
         @metrics << Metric.new(EVENT_SERIES, { count: 1 }, tags.merge(event: event_name), :event)
       end
 
@@ -95,12 +96,12 @@ module Gitlab
       end
 
       def increment(name, value, use_prometheus = true)
-        self.class.metric_transaction_counter(name).increment(labels, value) if use_prometheus
+        self.class.transaction_metric(name, :counter).increment(labels, value) if use_prometheus
         @values[name] += value
       end
 
       def set(name, value, use_prometheus = true)
-        self.class.metric_transaction_gauge(name).set(labels, value) if use_prometheus
+        self.class.transaction_metric(name, :gauge).set(labels, value) if use_prometheus
         @values[name] = value
       end
 
@@ -145,64 +146,28 @@ module Gitlab
         "#{labels[:controller]}##{labels[:action]}" if labels && !labels.empty?
       end
 
-      def self.metric_transaction_duration_seconds
-        return @metric_transaction_duration_seconds if @metric_transaction_duration_seconds
-
-        METRICS_MUTEX.synchronize do
-          @metric_transaction_duration_seconds ||= Gitlab::Metrics.histogram(
-            :gitlab_transaction_duration_seconds,
-            'Transaction duration',
-            BASE_LABELS,
-            [0.001, 0.002, 0.005, 0.01, 0.02, 0.05, 0.1, 0.500, 2.0, 10.0]
-          )
-        end
-      end
-
-      def self.metric_transaction_allocated_memory_bytes
-        return @metric_transaction_allocated_memory_bytes if @metric_transaction_allocated_memory_bytes
-
-        METRICS_MUTEX.synchronize do
-          @metric_transaction_allocated_memory_bytes ||= Gitlab::Metrics.histogram(
-            :gitlab_transaction_allocated_memory_bytes,
-            'Transaction allocated memory bytes',
-            BASE_LABELS,
-            [1000, 10000, 20000, 500000, 1000000, 2000000, 5000000, 10000000, 20000000, 100000000]
-          )
-        end
+      define_histogram :gitlab_transaction_duration_seconds do
+        docstring 'Transaction duration'
+        base_labels BASE_LABELS
+        buckets [0.001, 0.01, 0.1, 1.0, 10.0]
       end
 
-      def self.metric_event_counter(event_name, tags)
-        return @metric_event_counters[event_name] if @metric_event_counters&.has_key?(event_name)
-
-        METRICS_MUTEX.synchronize do
-          @metric_event_counters ||= {}
-          @metric_event_counters[event_name] ||= Gitlab::Metrics.counter(
-            "gitlab_transaction_event_#{event_name}_total".to_sym,
-            "Transaction event #{event_name} counter",
-            tags.merge(BASE_LABELS)
-          )
-        end
-      end
-
-      def self.metric_transaction_counter(name)
-        return @metric_transaction_counters[name] if @metric_transaction_counters&.has_key?(name)
-
-        METRICS_MUTEX.synchronize do
-          @metric_transaction_counters ||= {}
-          @metric_transaction_counters[name] ||= Gitlab::Metrics.counter(
-            "gitlab_transaction_#{name}_total".to_sym, "Transaction #{name} counter", BASE_LABELS
-          )
-        end
+      define_histogram :gitlab_transaction_allocated_memory_bytes do
+        docstring 'Transaction allocated memory bytes'
+        base_labels BASE_LABELS
+        buckets [100, 1000, 10000, 100000, 1000000, 10000000]
+        with_feature :prometheus_metrics_transaction_allocated_memory
       end
 
-      def self.metric_transaction_gauge(name)
-        return @metric_transaction_gauges[name] if @metric_transaction_gauges&.has_key?(name)
+      def self.transaction_metric(name, type, prefix: nil, tags: {})
+        metric_name = "gitlab_transaction_#{prefix}#{name}_total".to_sym
+        fetch_metric(type, metric_name) do
+          docstring "Transaction #{prefix}#{name} #{type}"
+          base_labels tags.merge(BASE_LABELS)
 
-        METRICS_MUTEX.synchronize do
-          @metric_transaction_gauges ||= {}
-          @metric_transaction_gauges[name] ||= Gitlab::Metrics.gauge(
-            "gitlab_transaction_#{name}".to_sym, "Transaction gauge #{name}", BASE_LABELS, :livesum
-          )
+          if type == :gauge
+            multiprocess_mode :livesum
+          end
         end
       end
     end

lib/gitlab/o_auth/user.rb

@@ -57,7 +57,7 @@ module Gitlab
         user ||= find_or_build_ldap_user if auto_link_ldap_user?
         user ||= build_new_user if signup_enabled?
 
-        user.external = true if external_provider? && user
+        user.external = true if external_provider? && user&.new_record?
 
         user
       end

spec/lib/gitlab/metrics/method_call_spec.rb

@@ -5,6 +5,10 @@ describe Gitlab::Metrics::MethodCall do
   let(:method_call) { described_class.new('Foo#bar', :Foo, '#bar', transaction) }
 
   describe '#measure' do
+    after do
+      described_class.reload_metric!(:gitlab_method_call_duration_seconds)
+    end
+
     it 'measures the performance of the supplied block' do
       method_call.measure { 'foo' }
 
@@ -20,8 +24,6 @@ describe Gitlab::Metrics::MethodCall do
 
       context 'prometheus instrumentation is enabled' do
         before do
-          allow(Feature.get(:prometheus_metrics_method_instrumentation)).to receive(:enabled?).and_call_original
-          described_class.measurement_enabled_cache_expires_at.value = Time.now.to_i - 1
           Feature.get(:prometheus_metrics_method_instrumentation).enable
         end
 
@@ -31,30 +33,12 @@ describe Gitlab::Metrics::MethodCall do
           end
         end
 
-        it 'caches subsequent invocations of feature check' do
-          10.times do
-            method_call.measure { 'foo' }
-          end
-
-          expect(Feature.get(:prometheus_metrics_method_instrumentation)).to have_received(:enabled?).once
-        end
-
-        it 'expires feature check cache after 1 minute' do
-          method_call.measure { 'foo' }
-
-          Timecop.travel(1.minute.from_now) do
-            method_call.measure { 'foo' }
-          end
-
-          Timecop.travel(1.minute.from_now + 1.second) do
-            method_call.measure { 'foo' }
-          end
-
-          expect(Feature.get(:prometheus_metrics_method_instrumentation)).to have_received(:enabled?).twice
+        it 'metric is not a NullMetric' do
+          expect(described_class).not_to be_instance_of(Gitlab::Metrics::NullMetric)
         end
 
         it 'observes the performance of the supplied block' do
-          expect(described_class.call_duration_histogram)
+          expect(described_class.gitlab_method_call_duration_seconds)
             .to receive(:observe)
                   .with({ module: :Foo, method: '#bar' }, be_a_kind_of(Numeric))
 
@@ -64,14 +48,12 @@ describe Gitlab::Metrics::MethodCall do
 
       context 'prometheus instrumentation is disabled' do
         before do
-          described_class.measurement_enabled_cache_expires_at.value = Time.now.to_i - 1
-
           Feature.get(:prometheus_metrics_method_instrumentation).disable
         end
 
-        it 'does not observe the performance' do
-          expect(described_class.call_duration_histogram)
-            .not_to receive(:observe)
+        it 'observes using NullMetric' do
+          expect(described_class.gitlab_method_call_duration_seconds).to be_instance_of(Gitlab::Metrics::NullMetric)
+          expect(described_class.gitlab_method_call_duration_seconds).to receive(:observe)
 
           method_call.measure { 'foo' }
         end
@@ -81,12 +63,10 @@ describe Gitlab::Metrics::MethodCall do
     context 'when measurement is below threshold' do
       before do
         allow(method_call).to receive(:above_threshold?).and_return(false)
-
-        Feature.get(:prometheus_metrics_method_instrumentation).enable
       end
 
       it 'does not observe the performance' do
-        expect(described_class.call_duration_histogram)
+        expect(described_class.gitlab_method_call_duration_seconds)
           .not_to receive(:observe)
 
         method_call.measure { 'foo' }
@@ -96,7 +76,7 @@ describe Gitlab::Metrics::MethodCall do
 
   describe '#to_metric' do
     it 'returns a Metric instance' do
-      expect(method_call).to receive(:real_time).and_return(4.0001)
+      expect(method_call).to receive(:real_time).and_return(4.0001).twice
       expect(method_call).to receive(:cpu_time).and_return(3.0001)
 
       method_call.measure { 'foo' }

spec/lib/gitlab/metrics/methods_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Metrics::Methods do
+  subject { Class.new { include Gitlab::Metrics::Methods } }
+
+  shared_context 'metric' do |metric_type, *args|
+    let(:docstring) { 'description' }
+    let(:metric_name) { :sample_metric }
+
+    describe "#define_#{metric_type}" do
+      define_method(:call_define_metric_method) do |**args|
+        subject.__send__("define_#{metric_type}", metric_name, **args)
+      end
+
+      context 'metrics access method not defined' do
+        it "defines metrics accessing method" do
+          expect(subject).not_to respond_to(metric_name)
+
+          call_define_metric_method(docstring: docstring)
+
+          expect(subject).to respond_to(metric_name)
+        end
+      end
+
+      context 'metrics access method defined' do
+        before do
+          call_define_metric_method(docstring: docstring)
+        end
+
+        it 'raises error when trying to redefine method' do
+          expect { call_define_metric_method(docstring: docstring) }.to raise_error(ArgumentError)
+        end
+
+        context 'metric is not cached' do
+          it 'calls fetch_metric' do
+            expect(subject).to receive(:init_metric).with(metric_type, metric_name, docstring: docstring)
+
+            subject.public_send(metric_name)
+          end
+        end
+
+        context 'metric is cached' do
+          before do
+            subject.public_send(metric_name)
+          end
+
+          it 'returns cached metric' do
+            expect(subject).not_to receive(:init_metric)
+
+            subject.public_send(metric_name)
+          end
+        end
+      end
+    end
+
+    describe "#fetch_#{metric_type}" do
+      let(:null_metric) { Gitlab::Metrics::NullMetric.instance }
+
+      define_method(:call_fetch_metric_method) do |**args|
+        subject.__send__("fetch_#{metric_type}", metric_name, **args)
+      end
+
+      context "when #{metric_type} is not cached" do
+        it 'initializes counter metric' do
+          allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+          call_fetch_metric_method(docstring: docstring)
+
+          expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+        end
+      end
+
+      context "when #{metric_type} is cached" do
+        before do
+          call_fetch_metric_method(docstring: docstring)
+        end
+
+        it 'uses class metric cache' do
+          expect(Gitlab::Metrics).not_to receive(metric_type)
+
+          call_fetch_metric_method(docstring: docstring)
+        end
+
+        context 'when metric is reloaded' do
+          before do
+            subject.reload_metric!(metric_name)
+          end
+
+          it "initializes #{metric_type} metric" do
+            allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+            call_fetch_metric_method(docstring: docstring)
+
+            expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+          end
+        end
+      end
+
+      context 'when metric is configured with feature' do
+        let(:feature_name) { :some_metric_feature }
+        let(:metric) { call_fetch_metric_method(docstring: docstring, with_feature: feature_name) }
+
+        context 'when feature is enabled' do
+          before do
+            Feature.get(feature_name).enable
+          end
+
+          it "initializes #{metric_type} metric" do
+            allow(Gitlab::Metrics).to receive(metric_type).and_return(null_metric)
+
+            metric
+
+            expect(Gitlab::Metrics).to have_received(metric_type).with(metric_name, docstring, *args)
+          end
+        end
+
+        context 'when feature is disabled' do
+          before do
+            Feature.get(feature_name).disable
+          end
+
+          it "returns NullMetric" do
+            allow(Gitlab::Metrics).to receive(metric_type)
+
+            expect(metric).to be_instance_of(Gitlab::Metrics::NullMetric)
+
+            expect(Gitlab::Metrics).not_to have_received(metric_type)
+          end
+        end
+      end
+    end
+  end
+
+  include_examples 'metric', :counter, {}
+  include_examples 'metric', :gauge, {}, :all
+  include_examples 'metric', :histogram, {}, [0.005, 0.01, 0.1, 1, 10]
+end

spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb

@@ -2,6 +2,11 @@ require 'spec_helper'
 
 describe Gitlab::Metrics::Samplers::RubySampler do
   let(:sampler) { described_class.new(5) }
+  let(:null_metric) { double('null_metric', set: nil, observe: nil) }
+
+  before do
+    allow(Gitlab::Metrics::NullMetric).to receive(:instance).and_return(null_metric)
+  end
 
   after do
     Allocations.stop if Gitlab::Metrics.mri?
@@ -17,12 +22,9 @@ describe Gitlab::Metrics::Samplers::RubySampler do
     end
 
     it 'adds a metric containing the memory usage' do
-      expect(Gitlab::Metrics::System).to receive(:memory_usage)
-                                           .and_return(9000)
+      expect(Gitlab::Metrics::System).to receive(:memory_usage).and_return(9000)
 
-      expect(sampler.metrics[:memory_usage]).to receive(:set)
-                                                  .with({}, 9000)
-                                                  .and_call_original
+      expect(sampler.metrics[:memory_usage]).to receive(:set).with({}, 9000)
 
       sampler.sample
     end
@@ -31,9 +33,7 @@ describe Gitlab::Metrics::Samplers::RubySampler do
       expect(Gitlab::Metrics::System).to receive(:file_descriptor_count)
                                            .and_return(4)
 
-      expect(sampler.metrics[:file_descriptors]).to receive(:set)
-                                                      .with({}, 4)
-                                                      .and_call_original
+      expect(sampler.metrics[:file_descriptors]).to receive(:set).with({}, 4)
 
       sampler.sample
     end
@@ -49,16 +49,14 @@ describe Gitlab::Metrics::Samplers::RubySampler do
     it 'adds a metric containing garbage collection time statistics' do
       expect(GC::Profiler).to receive(:total_time).and_return(0.24)
 
-      expect(sampler.metrics[:total_time]).to receive(:set)
-                                                .with({}, 240)
-                                                .and_call_original
+      expect(sampler.metrics[:total_time]).to receive(:set).with({}, 240)
 
       sampler.sample
     end
 
     it 'adds a metric containing garbage collection statistics' do
       GC.stat.keys.each do |key|
-        expect(sampler.metrics[key]).to receive(:set).with({}, anything).and_call_original
+        expect(sampler.metrics[key]).to receive(:set).with({}, anything)
       end
 
       sampler.sample

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

@@ -32,7 +32,7 @@ describe Gitlab::Metrics::Subscribers::ActionView do
     end
 
     it 'observes view rendering time' do
-      expect(subscriber.send(:metric_view_rendering_duration_seconds))
+      expect(described_class.gitlab_view_rendering_duration_seconds)
         .to receive(:observe)
               .with({ view: 'app/views/x.html.haml' }, 2.1)
 

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

@@ -25,7 +25,7 @@ describe Gitlab::Metrics::Subscribers::ActiveRecord do
         expect(subscriber).to receive(:current_transaction)
                                 .at_least(:once)
                                 .and_return(transaction)
-        expect(subscriber.send(:metric_sql_duration_seconds)).to receive(:observe).with({}, 0.002)
+        expect(described_class.send(:gitlab_sql_duration_seconds)).to receive(:observe).with({}, 0.002)
         subscriber.sql(event)
       end
 

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

spec/lib/gitlab/metrics_spec.rb

@@ -20,7 +20,7 @@ describe Gitlab::Metrics do
 
     context 'prometheus metrics enabled in config' do
       before do
-        allow(described_class).to receive(:current_application_settings).and_return(prometheus_metrics_enabled: true)
+        allow(Gitlab::CurrentSettings).to receive(:current_application_settings).and_return(prometheus_metrics_enabled: true)
       end
 
       context 'when metrics folder is present' do

spec/lib/gitlab/o_auth/user_spec.rb

@@ -44,6 +44,18 @@ describe Gitlab::OAuth::User do
 
     let(:provider) { 'twitter' }
 
+    describe 'when account exists on server' do
+      it 'does not mark the user as external' do
+        create(:omniauth_user, extern_uid: 'my-uid', provider: provider)
+        stub_omniauth_config(allow_single_sign_on: [provider], external_providers: [provider])
+
+        oauth_user.save
+
+        expect(gl_user).to be_valid
+        expect(gl_user.external).to be_falsey
+      end
+    end
+
     describe 'signup' do
       context 'when signup is disabled' do
         before do
@@ -51,7 +63,7 @@ describe Gitlab::OAuth::User do
         end
 
         it 'creates the user' do
-          stub_omniauth_config(allow_single_sign_on: ['twitter'])
+          stub_omniauth_config(allow_single_sign_on: [provider])
 
           oauth_user.save
 
@@ -65,7 +77,7 @@ describe Gitlab::OAuth::User do
         end
 
         it 'creates and confirms the user anyway' do
-          stub_omniauth_config(allow_single_sign_on: ['twitter'])
+          stub_omniauth_config(allow_single_sign_on: [provider])
 
           oauth_user.save
 
@@ -75,7 +87,7 @@ describe Gitlab::OAuth::User do
       end
 
       it 'marks user as having password_automatically_set' do
-        stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['twitter'])
+        stub_omniauth_config(allow_single_sign_on: [provider], external_providers: [provider])
 
         oauth_user.save
 
@@ -86,7 +98,7 @@ describe Gitlab::OAuth::User do
       shared_examples 'to verify compliance with allow_single_sign_on' do
         context 'provider is marked as external' do
           it 'marks user as external' do
-            stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['twitter'])
+            stub_omniauth_config(allow_single_sign_on: [provider], external_providers: [provider])
             oauth_user.save
             expect(gl_user).to be_valid
             expect(gl_user.external).to be_truthy
@@ -95,8 +107,8 @@ describe Gitlab::OAuth::User do
 
         context 'provider was external, now has been removed' do
           it 'does not mark external user as internal' do
-            create(:omniauth_user, extern_uid: 'my-uid', provider: 'twitter', external: true)
-            stub_omniauth_config(allow_single_sign_on: ['twitter'], external_providers: ['facebook'])
+            create(:omniauth_user, extern_uid: 'my-uid', provider: provider, external: true)
+            stub_omniauth_config(allow_single_sign_on: [provider], external_providers: ['facebook'])
             oauth_user.save
             expect(gl_user).to be_valid
             expect(gl_user.external).to be_truthy
@@ -118,7 +130,7 @@ describe Gitlab::OAuth::User do
 
         context 'with new allow_single_sign_on enabled syntax' do
           before do
-            stub_omniauth_config(allow_single_sign_on: ['twitter'])
+            stub_omniauth_config(allow_single_sign_on: [provider])
           end
 
           it "creates a user from Omniauth" do
@@ -127,7 +139,7 @@ describe Gitlab::OAuth::User do
             expect(gl_user).to be_valid
             identity = gl_user.identities.first
             expect(identity.extern_uid).to eql uid
-            expect(identity.provider).to eql 'twitter'
+            expect(identity.provider).to eql provider
           end
         end
 
@@ -142,7 +154,7 @@ describe Gitlab::OAuth::User do
             expect(gl_user).to be_valid
             identity = gl_user.identities.first
             expect(identity.extern_uid).to eql uid
-            expect(identity.provider).to eql 'twitter'
+            expect(identity.provider).to eql provider
           end
         end
 

spec/models/concerns/discussion_on_diff_spec.rb

@@ -20,6 +20,16 @@ describe DiscussionOnDiff do
         expect(truncated_lines).not_to include(be_meta)
       end
     end
+
+    context "when the diff line does not exist on a legacy diff note" do
+      it "returns an empty array" do
+        legacy_note = LegacyDiffNote.new
+
+        allow(subject).to receive(:first_note).and_return(legacy_note)
+
+        expect(truncated_lines).to eq([])
+      end
+    end
   end
 
   describe '#line_code_in_diffs' do

spec/models/wiki_page_spec.rb

@@ -387,13 +387,23 @@ describe WikiPage do
   end
 
   describe '#formatted_content' do
-    it 'returns processed content of the page', :disable_gitaly do
-      subject.create({ title: "RDoc", content: "*bold*", format: "rdoc" })
-      page = wiki.find_page('RDoc')
+    shared_examples 'fetching page formatted content' do
+      it 'returns processed content of the page' do
+        subject.create({ title: "RDoc", content: "*bold*", format: "rdoc" })
+        page = wiki.find_page('RDoc')
 
-      expect(page.formatted_content).to eq("\n<p><strong>bold</strong></p>\n")
+        expect(page.formatted_content).to eq("\n<p><strong>bold</strong></p>\n")
 
-      destroy_page('RDoc')
+        destroy_page('RDoc')
+      end
+    end
+
+    context 'when Gitaly wiki_page_formatted_data is enabled' do
+      it_behaves_like 'fetching page formatted content'
+    end
+
+    context 'when Gitaly wiki_page_formatted_data is disabled', :disable_gitaly do
+      it_behaves_like 'fetching page formatted content'
     end
   end