Move metric dashboard sequences

This moves metric dashboard stage sequences from the dashboard
processorto their approriate services. It also refactors the metric
dashboard finder by moving the environment argument into the options
hash.

app/controllers/projects/environments_controller.rb

@@ -164,7 +164,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
       result = dashboard_finder.find(
         project,
         current_user,
-        environment,
+        environment: environment,
         dashboard_path: params[:dashboard],
         **dashboard_params.to_h.symbolize_keys
       )
@@ -172,13 +172,13 @@ class Projects::EnvironmentsController < Projects::ApplicationController
       result = dashboard_finder.find(
         project,
         current_user,
-        environment,
+        environment: environment,
         dashboard_path: params[:dashboard]
       )
 
       result[:all_dashboards] = dashboard_finder.find_all_paths(project)
     else
-      result = dashboard_finder.find(project, current_user, environment)
+      result = dashboard_finder.find(project, current_user, environment: environment)
     end
 
     respond_to do |format|

app/services/metrics/dashboard/base_service.rb

@@ -7,6 +7,13 @@ module Metrics
     class BaseService < ::BaseService
       include Gitlab::Metrics::Dashboard::Errors
 
+      STAGES = ::Gitlab::Metrics::Dashboard::Stages
+      SEQUENCE = [
+        STAGES::CommonMetricsInserter,
+        STAGES::EndpointInserter,
+        STAGES::Sorter
+      ].freeze
+
       def get_dashboard
         return error('Insufficient permissions.', :unauthorized) unless allowed?
 
@@ -31,24 +38,20 @@ module Metrics
       # Determines whether users should be able to view
       # dashboards at all.
       def allowed?
-        if params[:cluster]
+        if params[:environment]
+          Ability.allowed?(current_user, :read_environment, project)
+        elsif params[:cluster]
           true # Authorization handled at controller level
         else
-          Ability.allowed?(current_user, :read_environment, project)
+          false
         end
       end
 
       # Returns a new dashboard Hash, supplemented with DB info
       def process_dashboard
-        if params[:cluster]
-          ::Gitlab::Metrics::Dashboard::ClusterProcessor
-            .new(project, raw_dashboard, params)
-            .process
-        else
-          ::Gitlab::Metrics::Dashboard::Processor
-            .new(project, raw_dashboard, params)
-            .process(insert_project_metrics: insert_project_metrics?)
-        end
+        ::Gitlab::Metrics::Dashboard::Processor
+          .new(project, raw_dashboard, sequence, params)
+          .process
       end
 
       # @return [String] Relative filepath of the dashboard yml
@@ -66,12 +69,11 @@ module Metrics
         raise NotImplementedError
       end
 
-      # Determines whether custom metrics should be included
-      # in the processed output.
-      # @return [Boolean]
-      def insert_project_metrics?
-        false
+      def sequence
+        SEQUENCE
       end
     end
   end
 end
+
+Metrics::Dashboard::BaseService.prepend_if_ee('EE::Metrics::Dashboard::BaseService')

app/services/metrics/dashboard/system_dashboard_service.rb

@@ -8,6 +8,13 @@ module Metrics
       SYSTEM_DASHBOARD_PATH = 'config/prometheus/common_metrics.yml'
       SYSTEM_DASHBOARD_NAME = 'Default'
 
+      SEQUENCE = [
+        STAGES::CommonMetricsInserter,
+        STAGES::ProjectMetricsInserter,
+        STAGES::EndpointInserter,
+        STAGES::Sorter
+      ].freeze
+
       class << self
         def all_dashboard_paths(_project)
           [{
@@ -24,6 +31,10 @@ module Metrics
 
       private
 
+      def cache_key
+        "metrics_dashboard_#{dashboard_path}"
+      end
+
       def dashboard_path
         SYSTEM_DASHBOARD_PATH
       end
@@ -35,13 +46,11 @@ module Metrics
         YAML.safe_load(yml)
       end
 
-      def cache_key
-        "metrics_dashboard_#{dashboard_path}"
-      end
-
-      def insert_project_metrics?
-        true
+      def sequence
+        SEQUENCE
       end
     end
   end
 end
+
+Metrics::Dashboard::SystemDashboardService.prepend_if_ee('EE::Metrics::Dashboard::SystemDashboardService')

ee/app/controllers/ee/clusters/clusters_controller.rb

@@ -52,18 +52,18 @@ module EE
       
       def metrics_dashboard
         project_for_dashboard = defined?(project) ? project : nil
-        dashboard = dashboard_finder.find(project_for_dashboard, current_user, nil, metrics_dashboard_params)
+        dashboard = ::Gitlab::Metrics::Dashboard::Finder.find(project_for_dashboard, current_user, metrics_dashboard_params)
 
         respond_to do |format|
           if dashboard[:status] == :success
             format.json do
-              render status: :ok, json: dashboard.slice(:all_dashboards, :dashboard, :status)
+              render status: :ok, json: dashboard.slice(:dashboard, :status)
             end
           else
             format.json do
               render(
                 status: dashboard[:http_status],
-                json: dashboard.slice(:all_dashboards, :message, :status)
+                json: dashboard.slice(:message, :status)
               )
             end
           end
@@ -72,10 +72,6 @@ module EE
 
       private
 
-      def dashboard_finder
-        ::Gitlab::Metrics::Dashboard::Finder
-      end
-
       def prometheus_adapter
         return unless cluster&.application_prometheus_available?
 

ee/app/services/ee/metrics/dashboard/base_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Metrics
+    module Dashboard
+      module BaseService
+        extend ::Gitlab::Utils::Override
+
+        EE_SEQUENCE = [
+          ::EE::Gitlab::Metrics::Dashboard::Stages::AlertsInserter
+        ].freeze
+
+        override :sequence
+        def sequence
+          super + EE_SEQUENCE
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/metrics/dashboard/system_dashboard_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Metrics
+    module Dashboard
+      module SystemDashboardService
+        extend ::Gitlab::Utils::Override
+
+        EE_SEQUENCE = [
+          ::EE::Gitlab::Metrics::Dashboard::Stages::AlertsInserter
+        ].freeze
+
+        override :sequence
+        def sequence
+          super + EE_SEQUENCE
+        end
+      end
+    end
+  end
+end

ee/app/services/metrics/dashboard/cluster_dashboard_service.rb

@@ -8,18 +8,18 @@ module Metrics
       CLUSTER_DASHBOARD_PATH = 'ee/config/prometheus/cluster_metrics_new.yml'
       CLUSTER_DASHBOARD_NAME = 'Cluster'
 
-      class << self
-        def all_dashboard_paths(_project)
-          [{
-            path: CLUSTER_DASHBOARD_PATH,
-            display_name: CLUSTER_DASHBOARD_NAME,
-            default: true
-          }]
-        end
-      end
+      SEQUENCE = [
+        STAGES::CommonMetricsInserter,
+        STAGES::ClusterEndpointInserter,
+        STAGES::Sorter
+      ].freeze
 
       private
 
+      def cache_key
+        "metrics_dashboard_#{dashboard_path}"
+      end
+
       def dashboard_path
         CLUSTER_DASHBOARD_PATH
       end
@@ -31,12 +31,8 @@ module Metrics
         YAML.safe_load(yml)
       end
 
-      def cache_key
-        "metrics_dashboard_#{dashboard_path}"
-      end
-
-      def insert_project_metrics?
-        false
+      def sequence
+        SEQUENCE
       end
     end
   end

ee/lib/ee/gitlab/metrics/dashboard/processor.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module Metrics
-      module Dashboard
-        module Processor
-          extend ::Gitlab::Utils::Override
-
-          EE_SEQUENCE = [
-            Stages::AlertsInserter
-          ].freeze
-
-          override :sequence
-          def sequence(_insert_project_metrics)
-            super + EE_SEQUENCE
-          end
-        end
-      end
-    end
-  end
-end

ee/lib/gitlab/metrics/dashboard/cluster_processor.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Metrics
-    module Dashboard
-      # Responsible for processesing a dashboard hash, inserting
-      # relevant DB records & sorting for proper rendering in
-      # the UI. These includes shared metric info, custom metrics
-      # info, and alerts (only in EE).
-      class ClusterProcessor
-        SEQUENCE = [
-          Stages::CommonMetricsInserter,
-          Stages::ClusterEndpointInserter,
-          Stages::Sorter
-        ].freeze
-
-        def initialize(project, dashboard, params)
-          @project = project
-          @dashboard = dashboard
-          @params = params
-        end
-
-        # Returns a new dashboard hash with the results of
-        # running transforms on the dashboard.
-        def process
-          @dashboard.deep_symbolize_keys.tap do |dashboard|
-            SEQUENCE.each do |stage|
-              stage.new(@project, dashboard, @params).transform!
-            end
-          end
-        end
-      end
-    end
-  end
-end

ee/lib/gitlab/metrics/dashboard/stages/cluster_endpoint_inserter.rb

@@ -15,36 +15,51 @@ module Gitlab
 
           private
 
-          def endpoint_for_metric(metric)
-            proxy_path = query_type(metric)
-            query = query_for_metric(metric)
+          def admin_url(metric)
+            Gitlab::Routing.url_helpers.prometheus_api_admin_cluster_path(
+              params[:cluster],
+              proxy_path: query_type(metric),
+              query: query_for_metric(metric)
+            )
+          end
 
+          def endpoint_for_metric(metric)
             case params[:cluster_type]
             when :admin
-              Gitlab::Routing.url_helpers.prometheus_api_admin_cluster_path(
-                params[:cluster],
-                proxy_path: proxy_path,
-                query: query
-              )
+              admin_url(metric)
             when :group
-              Gitlab::Routing.url_helpers.prometheus_api_group_cluster_path(
-                params[:group],
-                params[:cluster],
-                proxy_path: proxy_path,
-                query: query
-              )
+              error!('Group is required when cluster_type is :group') unless params[:group]
+              group_url(metric)
             when :project
-              Gitlab::Routing.url_helpers.prometheus_api_project_cluster_path(
-                project,
-                params[:cluster],
-                proxy_path: proxy_path,
-                query: query
-              )
+              error!('Project is required when cluster_type is :project') unless project
+              project_url(metric)
             else
-              Errors::DashboardProcessingError.new('Unrecognized cluster type')
+              error!('Unrecognized cluster type')
             end
           end
 
+          def error!(message)
+            raise Errors::DashboardProcessingError.new(message)
+          end
+
+          def group_url(metric)
+            Gitlab::Routing.url_helpers.prometheus_api_group_cluster_path(
+              params[:group],
+              params[:cluster],
+              proxy_path: query_type(metric),
+              query: query_for_metric(metric)
+            )
+          end
+
+          def project_url(metric)
+            Gitlab::Routing.url_helpers.prometheus_api_project_cluster_path(
+              project,
+              params[:cluster],
+              proxy_path: query_type(metric),
+              query: query_for_metric(metric)
+            )
+          end
+
           def query_type(metric)
             metric[:query] ? :query : :query_range
           end
@@ -60,17 +75,6 @@ module Gitlab
           def verify_params
             raise Errors::DashboardProcessingError.new('Cluster is required for Stages::ClusterEndpointInserter') unless params[:cluster]
             raise Errors::DashboardProcessingError.new('Cluster type must be specificed for Stages::ClusterEndpointInserter') unless params[:cluster_type]
-
-            verify_type_params
-          end
-
-          def verify_type_params
-            case params[:cluster_type]
-            when :group
-              raise Errors::DashboardProcessingError.new('Group is required when cluster_type is :group') unless params[:group]
-            when :project
-              raise Errors::DashboardProcessingError.new('Project is required when cluster_type is :project') unless project
-            end
           end
         end
       end

ee/spec/lib/ee/gitlab/metrics/dashboard/processor_spec.rb

@@ -6,19 +6,19 @@ describe Gitlab::Metrics::Dashboard::Processor do
   let(:project) { build(:project) }
   let(:environment) { create(:environment, project: project) }
   let(:dashboard_yml) { YAML.load_file('spec/fixtures/lib/gitlab/metrics/dashboard/sample_dashboard.yml') }
-  let(:params) { [project, dashboard_yml, { environment: environment }] }
-
-  describe 'sequence' do
-    let(:environment) { build(:environment) }
-    let(:sequence) { described_class.new(*params).__send__(:sequence, insert_project_metrics: true) }
-
-    it 'includes the alerts processing stage' do
-      expect(sequence.length).to eq(5)
-    end
-  end
+  let(:params) { [project, dashboard_yml, sequence, { environment: environment }] }
 
   describe 'process' do
-    let(:dashboard) { described_class.new(*params).process(insert_project_metrics: true) }
+    let(:dashboard) { described_class.new(*params).process }
+    let(:sequence) do
+      [
+        Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter,
+        Gitlab::Metrics::Dashboard::Stages::ProjectMetricsInserter,
+        Gitlab::Metrics::Dashboard::Stages::EndpointInserter,
+        Gitlab::Metrics::Dashboard::Stages::Sorter,
+        ::EE::Gitlab::Metrics::Dashboard::Stages::AlertsInserter
+      ]
+    end
 
     context 'when the dashboard references persisted metrics with alerts' do
       let!(:alert) do

ee/spec/lib/gitlab/metrics/dashboard/cluster_processor_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Gitlab::Metrics::Dashboard::ClusterProcessor do
-  let(:cluster_project) { build(:cluster_project) }
-  let(:cluster) { cluster_project.cluster }
-  let(:project) { cluster_project.project }
-  let(:dashboard_yml) { YAML.load_file('spec/fixtures/lib/gitlab/metrics/dashboard/sample_dashboard.yml') }
-
-  describe 'process' do
-    let(:process_params) { [project, dashboard_yml, { cluster: cluster, cluster_type: :project }] }
-    let(:dashboard) { described_class.new(*process_params).process }
-
-    it 'includes a path for the prometheus endpoint with each metric' do
-      expect(all_metrics).to satisfy_all do |metric|
-        metric[:prometheus_endpoint_path] == prometheus_path(metric[:query_range])
-      end
-    end
-
-    context 'when dashboard config corresponds to common metrics' do
-      let!(:common_metric) { create(:prometheus_metric, :common, identifier: 'metric_a1') }
-
-      it 'inserts metric ids into the config' do
-        target_metric = all_metrics.find { |metric| metric[:id] == 'metric_a1' }
-
-        expect(target_metric).to include(:metric_id)
-        expect(target_metric[:metric_id]).to eq(common_metric.id)
-      end
-    end
-
-    shared_examples_for 'errors with message' do |expected_message|
-      it 'raises a DashboardLayoutError' do
-        error_class = Gitlab::Metrics::Dashboard::Errors::DashboardProcessingError
-        expect { dashboard }.to raise_error(error_class, expected_message)
-      end
-    end
-
-    context 'when the dashboard is missing panel_groups' do
-      let(:dashboard_yml) { {} }
-
-      it_behaves_like 'errors with message', 'Top-level key :panel_groups must be an array'
-    end
-
-    context 'when the dashboard contains a panel_group which is missing panels' do
-      let(:dashboard_yml) { { panel_groups: [{}] } }
-
-      it_behaves_like 'errors with message', 'Each "panel_group" must define an array :panels'
-    end
-
-    context 'when the dashboard contains a panel which is missing metrics' do
-      let(:dashboard_yml) { { panel_groups: [{ panels: [{}] }] } }
-
-      it_behaves_like 'errors with message', 'Each "panel" must define an array :metrics'
-    end
-
-    context 'when the dashboard contains a metric which is missing a query' do
-      let(:dashboard_yml) { { panel_groups: [{ panels: [{ metrics: [{}] }] }] } }
-
-      it_behaves_like 'errors with message', 'Each "metric" must define one of :query or :query_range'
-    end
-  end
-
-  private
-
-  def all_metrics
-    dashboard[:panel_groups].flat_map do |group|
-      group[:panels].flat_map { |panel| panel[:metrics] }
-    end
-  end
-
-  def get_metric_details(metric)
-    {
-      query_range: metric.query,
-      unit: metric.unit,
-      label: metric.legend,
-      metric_id: metric.id,
-      prometheus_endpoint_path: prometheus_path(metric.query)
-    }
-  end
-
-  def prometheus_path(query)
-    Gitlab::Routing.url_helpers.prometheus_api_project_cluster_path(
-      project,
-      cluster,
-      proxy_path: :query_range,
-      query: query
-    )
-  end
-end

ee/spec/services/metrics/dashboard/cluster_dashboard_service_spec.rb

@@ -31,22 +31,8 @@ describe Metrics::Dashboard::ClusterDashboardService, :use_clean_rails_memory_st
     context 'when called with a non-system dashboard' do
       let(:dashboard_path) { 'garbage/dashboard/path' }
 
-      # We want to alwaus return the system dashboard.
+      # We want to alwaus return the cluster dashboard.
       it_behaves_like 'valid dashboard service response'
     end
   end
-
-  describe '::all_dashboard_paths' do
-    it 'returns the dashboard attributes' do
-      all_dashboards = described_class.all_dashboard_paths(project)
-
-      expect(all_dashboards).to eq(
-        [{
-          path: described_class::CLUSTER_DASHBOARD_PATH,
-          display_name: described_class::CLUSTER_DASHBOARD_NAME,
-          default: true
-        }]
-      )
-    end
-  end
 end

lib/gitlab/metrics/dashboard/finder.rb

@@ -28,9 +28,9 @@ module Gitlab
           # @param options - y_label [String] Y-Axis label of
           #         a panel. Used by embedded dashboards.
           # @return [Hash]
-          def find(project, user, environment = nil, options = {})
+          def find(project, user, options = {})
             service_for(options)
-              .new(project, user, options.merge(environment: environment))
+              .new(project, user, options)
               .get_dashboard
           end
 

lib/gitlab/metrics/dashboard/processor.rb

@@ -8,43 +8,23 @@ module Gitlab
       # the UI. These includes shared metric info, custom metrics
       # info, and alerts (only in EE).
       class Processor
-        SYSTEM_SEQUENCE = [
-          Stages::CommonMetricsInserter,
-          Stages::ProjectMetricsInserter,
-          Stages::EndpointInserter,
-          Stages::Sorter
-        ].freeze
-
-        PROJECT_SEQUENCE = [
-          Stages::CommonMetricsInserter,
-          Stages::EndpointInserter,
-          Stages::Sorter
-        ].freeze
-
-        def initialize(project, dashboard, params)
+        def initialize(project, dashboard, sequence, params)
           @project = project
           @dashboard = dashboard
+          @sequence = sequence
           @params = params
         end
 
         # Returns a new dashboard hash with the results of
         # running transforms on the dashboard.
-        def process(insert_project_metrics:)
+        def process
           @dashboard.deep_symbolize_keys.tap do |dashboard|
-            sequence(insert_project_metrics).each do |stage|
+            @sequence.each do |stage|
               stage.new(@project, dashboard, @params).transform!
             end
           end
         end
-
-        private
-
-        def sequence(insert_project_metrics)
-          insert_project_metrics ? SYSTEM_SEQUENCE : PROJECT_SEQUENCE
-        end
       end
     end
   end
 end
-
-Gitlab::Metrics::Dashboard::Processor.prepend_if_ee('EE::Gitlab::Metrics::Dashboard::Processor')

spec/lib/gitlab/metrics/dashboard/finder_spec.rb

@@ -15,7 +15,7 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
 
   describe '.find' do
     let(:dashboard_path) { '.gitlab/dashboards/test.yml' }
-    let(:service_call) { described_class.find(project, user, environment, dashboard_path: dashboard_path) }
+    let(:service_call) { described_class.find(project, user, environment: environment, dashboard_path: dashboard_path) }
 
     it_behaves_like 'misconfigured dashboard service response', :not_found
 
@@ -45,19 +45,19 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
     end
 
     context 'when no dashboard is specified' do
-      let(:service_call) { described_class.find(project, user, environment) }
+      let(:service_call) { described_class.find(project, user, environment: environment) }
 
       it_behaves_like 'valid dashboard service response'
     end
 
     context 'when the dashboard is expected to be embedded' do
-      let(:service_call) { described_class.find(project, user, environment, **params) }
-      let(:params) { { embedded: true } }
+      let(:service_call) { described_class.find(project, user, **params) }
+      let(:params) { { environment: environment, embedded: true } }
 
       it_behaves_like 'valid embedded dashboard service response'
 
       context 'when params are incomplete' do
-        let(:params) { { embedded: true, dashboard_path: system_dashboard_path } }
+        let(:params) { { environment: environment, embedded: true, dashboard_path: system_dashboard_path } }
 
         it_behaves_like 'valid embedded dashboard service response'
       end
@@ -65,11 +65,14 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
       context 'when the panel is specified' do
         context 'as a custom metric' do
           let(:params) do
-            { embedded: true,
+            {
+              environment: environment,
+              embedded: true,
               dashboard_path: system_dashboard_path,
               group: business_metric_title,
               title: 'title',
-              y_label: 'y_label' }
+              y_label: 'y_label'
+            }
           end
 
           it_behaves_like 'misconfigured dashboard service response', :not_found
@@ -86,11 +89,14 @@ describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store_cachi
         context 'as a project-defined panel' do
           let(:dashboard_path) { '.gitlab/dashboard/test.yml' }
           let(:params) do
-            { embedded: true,
+            {
+              environment: environment,
+              embedded: true,
               dashboard_path: dashboard_path,
               group: 'Group A',
               title: 'Super Chart A1',
-              y_label: 'y_label' }
+              y_label: 'y_label'
+            }
           end
 
           it_behaves_like 'misconfigured dashboard service response', :not_found

spec/lib/gitlab/metrics/dashboard/processor_spec.rb

@@ -8,8 +8,16 @@ describe Gitlab::Metrics::Dashboard::Processor do
   let(:dashboard_yml) { YAML.load_file('spec/fixtures/lib/gitlab/metrics/dashboard/sample_dashboard.yml') }
 
   describe 'process' do
-    let(:process_params) { [project, dashboard_yml, { environment: environment }] }
-    let(:dashboard) { described_class.new(*process_params).process(insert_project_metrics: true) }
+    let(:sequence) do
+      [
+        Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter,
+        Gitlab::Metrics::Dashboard::Stages::ProjectMetricsInserter,
+        Gitlab::Metrics::Dashboard::Stages::EndpointInserter,
+        Gitlab::Metrics::Dashboard::Stages::Sorter
+      ]
+    end
+    let(:process_params) { [project, dashboard_yml, sequence, { environment: environment }] }
+    let(:dashboard) { described_class.new(*process_params).process }
 
     it 'includes a path for the prometheus endpoint with each metric' do
       expect(all_metrics).to satisfy_all do |metric|
@@ -54,7 +62,14 @@ describe Gitlab::Metrics::Dashboard::Processor do
       end
 
       context 'when the dashboard should not include project metrics' do
-        let(:dashboard) { described_class.new(*process_params).process(insert_project_metrics: false) }
+        let(:sequence) do
+          [
+            Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter,
+            Gitlab::Metrics::Dashboard::Stages::EndpointInserter,
+            Gitlab::Metrics::Dashboard::Stages::Sorter
+          ]
+        end
+        let(:dashboard) { described_class.new(*process_params).process }
 
         it 'includes only dashboard metrics' do
           metrics = all_metrics.map { |m| m[:id] }