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.

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

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

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

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

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/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)