Test Additional metrics parser and fix query checking tests

Author: pawelchcki

app/models/project_services/prometheus_service.rb

@@ -75,10 +75,6 @@ def matched_metrics
     with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, &:itself)
   end
 
-  def with_reactive_cache(*args, &block)
-    yield calculate_reactive_cache(*args)
-  end
-
   # Cache metrics for specific environment
   def calculate_reactive_cache(query_class_name, *args)
     return unless active? && project && !project.pending_delete?

lib/gitlab/prometheus/additional_metrics_parser.rb

@@ -14,22 +14,29 @@ def metrics_from_list(list)
       end
 
       def metric_from_entry(entry)
-        missing_fields = [:title, :required_metrics, :weight, :queries].select { |key| !entry.key?(key) }
+        required_fields = [:title, :required_metrics, :weight, :queries]
+        missing_fields = required_fields.select { |key| entry[key].nil? }
         raise ParsingError.new("entry missing required fields #{missing_fields}") unless missing_fields.empty?
 
         Metric.new(entry[:title], entry[:required_metrics], entry[:weight], entry[:y_label], entry[:queries])
       end
 
       def group_from_entry(entry)
-        missing_fields = [:group, :priority, :metrics].select { |key| !entry.key?(key) }
-        raise ParsingError.new("entry missing required fields #{missing_fields}") unless missing_fields.empty?
+        required_fields = [:group, :priority, :metrics]
+        missing_fields = required_fields.select { |key| entry[key].nil? }
+
+        raise ParsingError.new("entry missing required fields #{missing_fields.map(&:to_s)}") unless missing_fields.empty?
 
         group = MetricGroup.new(entry[:group], entry[:priority])
         group.tap { |g| g.metrics = metrics_from_list(entry[:metrics]) }
       end
 
       def additional_metrics_raw
-        @additional_metrics_raw ||= YAML.load_file(Rails.root.join('config/additional_metrics.yml'))&.map(&:deep_symbolize_keys).freeze
+        @additional_metrics_raw ||= load_yaml_file&.map(&:deep_symbolize_keys).freeze
+      end
+
+      def load_yaml_file
+        YAML.load_file(Rails.root.join('config/additional_metrics.yml'))
       end
     end
   end

lib/gitlab/prometheus/queries/query_additional_metrics.rb

@@ -37,18 +37,19 @@ def group_with_any_metrics(group)
 
         def query_with_result(query)
           query[:result]&.any? do |item|
-            item&.[]('values')&.any? || item&.[]('value')&.any?
+            item&.[](:values)&.any? || item&.[](:value)&.any?
           end
         end
 
         def process_query(context, query)
           query_with_result = query.dup
-          query_with_result[:result] =
+          result =
             if query.key?(:query_range)
               client_query_range(query[:query_range] % context, start: context[:timeframe_start], stop: context[:timeframe_end])
             else
               client_query(query[:query] % context, time: context[:timeframe_end])
             end
+          query_with_result[:result] = result&.map(&:deep_symbolize_keys)
           query_with_result
         end
 

spec/lib/gitlab/prometheus/additional_metrics_parser_spec.rb

@@ -0,0 +1,250 @@
+require 'spec_helper'
+
+describe Gitlab::Prometheus::AdditionalMetricsParser, lib: true do
+  include Prometheus::MetricBuilders
+
+  let(:parser_error_class) { Gitlab::Prometheus::ParsingError }
+
+  describe '#load_groups_from_yaml' do
+    subject { described_class.load_groups_from_yaml }
+
+    describe 'parsing sample yaml' do
+      let(:sample_yaml) do
+        <<-EOF.strip_heredoc
+          - group: group_a
+            priority: 1
+            metrics:
+              - title: "title"
+                required_metrics: [ metric_a, metric_b ]
+                weight: 1
+                queries: [{ query_range: 'query_range_a', label: label, unit: unit }]
+              - title: "title"
+                required_metrics: [metric_a]
+                weight: 1
+                queries: [{ query_range: 'query_range_empty' }]
+          - group: group_b
+            priority: 1
+            metrics: 
+              - title: title
+                required_metrics: []
+                weight: 1
+                queries: [{query_range: query_range_a}]
+        EOF
+      end
+
+      before do
+        described_class.instance_variable_set :@additional_metrics_raw, nil
+        allow(described_class).to receive(:load_yaml_file) { YAML.load(sample_yaml) }
+      end
+
+      it 'parses to two metric groups with 2 and 1 metric respectively' do
+        expect(subject.count).to eq(2)
+        expect(subject[0].metrics.count).to eq(2)
+        expect(subject[1].metrics.count).to eq(1)
+      end
+
+      it 'provide group data' do
+        expect(subject[0]).to have_attributes(name: 'group_a', priority: 1)
+        expect(subject[1]).to have_attributes(name: 'group_b', priority: 1)
+      end
+
+      it 'provides metrics data' do
+        metrics = subject.flat_map(&:metrics)
+
+        expect(metrics.count).to eq(3)
+        expect(metrics[0]).to have_attributes(title: 'title', required_metrics: %w(metric_a metric_b), weight: 1)
+        expect(metrics[1]).to have_attributes(title: 'title', required_metrics: %w(metric_a), weight: 1)
+        expect(metrics[2]).to have_attributes(title: 'title', required_metrics: [], weight: 1)
+      end
+
+      it 'provides query data' do
+        queries = subject.flat_map(&:metrics).flat_map(&:queries)
+
+        expect(queries.count).to eq(3)
+        expect(queries[0]).to eq(query_range: 'query_range_a', label: 'label', unit: 'unit')
+        expect(queries[1]).to eq(query_range: 'query_range_empty')
+        expect(queries[2]).to eq(query_range: 'query_range_a')
+      end
+    end
+
+    shared_examples 'required field' do |field_name|
+      before do
+        described_class.instance_variable_set :@additional_metrics_raw, nil
+      end
+
+      context "when #{field_name} is nil" do
+        before do
+          allow(described_class).to receive(:load_yaml_file) { YAML.load(field_missing) }
+        end
+
+        it 'throws parsing error' do
+          expect { subject }.to raise_error(parser_error_class, /missing.*#{field_name}/)
+        end
+      end
+
+      context "when #{field_name} are not specified" do
+        before do
+          allow(described_class).to receive(:load_yaml_file) { YAML.load(field_nil) }
+        end
+
+        it 'throws parsing error' do
+          expect { subject }.to raise_error(parser_error_class, /missing.*#{field_name}/)
+        end
+      end
+    end
+
+    describe 'group required fields' do
+      it_behaves_like 'required field', :metrics do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+          EOF
+        end
+      end
+
+      it_behaves_like 'required field', :group do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - priority: 1
+              metrics: []
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - priority: 1
+              metrics: []
+          EOF
+        end
+      end
+
+      it_behaves_like 'required field', :priority do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority:
+              metrics: []
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              metrics: []
+          EOF
+        end
+      end
+    end
+
+    describe 'metrics fields parsing' do
+      it_behaves_like 'required field', :title do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: 
+                required_metrics: []
+                weight: 1
+                queries: []
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - required_metrics: []
+                weight: 1
+                queries: []
+          EOF
+        end
+      end
+
+      it_behaves_like 'required field', :required_metrics do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                required_metrics:
+                weight: 1
+                queries: []
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                weight: 1
+                queries: []
+          EOF
+        end
+      end
+
+      it_behaves_like 'required field', :weight do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                required_metrics: []
+                weight:
+                queries: []
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                required_metrics: []
+                queries: []
+          EOF
+        end
+      end
+
+      it_behaves_like 'required field', :queries do
+        let(:field_nil) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                required_metrics: []
+                weight: 1
+                queries:
+          EOF
+        end
+
+        let(:field_missing) do
+          <<-EOF.strip_heredoc
+            - group: group_a
+              priority: 1
+              metrics:
+              - title: title
+                required_metrics: []
+                weight: 1
+          EOF
+        end
+      end
+    end
+  end
+end

spec/support/prometheus/metric_builders.rb

@@ -21,7 +21,7 @@ def simple_metrics(added_metric_name: 'metric_a')
     end
 
     def simple_metric_group(name: 'name', metrics: simple_metrics)
-      Gitlab::Prometheus::MetricGroup.new(name: name, priority: 1, metrics: metrics)
+      Gitlab::Prometheus::MetricGroup.new( name, 1, metrics)
     end
   end
 end