Merge branch '51970-correct-ordering-of-metrics' into 'master'

Correct the ordering of metrics on performance dashboard

Closes #51970

See merge request gitlab-org/gitlab-ce!23630

6 files changed, 140 insertions, 28 deletions

diff --git a/app/models/prometheus_metric.rb b/app/models/prometheus_metric.rb
index defbade1ed6..5594594a48d 100644
--- a/app/models/prometheus_metric.rb
+++ b/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,4 +117,10 @@ class PrometheusMetric < ActiveRecord::Base
       }]
     end
   end
+
+  private
+
+  def group_details(group)
+    GROUP_DETAILS.fetch(group.to_sym)
+  end
 end
diff --git a/changelogs/unreleased/51970-correct-ordering-of-metrics.yml b/changelogs/unreleased/51970-correct-ordering-of-metrics.yml
new file mode 100644
index 00000000000..fbc7b58d901
--- /dev/null
+++ b/changelogs/unreleased/51970-correct-ordering-of-metrics.yml
@@ -0,0 +1,5 @@
+---
+title: Correct the ordering of metrics on the performance dashboard
+merge_request: 23630
+author:
+type: fixed
diff --git a/lib/gitlab/prometheus/metric_group.rb b/lib/gitlab/prometheus/metric_group.rb
index 8f30cdee232..394556e8708 100644
--- a/lib/gitlab/prometheus/metric_group.rb
+++ b/lib/gitlab/prometheus/metric_group.rb
@@ -10,9 +10,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
diff --git a/spec/db/importers/common_metrics_importer_spec.rb b/spec/db/importers/common_metrics_importer_spec.rb
index 68260820958..6133b17ac61 100644
--- a/spec/db/importers/common_metrics_importer_spec.rb
+++ b/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
 
diff --git a/spec/lib/gitlab/prometheus/metric_group_spec.rb b/spec/lib/gitlab/prometheus/metric_group_spec.rb
index e7d16e73663..5cc6827488b 100644
--- a/spec/lib/gitlab/prometheus/metric_group_spec.rb
+++ b/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
diff --git a/spec/models/prometheus_metric_spec.rb b/spec/models/prometheus_metric_spec.rb
index 3692fe9a559..2b978c1c8ff 100644
--- a/spec/models/prometheus_metric_spec.rb
+++ b/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)