diff --git a/app/models/prometheus_metric.rb b/app/models/prometheus_metric.rb index defbade1ed65d9cc272ff05e340dee2a5455b1eb..5594594a48dbcffeff51f6520d682e55288e6dee 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 0000000000000000000000000000000000000000..fbc7b58d901c323015d1be4670047065144dbeac --- /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 8f30cdee232943fc632d6fb1b43781b6533513c2..394556e8708314cfa05d460ee7739a31caf07e31 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 68260820958db113c904fd2bf5f319c8a18ff8c7..6133b17ac61b6971b7419cc936f73fae07a7bc40 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 e7d16e736631f94a4bb494a4204ac879133dab6e..5cc6827488befd3423aec01c7a67a4dbfea5b373 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 3692fe9a55989d93e750c88b611c1746f41ba07a..2b978c1c8ff93c52f26f98d9fb678a54a31b409e 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)