Automatic merge of gitlab-org/gitlab-ce master

app/models/commit.rb

@@ -469,6 +469,10 @@ class Commit
     !!merged_merge_request(user)
   end
 
+  def cache_key
+    "commit:#{sha}"
+  end
+
   private
 
   def commit_reference(from, referable_commit_id, full: false)

app/models/prometheus_metric.rb

@@ -18,6 +18,54 @@ class PrometheusMetric < ActiveRecord::Base
     system: 2
   }
 
+  GROUP_DETAILS = {
+    # built-in groups
+    nginx_ingress_vts: {
+      group_title: _('Response metrics (NGINX Ingress VTS)'),
+      required_metrics: %w(nginx_upstream_responses_total nginx_upstream_response_msecs_avg),
+      priority: 10
+    }.freeze,
+    nginx_ingress: {
+      group_title: _('Response metrics (NGINX Ingress)'),
+      required_metrics: %w(nginx_ingress_controller_requests nginx_ingress_controller_ingress_upstream_latency_seconds_sum),
+      priority: 10
+    }.freeze,
+    ha_proxy: {
+      group_title: _('Response metrics (HA Proxy)'),
+      required_metrics: %w(haproxy_frontend_http_requests_total haproxy_frontend_http_responses_total),
+      priority: 10
+    }.freeze,
+    aws_elb: {
+      group_title: _('Response metrics (AWS ELB)'),
+      required_metrics: %w(aws_elb_request_count_sum aws_elb_latency_average aws_elb_httpcode_backend_5_xx_sum),
+      priority: 10
+    }.freeze,
+    nginx: {
+      group_title: _('Response metrics (NGINX)'),
+      required_metrics: %w(nginx_server_requests nginx_server_requestMsec),
+      priority: 10
+    }.freeze,
+    kubernetes: {
+      group_title: _('System metrics (Kubernetes)'),
+      required_metrics: %w(container_memory_usage_bytes container_cpu_usage_seconds_total),
+      priority: 5
+    }.freeze,
+
+    # custom/user groups
+    business: {
+      group_title: _('Business metrics (Custom)'),
+      priority: 0
+    }.freeze,
+    response: {
+      group_title: _('Response metrics (Custom)'),
+      priority: -5
+    }.freeze,
+    system: {
+      group_title: _('System metrics (Custom)'),
+      priority: -10
+    }.freeze
+  }.freeze
+
   validates :title, presence: true
   validates :query, presence: true
   validates :group, presence: true
@@ -29,36 +77,16 @@ class PrometheusMetric < ActiveRecord::Base
 
   scope :common, -> { where(common: true) }
 
-  GROUP_TITLES = {
-    # built-in groups
-    nginx_ingress_vts: _('Response metrics (NGINX Ingress VTS)'),
-    nginx_ingress: _('Response metrics (NGINX Ingress)'),
-    ha_proxy: _('Response metrics (HA Proxy)'),
-    aws_elb: _('Response metrics (AWS ELB)'),
-    nginx: _('Response metrics (NGINX)'),
-    kubernetes: _('System metrics (Kubernetes)'),
-
-    # custom/user groups
-    business: _('Business metrics (Custom)'),
-    response: _('Response metrics (Custom)'),
-    system: _('System metrics (Custom)')
-  }.freeze
-
-  REQUIRED_METRICS = {
-    nginx_ingress_vts: %w(nginx_upstream_responses_total nginx_upstream_response_msecs_avg),
-    nginx_ingress: %w(nginx_ingress_controller_requests nginx_ingress_controller_ingress_upstream_latency_seconds_sum),
-    ha_proxy: %w(haproxy_frontend_http_requests_total haproxy_frontend_http_responses_total),
-    aws_elb: %w(aws_elb_request_count_sum aws_elb_latency_average aws_elb_httpcode_backend_5_xx_sum),
-    nginx: %w(nginx_server_requests nginx_server_requestMsec),
-    kubernetes: %w(container_memory_usage_bytes container_cpu_usage_seconds_total)
-  }.freeze
+  def priority
+    group_details(group).fetch(:priority)
+  end
 
   def group_title
-    GROUP_TITLES[group.to_sym]
+    group_details(group).fetch(:group_title)
   end
 
   def required_metrics
-    REQUIRED_METRICS[group.to_sym].to_a.map(&:to_s)
+    group_details(group).fetch(:required_metrics, []).map(&:to_s)
   end
 
   def to_query_metric
@@ -89,6 +117,12 @@ class PrometheusMetric < ActiveRecord::Base
       }]
     end
   end
+
+  private
+
+  def group_details(group)
+    GROUP_DETAILS.fetch(group.to_sym)
+  end
 end
 
 PrometheusMetric.prepend(EE::PrometheusMetric)

changelogs/unreleased/51970-correct-ordering-of-metrics.yml

+---
+title: Correct the ordering of metrics on the performance dashboard
+merge_request: 23630
+author:
+type: fixed

changelogs/unreleased/fj-44679-skip-per-commit-validations.yml

+---
+title: Skip per-commit validations already evaluated
+merge_request: 23984
+author:
+type: performance

lib/gitlab/checks/base_checker.rb

@@ -34,6 +34,22 @@ module Gitlab
       def tag_exists?
         project.repository.tag_exists?(tag_name)
       end
+
+      def validate_once(resource)
+        Gitlab::SafeRequestStore.fetch(cache_key_for_resource(resource)) do
+          yield(resource)
+
+          true
+        end
+      end
+
+      def cache_key_for_resource(resource)
+        "git_access:#{checker_cache_key}:#{resource.cache_key}"
+      end
+
+      def checker_cache_key
+        self.class.name.demodulize.underscore
+      end
     end
   end
 end

lib/gitlab/checks/diff_check.rb

@@ -15,13 +15,17 @@ module Gitlab
         return if deletion? || newrev.nil?
         return unless should_run_diff_validations?
         return if commits.empty?
-        return unless uses_raw_delta_validations?
 
         file_paths = []
-        process_raw_deltas do |diff|
-          file_paths << (diff.new_path || diff.old_path)
 
-          validate_diff(diff)
+        process_commits do |commit|
+          validate_once(commit) do
+            commit.raw_deltas.each do |diff|
+              file_paths << (diff.new_path || diff.old_path)
+
+              validate_diff(diff)
+            end
+          end
         end
 
         validate_file_paths(file_paths)
@@ -29,17 +33,13 @@ module Gitlab
 
       private
 
-      def should_run_diff_validations?
-        validate_lfs_file_locks?
-      end
-
       def validate_lfs_file_locks?
         strong_memoize(:validate_lfs_file_locks) do
           project.lfs_enabled? && project.any_lfs_file_locks?
         end
       end
 
-      def uses_raw_delta_validations?
+      def should_run_diff_validations?
         validations_for_diff.present? || path_validations.present?
       end
 
@@ -60,16 +60,14 @@ module Gitlab
         validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
       end
 
-      def process_raw_deltas
+      def process_commits
         logger.log_timed(LOG_MESSAGES[:diff_content_check]) do
           # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
           ::Gitlab::GitalyClient.allow_n_plus_1_calls do
             commits.each do |commit|
               logger.check_timeout_reached
 
-              commit.raw_deltas.each do |diff|
-                yield(diff)
-              end
+              yield(commit)
             end
           end
         end

lib/gitlab/checks/lfs_check.rb

@@ -7,6 +7,7 @@ module Gitlab
       ERROR_MESSAGE = 'LFS objects are missing. Ensure LFS is properly set up or try a manual "git lfs push --all".'.freeze
 
       def validate!
+        return unless project.lfs_enabled?
         return if skip_lfs_integrity_check
 
         logger.log_timed(LOG_MESSAGE) do

lib/gitlab/prometheus/metric_group.rb

@@ -11,9 +11,15 @@ module Gitlab
       validates :name, :priority, :metrics, presence: true
 
       def self.common_metrics
-        ::PrometheusMetric.common.group_by(&:group_title).map do |name, metrics|
-          MetricGroup.new(name: name, priority: 0, metrics: metrics.map(&:to_query_metric))
+        all_groups = ::PrometheusMetric.common.group_by(&:group_title).map do |name, metrics|
+          MetricGroup.new(
+            name: name,
+            priority: metrics.map(&:priority).max,
+            metrics: metrics.map(&:to_query_metric)
+          )
         end
+
+        all_groups.sort_by(&:priority).reverse
       end
 
       # EE only

spec/db/importers/common_metrics_importer_spec.rb

@@ -4,12 +4,18 @@ require 'rails_helper'
 require Rails.root.join("db", "importers", "common_metrics_importer.rb")
 
 describe Importers::PrometheusMetric do
+  let(:existing_group_titles) do
+    ::PrometheusMetric::GROUP_DETAILS.each_with_object({}) do |(key, value), memo|
+      memo[key] = value[:group_title]
+    end
+  end
+
   it 'group enum equals ::PrometheusMetric' do
     expect(described_class.groups).to eq(::PrometheusMetric.groups)
   end
 
   it 'GROUP_TITLES equals ::PrometheusMetric' do
-    expect(described_class::GROUP_TITLES).to eq(::PrometheusMetric::GROUP_TITLES)
+    expect(described_class::GROUP_TITLES).to eq(existing_group_titles)
   end
 end
 

spec/lib/gitlab/checks/diff_check_spec.rb

@@ -47,5 +47,43 @@ describe Gitlab::Checks::DiffCheck do
         end
       end
     end
+
+    context 'commit diff validations' do
+      before do
+        allow(subject).to receive(:validations_for_diff).and_return([lambda { |diff| return }])
+
+        expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
+
+        subject.validate!
+      end
+
+      context 'when request store is inactive' do
+        it 'are run for every commit' do
+          expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
+
+          subject.validate!
+        end
+      end
+
+      context 'when request store is active', :request_store do
+        it 'are cached for every commit' do
+          expect_any_instance_of(Commit).not_to receive(:raw_deltas)
+
+          subject.validate!
+        end
+
+        it 'are run for not cached commits' do
+          allow(project.repository).to receive(:new_commits).and_return(
+            project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', 'a5391128b0ef5d21df5dd23d98557f4ef12fae20')
+          )
+          change_access.instance_variable_set(:@commits, project.repository.new_commits)
+
+          expect(project.repository.new_commits.first).not_to receive(:raw_deltas).and_call_original
+          expect(project.repository.new_commits.last).to receive(:raw_deltas).and_call_original
+
+          subject.validate!
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/git_access_spec.rb

@@ -709,10 +709,22 @@ describe Gitlab::GitAccess do
       project.add_developer(user)
     end
 
-    it 'checks LFS integrity only for first change' do
-      expect_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).exactly(1).times
+    context 'when LFS is not enabled' do
+      it 'does not run LFSIntegrity check' do
+        expect(Gitlab::Checks::LfsIntegrity).not_to receive(:new)
 
-      push_access_check
+        push_access_check
+      end
+    end
+
+    context 'when LFS is enabled' do
+      it 'checks LFS integrity only for first change' do
+        allow(project).to receive(:lfs_enabled?).and_return(true)
+
+        expect_any_instance_of(Gitlab::Checks::LfsIntegrity).to receive(:objects_missing?).exactly(1).times
+
+        push_access_check
+      end
     end
   end
 

spec/lib/gitlab/prometheus/metric_group_spec.rb

@@ -21,6 +21,13 @@ describe Gitlab::Prometheus::MetricGroup do
         common_metric_group_a.id, common_metric_group_b_q1.id,
         common_metric_group_b_q2.id)
     end
+
+    it 'orders by priority' do
+      priorities = subject.map(&:priority)
+      names = subject.map(&:name)
+      expect(priorities).to eq([10, 5])
+      expect(names).to eq(['Response metrics (AWS ELB)', 'System metrics (Kubernetes)'])
+    end
   end
 
   describe '.for_project' do

spec/models/prometheus_metric_spec.rb

@@ -59,11 +59,65 @@ describe PrometheusMetric do
       end
     end
 
+    it_behaves_like 'group_title', :nginx_ingress_vts, 'Response metrics (NGINX Ingress VTS)'
+    it_behaves_like 'group_title', :nginx_ingress, 'Response metrics (NGINX Ingress)'
+    it_behaves_like 'group_title', :ha_proxy, 'Response metrics (HA Proxy)'
+    it_behaves_like 'group_title', :aws_elb, 'Response metrics (AWS ELB)'
+    it_behaves_like 'group_title', :nginx, 'Response metrics (NGINX)'
+    it_behaves_like 'group_title', :kubernetes, 'System metrics (Kubernetes)'
     it_behaves_like 'group_title', :business, 'Business metrics (Custom)'
     it_behaves_like 'group_title', :response, 'Response metrics (Custom)'
     it_behaves_like 'group_title', :system, 'System metrics (Custom)'
   end
 
+  describe '#priority' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:group, :priority) do
+      :nginx_ingress_vts | 10
+      :nginx_ingress     | 10
+      :ha_proxy          | 10
+      :aws_elb           | 10
+      :nginx             | 10
+      :kubernetes        | 5
+      :business          | 0
+      :response          | -5
+      :system            | -10
+    end
+
+    with_them do
+      before do
+        subject.group = group
+      end
+
+      it { expect(subject.priority).to eq(priority) }
+    end
+  end
+
+  describe '#required_metrics' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:group, :required_metrics) do
+      :nginx_ingress_vts | %w(nginx_upstream_responses_total nginx_upstream_response_msecs_avg)
+      :nginx_ingress     | %w(nginx_ingress_controller_requests nginx_ingress_controller_ingress_upstream_latency_seconds_sum)
+      :ha_proxy          | %w(haproxy_frontend_http_requests_total haproxy_frontend_http_responses_total)
+      :aws_elb           | %w(aws_elb_request_count_sum aws_elb_latency_average aws_elb_httpcode_backend_5_xx_sum)
+      :nginx             | %w(nginx_server_requests nginx_server_requestMsec)
+      :kubernetes        | %w(container_memory_usage_bytes container_cpu_usage_seconds_total)
+      :business          | %w()
+      :response          | %w()
+      :system            | %w()
+    end
+
+    with_them do
+      before do
+        subject.group = group
+      end
+
+      it { expect(subject.required_metrics).to eq(required_metrics) }
+    end
+  end
+
   describe '#to_query_metric' do
     it 'converts to queryable metric object' do
       expect(subject.to_query_metric).to be_instance_of(Gitlab::Prometheus::Metric)