Add latest changes from gitlab-org/gitlab@master

.rubocop.yml

@@ -402,7 +402,6 @@ RSpec/RepeatedExample:
     - 'spec/graphql/gitlab_schema_spec.rb'
     - 'spec/helpers/users_helper_spec.rb'
     - 'spec/lib/banzai/filter/autolink_filter_spec.rb'
-    - 'spec/lib/banzai/filter/issuable_state_filter_spec.rb'
     - 'spec/lib/gitlab/closing_issue_extractor_spec.rb'
     - 'spec/lib/gitlab/danger/changelog_spec.rb'
     - 'spec/lib/gitlab/git/blob_spec.rb'

app/controllers/projects/blob_controller.rb

@@ -208,24 +208,12 @@ class Projects::BlobController < Projects::ApplicationController
       .last_for_path(@repository, @ref, @path).sha
   end
 
-  def set_code_navigation_build
-    return if Feature.disabled?(:code_navigation, @project)
-
-    artifact =
-      Ci::JobArtifact
-        .for_sha(@blob.commit_id, @project.id)
-        .for_job_name(Ci::Build::CODE_NAVIGATION_JOB_NAME)
-        .last
-
-    @code_navigation_build = artifact&.job
-  end
-
   def show_html
     environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit }
     environment_params[:find_latest] = true
     @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
     @last_commit = @repository.last_commit_for_path(@commit.id, @blob.path)
-    set_code_navigation_build
+    @code_navigation_path = Gitlab::CodeNavigationPath.new(@project, @blob.commit_id).full_json_path_for(@blob.path)
 
     render 'show'
   end

app/models/ci/build.rb

@@ -33,8 +33,6 @@ module Ci
       scheduler_failure: 2
     }.freeze
 
-    CODE_NAVIGATION_JOB_NAME = 'code_navigation'
-
     has_one :deployment, as: :deployable, class_name: 'Deployment'
     has_one :resource, class_name: 'Ci::Resource', inverse_of: :build
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'

app/serializers/diff_file_entity.rb

@@ -64,6 +64,10 @@ class DiffFileEntity < DiffFileBaseEntity
   # Used for parallel diffs
   expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, options) { parallel_diff_view?(options, diff_file) && diff_file.text? }
 
+  expose :code_navigation_path, if: -> (diff_file) { options[:code_navigation_path] } do |diff_file|
+    options[:code_navigation_path].full_json_path_for(diff_file.new_path)
+  end
+
   private
 
   def parallel_diff_view?(options, diff_file)

app/serializers/diffs_entity.rb

@@ -70,13 +70,21 @@ class DiffsEntity < Grape::Entity
 
   expose :diff_files do |diffs, options|
     submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
-    DiffFileEntity.represent(diffs.diff_files, options.merge(submodule_links: submodule_links))
+    code_navigation_path =
+      Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs.head_sha)
+
+    DiffFileEntity.represent(diffs.diff_files,
+      options.merge(submodule_links: submodule_links, code_navigation_path: code_navigation_path))
   end
 
   expose :merge_request_diffs, using: MergeRequestDiffEntity, if: -> (_, options) { options[:merge_request_diffs]&.any? } do |diffs|
     options[:merge_request_diffs]
   end
 
+  expose :definition_path_prefix, if: -> (diff_file) { Feature.enabled?(:code_navigation, merge_request.project) } do |diffs|
+    project_blob_path(merge_request.project, diffs.diff_refs.head_sha)
+  end
+
   def merge_request
     options[:merge_request]
   end

app/serializers/paginated_diff_entity.rb

@@ -10,7 +10,11 @@ class PaginatedDiffEntity < Grape::Entity
 
   expose :diff_files do |diffs, options|
     submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
-    DiffFileEntity.represent(diffs.diff_files, options.merge(submodule_links: submodule_links))
+    code_navigation_path =
+      Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs.head_sha)
+
+    DiffFileEntity.represent(diffs.diff_files,
+      options.merge(submodule_links: submodule_links, code_navigation_path: code_navigation_path))
   end
 
   expose :pagination do

app/services/metrics/dashboard/base_service.rb

@@ -45,9 +45,30 @@ module Metrics
 
       # Returns a new dashboard Hash, supplemented with DB info
       def process_dashboard
-        ::Gitlab::Metrics::Dashboard::Processor
-          .new(project, raw_dashboard, sequence, process_params)
-          .process
+        # Get the dashboard from cache/disk before beginning the benchmark.
+        dashboard = raw_dashboard
+        processed_dashboard = nil
+
+        benchmark_processing do
+          processed_dashboard = ::Gitlab::Metrics::Dashboard::Processor
+            .new(project, dashboard, sequence, process_params)
+            .process
+        end
+
+        processed_dashboard
+      end
+
+      def benchmark_processing
+        output = nil
+
+        processing_time_seconds = Benchmark.realtime { output = yield }
+
+        if output
+          processing_time_metric.observe(
+            processing_time_metric_labels,
+            processing_time_seconds * 1_000
+          )
+        end
       end
 
       def process_params
@@ -72,6 +93,26 @@ module Metrics
       def sequence
         SEQUENCE
       end
+
+      def processing_time_metric
+        @processing_time_metric ||= ::Gitlab::Metrics.summary(
+          :gitlab_metrics_dashboard_processing_time_ms,
+          'Metrics dashboard processing time in milliseconds'
+        )
+      end
+
+      def processing_time_metric_labels
+        {
+          stages: sequence_string,
+          service: self.class.name
+        }
+      end
+
+      # If @sequence is [STAGES::CommonMetricsInserter, STAGES::CustomMetricsInserter],
+      # this function will output `CommonMetricsInserter-CustomMetricsInserter`.
+      def sequence_string
+        sequence.map { |stage_class| stage_class.to_s.split('::').last }.join('-')
+      end
     end
   end
 end

app/views/projects/blob/_blob.html.haml

@@ -9,9 +9,8 @@
   = render "projects/blob/auxiliary_viewer", blob: blob
 
 #blob-content-holder.blob-content-holder
-  - if @code_navigation_build
-    - code_nav_url = raw_project_job_artifacts_url(@project, @code_navigation_build, path: "lsif/#{blob.path}")
-    #js-code-navigation{ data: { code_nav_url: "#{code_nav_url}.json", definition_path_prefix: project_blob_path(@project, @ref) } }
+  - if @code_navigation_path
+    #js-code-navigation{ data: { code_nav_url: @code_navigation_path, definition_path_prefix: project_blob_path(@project, @ref) } }
   %article.file-holder
     = render 'projects/blob/header', blob: blob
     = render 'projects/blob/content', blob: blob

changelogs/unreleased/208890-optimize-notes-and-suggestion-counters.yml

+---
+title: Optimize suggestions counters
+merge_request: 26443
+author:
+type: performance

config/feature_categories.yml

@@ -71,7 +71,6 @@
 - kubernetes_management
 - language_specific
 - license_compliance
-- live_coding
 - load_testing
 - logging
 - malware_scanning

db/migrate/20200330132913_add_index_on_author_id_and_created_at_and_id_to_notes.rb

+# frozen_string_literal: true
+
+class AddIndexOnAuthorIdAndCreatedAtAndIdToNotes < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :notes, [:author_id, :created_at, :id]
+    remove_concurrent_index :notes, [:author_id, :created_at]
+  end
+
+  def down
+    add_concurrent_index :notes, [:author_id, :created_at]
+    remove_concurrent_index :notes, [:author_id, :created_at, :id]
+  end
+end

db/structure.sql

@@ -9519,7 +9519,7 @@ CREATE INDEX index_namespaces_on_type_partial ON public.namespaces USING btree (
 
 CREATE UNIQUE INDEX index_note_diff_files_on_diff_note_id ON public.note_diff_files USING btree (diff_note_id);
 
-CREATE INDEX index_notes_on_author_id_and_created_at ON public.notes USING btree (author_id, created_at);
+CREATE INDEX index_notes_on_author_id_and_created_at_and_id ON public.notes USING btree (author_id, created_at, id);
 
 CREATE INDEX index_notes_on_commit_id ON public.notes USING btree (commit_id);
 
@@ -12925,5 +12925,6 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200326144443
 20200326145443
 20200330074719
+20200330132913
 \.
 

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -92,6 +92,7 @@ The following metrics are available:
 | `failed_login_captcha_total`                                   | Gauge     |                   11.0 | Counter of failed CAPTCHA attempts during login                                                     |                                                     |
 | `successful_login_captcha_total`                               | Gauge     |                   11.0 | Counter of successful CAPTCHA attempts during login                                                 |                                                     |
 | `auto_devops_pipelines_completed_total`                        | Counter   |                   12.7 | Counter of completed Auto DevOps pipelines, labeled by status                                       |                                                     |
+| `gitlab_metrics_dashboard_processing_time_ms` | Summary | 12.10 | Metrics dashboard processing time in milliseconds | service, stages |
 
 ## Metrics controlled by a feature flag
 

lib/gitlab/code_navigation_path.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class CodeNavigationPath
+    include Gitlab::Utils::StrongMemoize
+    include Gitlab::Routing
+
+    CODE_NAVIGATION_JOB_NAME = 'code_navigation'
+
+    def initialize(project, commit_sha)
+      @project = project
+      @commit_sha = commit_sha
+    end
+
+    def full_json_path_for(path)
+      return if Feature.disabled?(:code_navigation, project)
+      return unless build
+
+      raw_project_job_artifacts_path(project, build, path: "lsif/#{path}.json")
+    end
+
+    private
+
+    attr_reader :project, :commit_sha
+
+    def build
+      strong_memoize(:build) do
+        artifact = ::Ci::JobArtifact
+          .for_sha(commit_sha, project.id)
+          .for_job_name(CODE_NAVIGATION_JOB_NAME)
+          .last
+
+        artifact&.job
+      end
+    end
+  end
+end

spec/controllers/projects/blob_controller_spec.rb

@@ -118,32 +118,6 @@ describe Projects::BlobController do
         end
       end
     end
-
-    context 'when there is an artifact with code navigation data' do
-      let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id) }
-      let!(:job) { create(:ci_build, pipeline: pipeline, name: Ci::Build::CODE_NAVIGATION_JOB_NAME) }
-      let!(:artifact) { create(:ci_job_artifact, :lsif, job: job) }
-
-      let(:id) { 'master/README.md' }
-
-      it 'assigns code_navigation_build variable' do
-        request
-
-        expect(assigns[:code_navigation_build]).to eq(job)
-      end
-
-      context 'when code_navigation feature is disabled' do
-        before do
-          stub_feature_flags(code_navigation: false)
-        end
-
-        it 'does not assign code_navigation_build variable' do
-          request
-
-          expect(assigns[:code_navigation_build]).to be_nil
-        end
-      end
-    end
   end
 
   describe 'GET diff' do

spec/lib/banzai/filter/issuable_state_filter_spec.rb

@@ -156,20 +156,6 @@ describe Banzai::Filter::IssuableStateFilter do
       expect(doc.css('a').last.text).to eq(merge_request.to_reference)
     end
 
-    it 'ignores reopened merge request references' do
-      merge_request = create_merge_request(:opened)
-
-      link = create_link(
-        merge_request.to_reference,
-        merge_request: merge_request.id,
-        reference_type: 'merge_request'
-      )
-
-      doc = filter(link, context)
-
-      expect(doc.css('a').last.text).to eq(merge_request.to_reference)
-    end
-
     it 'ignores locked merge request references' do
       merge_request = create_merge_request(:locked)
 

spec/lib/gitlab/code_navigation_path_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::CodeNavigationPath do
+  context 'when there is an artifact with code navigation data' do
+    let(:project) { create(:project, :repository) }
+    let(:sha) { project.commit.id }
+    let(:build_name) { Gitlab::CodeNavigationPath::CODE_NAVIGATION_JOB_NAME }
+    let(:path) { 'lib/app.rb' }
+    let!(:pipeline) { create(:ci_pipeline, project: project, sha: sha) }
+    let!(:job) { create(:ci_build, pipeline: pipeline, name: build_name) }
+    let!(:artifact) { create(:ci_job_artifact, :lsif, job: job) }
+
+    subject { described_class.new(project, sha).full_json_path_for(path) }
+
+    it 'assigns code_navigation_build variable' do
+      expect(subject).to eq("/#{project.full_path}/-/jobs/#{job.id}/artifacts/raw/lsif/#{path}.json")
+    end
+
+    context 'when code_navigation feature is disabled' do
+      before do
+        stub_feature_flags(code_navigation: false)
+      end
+
+      it 'does not assign code_navigation_build variable' do
+        expect(subject).to be_nil
+      end
+    end
+  end
+end

spec/serializers/diff_file_entity_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe DiffFileEntity do
   include RepoHelpers
 
-  let(:project) { create(:project, :repository) }
+  let_it_be(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:commit) { project.commit(sample_commit.id) }
   let(:diff_refs) { commit.diff_refs }
@@ -21,10 +21,11 @@ describe DiffFileEntity do
   end
 
   context 'when there is a merge request' do
+    let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
     let(:user) { create(:user) }
+    let(:code_navigation_path) { Gitlab::CodeNavigationPath.new(project, project.commit.sha) }
     let(:request) { EntityRequest.new(project: project, current_user: user) }
-    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-    let(:entity) { described_class.new(diff_file, options.merge(request: request, merge_request: merge_request)) }
+    let(:entity) { described_class.new(diff_file, options.merge(request: request, merge_request: merge_request, code_navigation_path: code_navigation_path)) }
     let(:exposed_urls) { %i(edit_path view_path context_lines_path) }
 
     it_behaves_like 'diff file entity'
@@ -32,6 +33,7 @@ describe DiffFileEntity do
     it 'exposes additional attributes' do
       expect(subject).to include(*exposed_urls)
       expect(subject).to include(:replaced_view_path)
+      expect(subject).to include(:code_navigation_path)
     end
 
     it 'points all urls to merge request target project' do

spec/serializers/diffs_entity_spec.rb

@@ -23,7 +23,7 @@ describe DiffsEntity do
         :start_version, :latest_diff, :latest_version_path,
         :added_lines, :removed_lines, :render_overflow_warning,
         :email_patch_path, :plain_diff_path, :diff_files,
-        :merge_request_diffs
+        :merge_request_diffs, :definition_path_prefix
       )
     end
   end

spec/serializers/diffs_metadata_entity_spec.rb

@@ -28,8 +28,8 @@ describe DiffsMetadataEntity do
         :start_version, :latest_diff, :latest_version_path,
         :added_lines, :removed_lines, :render_overflow_warning,
         :email_patch_path, :plain_diff_path,
-        :merge_request_diffs,
-        :context_commits,
+        :merge_request_diffs, :context_commits,
+        :definition_path_prefix,
         # Attributes
         :diff_files
       )

spec/services/metrics/dashboard/custom_dashboard_service_spec.rb

@@ -16,10 +16,20 @@ describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memory_sto
   describe '#get_dashboard' do
     let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
-    let(:service_call) { described_class.new(*service_params).get_dashboard }
+    let(:service_call) { subject.get_dashboard }
+
+    subject { described_class.new(*service_params) }
 
     context 'when the dashboard does not exist' do
       it_behaves_like 'misconfigured dashboard service response', :not_found
+
+      it 'does not update gitlab_metrics_dashboard_processing_time_ms metric', :prometheus do
+        service_call
+        metric = subject.send(:processing_time_metric)
+        labels = subject.send(:processing_time_metric_labels)
+
+        expect(metric.get(labels)).to eq(0)
+      end
     end
 
     it_behaves_like 'raises error for users with insufficient permissions'
@@ -28,6 +38,7 @@ describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memory_sto
       let(:project) { project_with_dashboard(dashboard_path) }
 
       it_behaves_like 'valid dashboard service response'
+      it_behaves_like 'updates gitlab_metrics_dashboard_processing_time_ms metric'
 
       it 'caches the unprocessed dashboard for subsequent calls' do
         expect_any_instance_of(described_class)

spec/services/metrics/dashboard/pod_dashboard_service_spec.rb

@@ -36,9 +36,12 @@ describe Metrics::Dashboard::PodDashboardService, :use_clean_rails_memory_store_
   describe '#get_dashboard' do
     let(:dashboard_path) { described_class::DASHBOARD_PATH }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
-    let(:service_call) { described_class.new(*service_params).get_dashboard }
+    let(:service_call) { subject.get_dashboard }
+
+    subject { described_class.new(*service_params) }
 
     it_behaves_like 'valid dashboard service response'
     it_behaves_like 'caches the unprocessed dashboard for subsequent calls'
+    it_behaves_like 'updates gitlab_metrics_dashboard_processing_time_ms metric'
   end
 end

spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb

@@ -16,11 +16,14 @@ describe Metrics::Dashboard::SelfMonitoringDashboardService, :use_clean_rails_me
 
   describe '#get_dashboard' do
     let(:service_params) { [project, user, { environment: environment }] }
-    let(:service_call) { described_class.new(*service_params).get_dashboard }
+    let(:service_call) { subject.get_dashboard }
+
+    subject { described_class.new(*service_params) }
 
     it_behaves_like 'valid dashboard service response'
     it_behaves_like 'raises error for users with insufficient permissions'
     it_behaves_like 'caches the unprocessed dashboard for subsequent calls'
+    it_behaves_like 'updates gitlab_metrics_dashboard_processing_time_ms metric'
   end
 
   describe '.all_dashboard_paths' do

spec/services/metrics/dashboard/system_dashboard_service_spec.rb

@@ -16,11 +16,14 @@ describe Metrics::Dashboard::SystemDashboardService, :use_clean_rails_memory_sto
   describe '#get_dashboard' do
     let(:dashboard_path) { described_class::DASHBOARD_PATH }
     let(:service_params) { [project, user, { environment: environment, dashboard_path: dashboard_path }] }
-    let(:service_call) { described_class.new(*service_params).get_dashboard }
+    let(:service_call) { subject.get_dashboard }
+
+    subject { described_class.new(*service_params) }
 
     it_behaves_like 'valid dashboard service response'
     it_behaves_like 'raises error for users with insufficient permissions'
     it_behaves_like 'caches the unprocessed dashboard for subsequent calls'
+    it_behaves_like 'updates gitlab_metrics_dashboard_processing_time_ms metric'
 
     context 'when called with a non-system dashboard' do
       let(:dashboard_path) { 'garbage/dashboard/path' }

spec/support/shared_examples/services/metrics/dashboard_shared_examples.rb

@@ -118,3 +118,13 @@ RSpec.shared_examples 'misconfigured dashboard service response with stepable' d
     expect(result[:message]).to eq(message) if message
   end
 end
+
+RSpec.shared_examples 'updates gitlab_metrics_dashboard_processing_time_ms metric' do
+  specify :prometheus do
+    service_call
+    metric = subject.send(:processing_time_metric)
+    labels = subject.send(:processing_time_metric_labels)
+
+    expect(metric.get(labels)).to be > 0
+  end
+end