Merge branch...

Merge branch 'mwaw/219398-metrics-extend-dashboard-yaml-definion-validation-when-viewing-the-file-to-exhaustive-list' into 'master'

Improve errors on metrics dashboard config viewer

See merge request gitlab-org/gitlab!38925

app/graphql/resolvers/metrics/dashboard_resolver.rb

@@ -14,7 +14,8 @@ module Resolvers
       def resolve(**args)
         return unless environment
 
-        ::PerformanceMonitoring::PrometheusDashboard.find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment })
+        ::PerformanceMonitoring::PrometheusDashboard
+          .find_for(project: environment.project, user: context[:current_user], path: args[:path], options: { environment: environment })
       end
     end
   end

app/graphql/types/metrics/dashboard_type.rb

@@ -16,6 +16,13 @@ module Types
       field :annotations, Types::Metrics::Dashboards::AnnotationType.connection_type, null: true,
             description: 'Annotations added to the dashboard',
             resolver: Resolvers::Metrics::Dashboards::AnnotationResolver
+
+      # In order to maintain backward compatibility we need to return NULL when there are no warnings
+      # and dashboard validation returns an empty array when there are no issues.
+      def schema_validation_warnings
+        warnings = object.schema_validation_warnings
+        warnings unless warnings.empty?
+      end
     end
     # rubocop: enable Graphql/AuthorizeTypes
   end

app/models/blob_viewer/metrics_dashboard_yml.rb

@@ -26,19 +26,10 @@ module BlobViewer
 
     def parse_blob_data
       yaml = ::Gitlab::Config::Loader::Yaml.new(blob.data).load_raw!
-
-      ::PerformanceMonitoring::PrometheusDashboard.from_json(yaml)
-      nil
+      Gitlab::Metrics::Dashboard::Validator
+        .errors(yaml, dashboard_path: blob.path, project: project)
     rescue Gitlab::Config::Loader::FormatError => error
-      wrap_yml_syntax_error(error)
-    rescue ActiveModel::ValidationError => invalid
-      invalid.model.errors
-    end
-
-    def wrap_yml_syntax_error(error)
-      ::PerformanceMonitoring::PrometheusDashboard.new.errors.tap do |errors|
-        errors.add(:'YAML syntax', error.message)
-      end
+      [error]
     end
   end
 end

app/models/performance_monitoring/prometheus_dashboard.rb

@@ -53,14 +53,18 @@ module PerformanceMonitoring
     # This method is planned to be refactored as a part of https://gitlab.com/gitlab-org/gitlab/-/issues/219398
     # implementation. For new existing logic was reused to faster deliver MVC
     def schema_validation_warnings
-      self.class.from_json(reload_schema)
-      nil
-    rescue ActiveModel::ValidationError => exception
-      exception.model.errors.map { |attr, error| "#{attr}: #{error}" }
+      run_custom_validation.map(&:message)
     end
 
     private
 
+    def run_custom_validation
+      Gitlab::Metrics::Dashboard::Validator
+        .errors(reload_schema, dashboard_path: path, project: environment&.project)
+    rescue Gitlab::Config::Loader::FormatError => error
+      [error.message]
+    end
+
     # dashboard finder methods are somehow limited, #find includes checking if
     # user is authorised to view selected dashboard, but modifies schema, which in some cases may
     # cause false positives returned from validation, and #find_raw does not authorise users

app/views/projects/blob/viewers/_metrics_dashboard_yml.html.haml

@@ -5,7 +5,7 @@
   = icon('warning fw')
   = _('Metrics Dashboard YAML definition is invalid:')
   %ul
-    - viewer.errors.messages.each do |error|
-      %li= error.join(': ')
+    - viewer.errors.each do |error|
+      %li= error
 
 = link_to _('Learn more'), help_page_path('operations/metrics/dashboards/index.md')

changelogs/unreleased/mwaw-219398-metrics-extend-dashboard-yaml-definion-validation-when-viewin.yml

+---
+title: Change metrics dashboard schema validation messages into exhaustive list of
+  all encountered errors.
+merge_request: 38925
+author:
+type: changed

doc/operations/metrics/dashboards/yaml.md

@@ -156,21 +156,46 @@ and files with invalid syntax display **Metrics Dashboard YAML definition is inv
 
 When **Metrics Dashboard YAML definition is invalid** at least one of the following messages is displayed:
 
-1. `dashboard: can't be blank` [learn more](#dashboard-top-level-properties)
-1. `panel_groups: should be an array of panel_groups objects` [learn more](#dashboard-top-level-properties)
-1. `group: can't be blank` [learn more](#panel-group-panel_groups-properties)
-1. `panels: should be an array of panels objects` [learn more](#panel-group-panel_groups-properties)
-1. `title: can't be blank` [learn more](#panel-panels-properties)
-1. `metrics: should be an array of metrics objects` [learn more](#panel-panels-properties)
-1. `query: can't be blank` [learn more](#metrics-metrics-properties)
-1. `query_range: can't be blank` [learn more](#metrics-metrics-properties)
-1. `unit: can't be blank` [learn more](#metrics-metrics-properties)
-1. `YAML syntax: The parsed YAML is too big`
-
-   This is displayed when the YAML file is larger than 1 MB.
-
-1. `YAML syntax: Invalid configuration format`
-
-   This is displayed when the YAML file is empty or does not contain valid YAML.
+1. `[location] is missing required keys: [list of missing keys]` - The entry at
+   `[location]` is missing a key, or a key has been mistyped. This
+   example returns the error `root is missing required keys: panel_groups`:
+
+   ```yaml
+   dashboard: Important metrics
+   group_panels:
+     - ...
+   ```
+
+1. `[data] at [location] is not of type: [type]` - The entry at `[location]` contains
+   `[data]` which type does not adhere to required types. This example returns the
+   error `'123' at /panel_groups/0/group is not of type: string`:
+
+   ```yaml
+   dashboard: Environment metrics
+   panel_groups:
+   - group: 123
+     panels:
+     ...
+   ```
+
+1. `[data] at [location] is not one of: [types]` - The entry at `[location]` contains
+   `[data]` which is not included in the list of required values. This example returns
+   the error `'scatterplot-chart' at /panel_groups/0/panels/0/type is not one of: ["area-chart", "line-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap"]`:
+
+   ```yaml
+   dashboard: Environment metrics
+   panel_groups:
+   - group: Network
+     panels:
+     - title: Throughput
+       type: scatterplot-chart
+       y_label: Requests / Sec
+     ...
+   ```
+
+1. `metric_id must be unique across a project` - At least two metrics entries have
+   the same `id` attribute, which [must be unique](#metrics-metrics-properties).
+1. `The parsed YAML is too big` - The YAML file is larger than 1 MB.
+1. `Invalid configuration format` - The YAML file is empty or does not contain valid YAML.
 
 Metrics Dashboard YAML definition validation information is also available as a [GraphQL API field](../../../api/graphql/reference/index.md#metricsdashboard)

lib/gitlab/metrics/dashboard/validator.rb

@@ -8,20 +8,18 @@ module Gitlab
 
         class << self
           def validate(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
-            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
-            errors.empty?
+            errors(content, schema_path, dashboard_path: dashboard_path, project: project).empty?
           end
 
           def validate!(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
-            errors = _validate(content, schema_path, dashboard_path: dashboard_path, project: project)
+            errors = errors(content, schema_path, dashboard_path: dashboard_path, project: project)
             errors.empty? || raise(errors.first)
           end
 
-          private
-
-          def _validate(content, schema_path, dashboard_path: nil, project: nil)
-            client = Validator::Client.new(content, schema_path, dashboard_path: dashboard_path, project: project)
-            client.execute
+          def errors(content, schema_path = DASHBOARD_SCHEMA_PATH, dashboard_path: nil, project: nil)
+            Validator::Client
+              .new(content, schema_path, dashboard_path: dashboard_path, project: project)
+              .execute
           end
         end
       end

lib/gitlab/metrics/dashboard/validator/client.rb

@@ -46,7 +46,7 @@ module Gitlab
 
           def validate_against_schema
             schemer.validate(content).map do |error|
-              Errors::SchemaValidationError.new(error)
+              ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new(error)
             end
           end
         end

lib/gitlab/metrics/dashboard/validator/schemas/panel.json

@@ -4,7 +4,7 @@
   "properties": {
     "type": {
       "type": "string",
-      "enum": ["area-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap"],
+      "enum": ["area-chart", "line-chart", "anomaly-chart", "bar", "column", "stacked-column", "single-stat", "heatmap", "gauge"],
       "default": "area-chart"
     },
     "title": { "type": "string" },

spec/features/projects/blobs/blob_show_spec.rb

@@ -589,7 +589,7 @@ RSpec.describe 'File blob', :js do
         aggregate_failures do
           # shows that dashboard yaml is invalid
           expect(page).to have_content('Metrics Dashboard YAML definition is invalid:')
-          expect(page).to have_content("panel_groups: should be an array of panel_groups objects")
+          expect(page).to have_content("root is missing required keys: panel_groups")
 
           # shows a learn more link
           expect(page).to have_link('Learn more')

spec/lib/gitlab/metrics/dashboard/validator/errors_spec.rb

@@ -34,6 +34,19 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator::Errors do
 
           it { is_expected.to eq 'root is missing required keys: one' }
         end
+
+        context 'when there is type mismatch' do
+          %w(null string boolean integer number array object).each do |expected_type|
+            context "on type: #{expected_type}" do
+              let(:type) { expected_type }
+              let(:details) { nil }
+
+              subject { described_class.new(error_hash).message }
+
+              it { is_expected.to eq "'property_name' at root is not of type: #{expected_type}" }
+            end
+          end
+        end
       end
 
       context 'for nested object' do

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

@@ -143,4 +143,56 @@ RSpec.describe Gitlab::Metrics::Dashboard::Validator do
       end
     end
   end
+
+  describe '#errors' do
+    context 'valid dashboard schema' do
+      it 'returns no errors' do
+        expect(described_class.errors(valid_dashboard)).to eq []
+      end
+
+      context 'with duplicate metric_ids' do
+        it 'returns errors' do
+          expect(described_class.errors(duplicate_id_dashboard)).to eq [Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds.new]
+        end
+      end
+
+      context 'with dashboard_path and project' do
+        subject { described_class.errors(valid_dashboard, dashboard_path: 'test/path.yml', project: project) }
+
+        context 'with no conflicting metric identifiers in db' do
+          it { is_expected.to eq [] }
+        end
+
+        context 'with metric identifier present in current dashboard' do
+          before do
+            create(:prometheus_metric,
+                   identifier:     'metric_a1',
+                   dashboard_path: 'test/path.yml',
+                   project:        project
+                  )
+          end
+
+          it { is_expected.to eq [] }
+        end
+
+        context 'with metric identifier present in another dashboard' do
+          before do
+            create(:prometheus_metric,
+                   identifier:     'metric_a1',
+                   dashboard_path: 'some/other/dashboard/path.yml',
+                   project:        project
+                  )
+          end
+
+          it { is_expected.to eq [Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds.new] }
+        end
+      end
+    end
+
+    context 'invalid dashboard schema' do
+      it 'returns collection of validation errors' do
+        expect(described_class.errors(invalid_dashboard)).to all be_kind_of(Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError)
+      end
+    end
+  end
 end

spec/models/blob_viewer/metrics_dashboard_yml_spec.rb

@@ -13,11 +13,11 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
   subject(:viewer) { described_class.new(blob) }
 
   context 'when the definition is valid' do
-    let(:data) { File.read(Rails.root.join('config/prometheus/common_metrics.yml')) }
+    let(:data) { fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml')}
 
     describe '#valid?' do
       it 'calls prepare! on the viewer' do
-        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
+        allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors)
 
         expect(viewer).to receive(:prepare!)
 
@@ -30,46 +30,44 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
         expect_next_instance_of(::Gitlab::Config::Loader::Yaml, data) do |loader|
           expect(loader).to receive(:load_raw!).and_call_original
         end
-        expect(PerformanceMonitoring::PrometheusDashboard)
-          .to receive(:from_json)
-          .with(yml)
+        expect(Gitlab::Metrics::Dashboard::Validator)
+          .to receive(:errors)
+          .with(yml, dashboard_path: '.gitlab/dashboards/custom-dashboard.yml', project: project)
           .and_call_original
         expect(viewer.valid?).to be_truthy
       end
     end
 
     describe '#errors' do
-      it 'returns nil' do
-        allow(PerformanceMonitoring::PrometheusDashboard).to receive(:from_json)
+      it 'returns empty array' do
+        allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).and_return([])
 
-        expect(viewer.errors).to be nil
+        expect(viewer.errors).to eq []
       end
     end
   end
 
   context 'when definition is invalid' do
-    let(:error) { ActiveModel::ValidationError.new(PerformanceMonitoring::PrometheusDashboard.new.tap(&:validate)) }
+    let(:error) { ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new }
     let(:data) do
       <<~YAML
         dashboard:
       YAML
     end
 
+    before do
+      allow(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).and_return([error])
+    end
+
     describe '#valid?' do
       it 'returns false' do
-        expect(PerformanceMonitoring::PrometheusDashboard)
-          .to receive(:from_json).and_raise(error)
-
-        expect(viewer.valid?).to be_falsey
+        expect(viewer.valid?).to be false
       end
     end
 
     describe '#errors' do
       it 'returns validation errors' do
-        allow(PerformanceMonitoring::PrometheusDashboard)
-          .to receive(:from_json).and_raise(error)
-
-        expect(viewer.errors).to be error.model.errors
+        expect(viewer.errors).to eq [error]
       end
     end
   end
@@ -85,17 +83,14 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
 
     describe '#valid?' do
       it 'returns false' do
-        expect(PerformanceMonitoring::PrometheusDashboard).not_to receive(:from_json)
-        expect(viewer.valid?).to be_falsey
+        expect(Gitlab::Metrics::Dashboard::Validator).not_to receive(:errors)
+        expect(viewer.valid?).to be false
       end
     end
 
     describe '#errors' do
       it 'returns validation errors' do
-        yaml_wrapped_errors = { 'YAML syntax': ["(<unknown>): did not find expected key while parsing a block mapping at line 1 column 1"] }
-
-        expect(viewer.errors).to be_kind_of ActiveModel::Errors
-        expect(viewer.errors.messages).to eql(yaml_wrapped_errors)
+        expect(viewer.errors).to all be_kind_of Gitlab::Config::Loader::FormatError
       end
     end
   end
@@ -113,15 +108,12 @@ RSpec.describe BlobViewer::MetricsDashboardYml do
     end
 
     it 'is invalid' do
-      expect(PerformanceMonitoring::PrometheusDashboard).not_to receive(:from_json)
-      expect(viewer.valid?).to be(false)
+      expect(Gitlab::Metrics::Dashboard::Validator).not_to receive(:errors)
+      expect(viewer.valid?).to be false
     end
 
     it 'returns validation errors' do
-      yaml_wrapped_errors = { 'YAML syntax': ["The parsed YAML is too big"] }
-
-      expect(viewer.errors).to be_kind_of(ActiveModel::Errors)
-      expect(viewer.errors.messages).to eq(yaml_wrapped_errors)
+      expect(viewer.errors).to all be_kind_of Gitlab::Config::Loader::FormatError
     end
   end
 end

spec/models/performance_monitoring/prometheus_dashboard_spec.rb

@@ -219,20 +219,31 @@ RSpec.describe PerformanceMonitoring::PrometheusDashboard do
   end
 
   describe '#schema_validation_warnings' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:environment) { create(:environment, project: project) }
+    let(:path) { '.gitlab/dashboards/test.yml' }
+
+    subject(:schema_validation_warnings) { described_class.new(json_content.merge(path: path, environment: environment)).schema_validation_warnings }
+
+    before do
+      allow(Gitlab::Metrics::Dashboard::Finder).to receive(:find_raw).with(project, dashboard_path: path).and_return(json_content)
+    end
+
     context 'when schema is valid' do
       it 'returns nil' do
-        expect(described_class).to receive(:from_json)
-        expect(described_class.new.schema_validation_warnings).to be_nil
+        expect(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).with(json_content, dashboard_path: path, project: project).and_return([])
+
+        expect(schema_validation_warnings).to eq []
       end
     end
 
     context 'when schema is invalid' do
       it 'returns array with errors messages' do
-        instance = described_class.new
-        instance.errors.add(:test, 'test error')
+        error = ::Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError.new
+
+        expect(Gitlab::Metrics::Dashboard::Validator).to receive(:errors).with(json_content, dashboard_path: path, project: project).and_return([error])
 
-        expect(described_class).to receive(:from_json).and_raise(ActiveModel::ValidationError.new(instance))
-        expect(described_class.new.schema_validation_warnings).to eq ['test: test error']
+        expect(schema_validation_warnings).to eq [error.message]
       end
     end
   end

spec/requests/api/graphql/metrics/dashboard_query_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
 
   let_it_be(:current_user) { create(:user) }
   let(:project) { create(:project) }
-  let!(:environment) { create(:environment, project: project) }
+  let(:environment) { create(:environment, project: project) }
 
   let(:query) do
     graphql_query_for(
@@ -67,7 +67,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
         it 'returns metrics dashboard' do
           dashboard = graphql_data.dig('project', 'environments', 'nodes', 0, 'metricsDashboard')
 
-          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["panel_groups: should be an array of panel_groups objects"])
+          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["root is missing required keys: panel_groups"])
         end
       end
 
@@ -78,7 +78,7 @@ RSpec.describe 'Getting Metrics Dashboard' do
         it 'returns metrics dashboard' do
           dashboard = graphql_data.dig('project', 'environments', 'nodes', 0, 'metricsDashboard')
 
-          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["dashboard: can't be blank", "panel_groups: should be an array of panel_groups objects"])
+          expect(dashboard).to eql("path" => path, "schemaValidationWarnings" => ["root is missing required keys: dashboard, panel_groups"])
         end
       end
     end