Add finder for prometheus metrics

Adds a finder for the PrometheusMetric class to create a unified
way to retrieve instances of the class. Note that this should not
be confused with the PrometheusMetric model used for importing
common metrics into the database.

Updates all retrievals of PrometheusMetric instances to use the
new PrometheusMetricFinder class.

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,7 +10,9 @@ module EE
           extend ::Gitlab::Utils::Override
 
           def custom_metrics(project)
-            project.prometheus_metrics.all.group_by(&:group_title).map do |name, metrics|
+            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

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,7 +11,9 @@ module Gitlab
       validates :name, :priority, :metrics, presence: true
 
       def self.common_metrics
-        all_groups = ::PrometheusMetric.common.group_by(&:group_title).map do |name, metrics|
+        all_groups = ::PrometheusMetricsFinder.new(common: true).execute
+          .group_by(&:group_title)
+          .map do |name, metrics|
             MetricGroup.new(
               name: name,
               priority: metrics.map(&:priority).max,

lib/gitlab/prometheus/queries/knative_invocation_query.rb

@@ -7,9 +7,12 @@ 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|
+          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

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