Merge branch 'add-metrics-finder-ee' into 'master'

Add finder for prometheus metrics

See merge request gitlab-org/gitlab!16235

app/finders/prometheus_metrics_finder.rb

+# frozen_string_literal: true
+
+class PrometheusMetricsFinder
+  ACCEPTED_PARAMS = [
+    :project,
+    :group,
+    :title,
+    :y_label,
+    :identifier,
+    :id,
+    :common,
+    :ordered
+  ].freeze
+
+  # Cautiously preferring a memoized class method over a constant
+  # so that the DB connection is accessed after the class is loaded.
+  def self.indexes
+    @indexes ||= PrometheusMetric
+      .connection
+      .indexes(:prometheus_metrics)
+      .map { |index| index.columns.map(&:to_sym) }
+  end
+
+  def initialize(params = {})
+    @params = params.slice(*ACCEPTED_PARAMS)
+  end
+
+  # @return [PrometheusMetric, PrometheusMetric::ActiveRecord_Relation]
+  def execute
+    validate_params!
+
+    metrics = by_project(::PrometheusMetric.all)
+    metrics = by_group(metrics)
+    metrics = by_title(metrics)
+    metrics = by_y_label(metrics)
+    metrics = by_common(metrics)
+    metrics = by_ordered(metrics)
+    metrics = by_identifier(metrics)
+    metrics = by_id(metrics)
+
+    metrics
+  end
+
+  private
+
+  attr_reader :params
+
+  def by_project(metrics)
+    return metrics unless params[:project]
+
+    metrics.for_project(params[:project])
+  end
+
+  def by_group(metrics)
+    return metrics unless params[:group]
+
+    metrics.for_group(params[:group])
+  end
+
+  def by_title(metrics)
+    return metrics unless params[:title]
+
+    metrics.for_title(params[:title])
+  end
+
+  def by_y_label(metrics)
+    return metrics unless params[:y_label]
+
+    metrics.for_y_label(params[:y_label])
+  end
+
+  def by_common(metrics)
+    return metrics unless params[:common]
+
+    metrics.common
+  end
+
+  def by_ordered(metrics)
+    return metrics unless params[:ordered]
+
+    metrics.ordered
+  end
+
+  def by_identifier(metrics)
+    return metrics unless params[:identifier]
+
+    metrics.for_identifier(params[:identifier])
+  end
+
+  def by_id(metrics)
+    return metrics unless params[:id]
+
+    metrics.id_in(params[:id])
+  end
+
+  def validate_params!
+    validate_params_present!
+    validate_id_params!
+    validate_indexes!
+  end
+
+  # Ensure all provided params are supported
+  def validate_params_present!
+    raise ArgumentError, "Please provide one or more of: #{ACCEPTED_PARAMS}" if params.blank?
+  end
+
+  # Protect against the caller "finding" the wrong metric
+  def validate_id_params!
+    raise ArgumentError, 'Only one of :identifier, :id is permitted' if params[:identifier] && params[:id]
+    raise ArgumentError, ':identifier must be scoped to a :project or :common' if params[:identifier] && !(params[:project] || params[:common])
+  end
+
+  # Protect against unaccounted-for, complex/slow queries.
+  # This is not a hard and fast rule, but is meant to encourage
+  # mindful inclusion of new queries.
+  def validate_indexes!
+    indexable_params = params.except(:ordered, :id, :project).keys
+    indexable_params << :project_id if params[:project]
+    indexable_params.sort!
+
+    raise ArgumentError, "An index should exist for params: #{indexable_params}" unless appropriate_index?(indexable_params)
+  end
+
+  def appropriate_index?(indexable_params)
+    return true if indexable_params.blank?
+
+    self.class.indexes.any? { |index| (index - indexable_params).empty? }
+  end
+end

app/models/prometheus_metric.rb

@@ -14,7 +14,13 @@ class PrometheusMetric < ApplicationRecord
   validates :project, presence: true, unless: :common?
   validates :project, absence: true, if: :common?
 
+  scope :for_project, -> (project) { where(project: project) }
+  scope :for_group, -> (group) { where(group: group) }
+  scope :for_title, -> (title) { where(title: title) }
+  scope :for_y_label, -> (y_label) { where(y_label: y_label) }
+  scope :for_identifier, -> (identifier) { where(identifier: identifier) }
   scope :common, -> { where(common: true) }
+  scope :ordered, -> { reorder(created_at: :asc) }
 
   def priority
     group_details(group).fetch(:priority)

app/services/metrics/dashboard/custom_metric_embed_service.rb

@@ -77,15 +77,14 @@ module Metrics
       # There may be multiple metrics, but they should be
       # displayed in a single panel/chart.
       # @return [ActiveRecord::AssociationRelation<PromtheusMetric>]
-      # rubocop: disable CodeReuse/ActiveRecord
       def metrics
-        project.prometheus_metrics.where(
+        PrometheusMetricsFinder.new(
+          project: project,
           group: group_key,
           title: title,
           y_label: y_label
-        )
+        ).execute
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       # Returns a symbol representing the group that
       # the dashboard's group title belongs to.

ee/app/controllers/ee/projects/prometheus/metrics_controller.rb

@@ -29,25 +29,31 @@ module EE
           @metric = project.prometheus_metrics.new # rubocop:disable Gitlab/ModuleWithInstanceVariables
         end
 
-        # rubocop: disable CodeReuse/ActiveRecord
         def index
           respond_to do |format|
             format.json do
-              metrics = project.prometheus_metrics
+              metrics = ::PrometheusMetricsFinder.new(
+                project: project,
+                ordered: true
+              ).execute.to_a
+
               response = {}
               if metrics.any?
-                response[:metrics] = ::PrometheusMetricSerializer.new(project: project)
-                                       .represent(metrics.order(created_at: :asc))
+                response[:metrics] = ::PrometheusMetricSerializer
+                                       .new(project: project)
+                                       .represent(metrics)
               end
 
               render json: response
             end
           end
         end
-        # rubocop: enable CodeReuse/ActiveRecord
 
         def create
-          @metric = project.prometheus_metrics.create(metrics_params) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          @metric = project.prometheus_metrics.create( # rubocop:disable Gitlab/ModuleWithInstanceVariables
+            metrics_params.to_h.symbolize_keys
+          )
+
           if @metric.persisted? # rubocop:disable Gitlab/ModuleWithInstanceVariables
             redirect_to edit_project_service_path(project, ::PrometheusService),
                         notice: _('Metric was successfully added.')
@@ -57,8 +63,7 @@ module EE
         end
 
         def update
-          @metric = project.prometheus_metrics.find(params[:id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-          @metric = update_metrics_service(@metric).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          @metric = update_metrics_service(prometheus_metric).execute # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
           if @metric.persisted? # rubocop:disable Gitlab/ModuleWithInstanceVariables
             redirect_to edit_project_service_path(project, ::PrometheusService),
@@ -69,12 +74,11 @@ module EE
         end
 
         def edit
-          @metric = project.prometheus_metrics.find(params[:id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          @metric = prometheus_metric # rubocop:disable Gitlab/ModuleWithInstanceVariables
         end
 
         def destroy
-          metric = project.prometheus_metrics.find(params[:id])
-          destroy_metrics_service(metric).execute
+          destroy_metrics_service(prometheus_metric).execute
 
           respond_to do |format|
             format.html do
@@ -92,6 +96,10 @@ module EE
           render_404 unless project.feature_available?(:custom_prometheus_metrics)
         end
 
+        def prometheus_metric
+          @prometheus_metric ||= ::PrometheusMetricsFinder.new(id: params[:id]).execute.first
+        end
+
         def update_metrics_service(metric)
           ::Projects::Prometheus::Metrics::UpdateService.new(metric, metrics_params)
         end

ee/lib/ee/gitlab/prometheus/metric_group.rb

@@ -10,10 +10,12 @@ module EE
           extend ::Gitlab::Utils::Override
 
           def custom_metrics(project)
-            project.prometheus_metrics.all.group_by(&:group_title).map do |name, metrics|
-              ::Gitlab::Prometheus::MetricGroup.new(
-                name: name, priority: 0, metrics: metrics.map(&:to_query_metric))
-            end
+            PrometheusMetricsFinder.new(project: project).execute
+              .group_by(&:group_title)
+              .map do |name, metrics|
+                ::Gitlab::Prometheus::MetricGroup.new(
+                  name: name, priority: 0, metrics: metrics.map(&:to_query_metric))
+              end
           end
 
           override :for_project

lib/gitlab/metrics/dashboard/stages/common_metrics_inserter.rb

@@ -9,7 +9,7 @@ module Gitlab
           # find a corresponding database record. If found,
           # includes the record's id in the dashboard config.
           def transform!
-            common_metrics = ::PrometheusMetric.common
+            common_metrics = ::PrometheusMetricsFinder.new(common: true).execute
 
             for_metrics do |metric|
               metric_record = common_metrics.find { |m| m.identifier == metric[:id] }

lib/gitlab/metrics/dashboard/stages/project_metrics_inserter.rb

@@ -9,7 +9,7 @@ module Gitlab
           # config. If there are no project-specific metrics,
           # this will have no effect.
           def transform!
-            project.prometheus_metrics.each do |project_metric|
+            PrometheusMetricsFinder.new(project: project).execute.each do |project_metric|
               group = find_or_create_panel_group(dashboard[:panel_groups], project_metric)
               panel = find_or_create_panel(group[:panels], project_metric)
               find_or_create_metric(panel[:metrics], project_metric)

lib/gitlab/prometheus/metric_group.rb

@@ -11,13 +11,15 @@ module Gitlab
       validates :name, :priority, :metrics, presence: true
 
       def self.common_metrics
-        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 = ::PrometheusMetricsFinder.new(common: true).execute
+          .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

lib/gitlab/prometheus/queries/knative_invocation_query.rb

@@ -7,11 +7,14 @@ module Gitlab
         include QueryAdditionalMetrics
 
         def query(serverless_function_id)
-          PrometheusMetric
-            .find_by_identifier(:system_metrics_knative_function_invocation_count)
-            .to_query_metric.tap do |q|
-            q.queries[0][:result] = run_query(q.queries[0][:query_range], context(serverless_function_id))
-          end
+          PrometheusMetricsFinder
+            .new(identifier: :system_metrics_knative_function_invocation_count, common: true)
+            .execute
+            .first
+            .to_query_metric
+            .tap do |q|
+              q.queries[0][:result] = run_query(q.queries[0][:query_range], context(serverless_function_id))
+            end
         end
 
         protected

spec/finders/prometheus_metrics_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PrometheusMetricsFinder do
+  describe '#execute' do
+    let(:finder) { described_class.new(params) }
+    let(:params) { {} }
+
+    subject { finder.execute }
+
+    context 'with params' do
+      let_it_be(:project) { create(:project) }
+      let_it_be(:project_metric) { create(:prometheus_metric, project: project) }
+      let_it_be(:common_metric) { create(:prometheus_metric, :common) }
+      let_it_be(:unique_metric) do
+        create(
+          :prometheus_metric,
+          :common,
+          title: 'Unique title',
+          y_label: 'Unique y_label',
+          group: :kubernetes,
+          identifier: 'identifier',
+          created_at: 5.minutes.ago
+        )
+      end
+
+      context 'with appropriate indexes' do
+        before do
+          allow_any_instance_of(described_class).to receive(:appropriate_index?).and_return(true)
+        end
+
+        context 'with project' do
+          let(:params) { { project: project } }
+
+          it { is_expected.to eq([project_metric]) }
+        end
+
+        context 'with group' do
+          let(:params) { { group: project_metric.group } }
+
+          it { is_expected.to contain_exactly(common_metric, project_metric) }
+        end
+
+        context 'with title' do
+          let(:params) { { title: project_metric.title } }
+
+          it { is_expected.to contain_exactly(project_metric, common_metric) }
+        end
+
+        context 'with y_label' do
+          let(:params) { { y_label: project_metric.y_label } }
+
+          it { is_expected.to contain_exactly(project_metric, common_metric) }
+        end
+
+        context 'with common' do
+          let(:params) { { common: true } }
+
+          it { is_expected.to contain_exactly(common_metric, unique_metric) }
+        end
+
+        context 'with ordered' do
+          let(:params) { { ordered: true } }
+
+          it { is_expected.to eq([unique_metric, project_metric, common_metric]) }
+        end
+
+        context 'with indentifier' do
+          let(:params) { { identifier: unique_metric.identifier } }
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(
+              ArgumentError,
+              ':identifier must be scoped to a :project or :common'
+            )
+          end
+
+          context 'with common' do
+            let(:params) { { identifier: unique_metric.identifier, common: true } }
+
+            it { is_expected.to contain_exactly(unique_metric) }
+          end
+
+          context 'with id' do
+            let(:params) { { id: 14, identifier: 'string' } }
+
+            it 'raises an error' do
+              expect { subject }.to raise_error(
+                ArgumentError,
+                'Only one of :identifier, :id is permitted'
+              )
+            end
+          end
+        end
+
+        context 'with id' do
+          let(:params) { { id: common_metric.id } }
+
+          it { is_expected.to contain_exactly(common_metric) }
+        end
+
+        context 'with multiple params' do
+          let(:params) do
+            {
+              group: project_metric.group,
+              title: project_metric.title,
+              y_label: project_metric.y_label,
+              common: true,
+              ordered: true
+            }
+          end
+
+          it { is_expected.to contain_exactly(common_metric) }
+        end
+      end
+
+      context 'without an appropriate index' do
+        let(:params) do
+          {
+            title: project_metric.title,
+            ordered: true
+          }
+        end
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(
+            ArgumentError,
+            'An index should exist for params: [:title]'
+          )
+        end
+      end
+    end
+
+    context 'without params' do
+      it 'raises an error' do
+        expect { subject }.to raise_error(
+          ArgumentError,
+          'Please provide one or more of: [:project, :group, :title, :y_label, :identifier, :id, :common, :ordered]'
+        )
+      end
+    end
+  end
+end

spec/lib/gitlab/prometheus/queries/knative_invocation_query_spec.rb

@@ -13,14 +13,19 @@ describe Gitlab::Prometheus::Queries::KnativeInvocationQuery do
 
   context 'verify queries' do
     before do
-      allow(PrometheusMetric).to receive(:find_by_identifier).and_return(create(:prometheus_metric, query: prometheus_istio_query('test-name', 'test-ns')))
-      allow(client).to receive(:query_range)
+      create(:prometheus_metric,
+             :common,
+             identifier: :system_metrics_knative_function_invocation_count,
+             query: 'sum(ceil(rate(istio_requests_total{destination_service_namespace="%{kube_namespace}", destination_app=~"%{function_name}.*"}[1m])*60))')
     end
 
     it 'has the query, but no data' do
-      results = subject.query(serverless_func.id)
+      expect(client).to receive(:query_range).with(
+        'sum(ceil(rate(istio_requests_total{destination_service_namespace="test-ns", destination_app=~"test-name.*"}[1m])*60))',
+        hash_including(:start, :stop)
+      )
 
-      expect(results.queries[0][:query_range]).to eql('floor(sum(rate(istio_revision_request_count{destination_configuration="test-name", destination_namespace="test-ns"}[1m])*30))')
+      subject.query(serverless_func.id)
     end
   end
 end