Correct ordering of metrics

Correct the ordering of metrics on performance dashboard. Before common
metrics were moved into the DB, metric groups were ordered by the
priority defined in the common_metrics.yml file.
This commit adds a priority to each metric group in the PrometheusMetric
model.
It also combines title, priority and required_metrics into one frozen
GROUP_DETAILS hash so that the code is clearer.
This can be done since there is a fixed set of groups which are not
configurable.

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)