Merge branch 'sy-grafana-default-panel' into 'master'

Default to sensible panel for grafana embeds

See merge request gitlab-org/gitlab!21932

app/services/metrics/dashboard/grafana_metric_embed_service.rb

@@ -138,7 +138,9 @@ module Metrics
     end
 
     # Identifies the name of the datasource for a dashboard
-    # based on the panelId query parameter found in the url
+    # based on the panelId query parameter found in the url.
+    #
+    # If no panel is specified, defaults to the first valid panel.
     class DatasourceNameParser
       def initialize(grafana_url, grafana_dashboard)
         @grafana_url, @grafana_dashboard = grafana_url, grafana_dashboard
@@ -146,15 +148,29 @@ module Metrics
 
       def parse
         @grafana_dashboard[:dashboard][:panels]
-          .find { |panel| panel[:id].to_s == query_params[:panelId] }
+          .find { |panel| panel_id ? matching_panel?(panel) : valid_panel?(panel) }
           .try(:[], :datasource)
       end
 
       private
 
+      def panel_id
+        query_params[:panelId]
+      end
+
       def query_params
         Gitlab::Metrics::Dashboard::Url.parse_query(@grafana_url)
       end
+
+      def matching_panel?(panel)
+        panel[:id].to_s == panel_id
+      end
+
+      def valid_panel?(panel)
+        ::Grafana::Validator
+          .new(@grafana_dashboard, nil, panel, query_params)
+          .valid?
+      end
     end
   end
 end

changelogs/unreleased/sy-grafana-default-panel.yml

+---
+title: Default to first valid panel in unspecified Grafana embeds
+merge_request: 21932
+author:
+type: changed

doc/user/project/integrations/prometheus.md

@@ -820,7 +820,7 @@ Prerequisites for embedding from a Grafana instance:
    ![Grafana Metric Panel](img/grafana_panel_v12_5.png)
 1. In the upper-left corner of the page, select a specific value for each variable required for the queries in the chart.
    ![Select Query Variables](img/select_query_variables_v12_5.png)
-1. In Grafana, click on a panel's title, then click **Share** to open the panel's sharing dialog to the **Link** tab.
+1. In Grafana, click on a panel's title, then click **Share** to open the panel's sharing dialog to the **Link** tab. If you click the _dashboard's_ share panel instead, GitLab will attempt to embed the first supported panel on the dashboard (if available).
 1. If your Prometheus queries use Grafana's custom template variables, ensure that "Template variables" option is toggled to **On**. Of Grafana global template variables, only `$__interval`, `$__from`, and `$__to` are currently supported. Toggle **On** the "Current time range" option to specify the time range of the chart. Otherwise, the default range will be the last 8 hours.
    ![Grafana Sharing Dialog](img/grafana_sharing_dialog_v12_5.png)
 1. Click **Copy** to copy the URL to the clipboard.

lib/banzai/filter/inline_grafana_metrics_filter.rb

@@ -17,8 +17,6 @@ module Banzai
       def embed_params(node)
         query_params = Gitlab::Metrics::Dashboard::Url.parse_query(node['href'])
 
-        return unless query_params.include?(:panelId)
-
         time_window = Grafana::TimeWindow.new(query_params[:from], query_params[:to])
         url = url_with_window(node['href'], query_params, time_window.in_milliseconds)
 

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

@@ -13,12 +13,7 @@ module Gitlab
           # Reformats the specified panel in the Gitlab
           # dashboard-yml format
           def transform!
-            InputFormatValidator.new(
-              grafana_dashboard,
-              datasource,
-              panel,
-              query_params
-            ).validate!
+            validate_input!
 
             new_dashboard = formatted_dashboard
 
@@ -28,6 +23,17 @@ module Gitlab
 
           private
 
+          def validate_input!
+            ::Grafana::Validator.new(
+              grafana_dashboard,
+              datasource,
+              panel,
+              query_params
+            ).validate!
+          rescue ::Grafana::Validator::Error => e
+            raise ::Gitlab::Metrics::Dashboard::Errors::DashboardProcessingError, e.message
+          end
+
           def formatted_dashboard
             { panel_groups: [{ panels: [formatted_panel] }] }
           end
@@ -56,11 +62,25 @@ module Gitlab
           def panel
             strong_memoize(:panel) do
               grafana_dashboard[:dashboard][:panels].find do |panel|
-                panel[:id].to_s == query_params[:panelId]
+                query_params[:panelId] ? matching_panel?(panel) : valid_panel?(panel)
               end
             end
           end
 
+          # Determines whether a given panel is the one
+          # specified by the linked grafana url
+          def matching_panel?(panel)
+            panel[:id].to_s == query_params[:panelId]
+          end
+
+          # Determines whether any given panel has the potenial
+          # to return valid results from grafana/prometheus
+          def valid_panel?(panel)
+            ::Grafana::Validator
+              .new(grafana_dashboard, datasource, panel, query_params)
+              .valid?
+          end
+
           # Grafana url query parameters. Includes information
           # on which panel to select and time range.
           def query_params
@@ -141,83 +161,6 @@ module Gitlab
             params[:grafana_url]
           end
         end
-
-        class InputFormatValidator
-          include ::Gitlab::Metrics::Dashboard::Errors
-
-          attr_reader :grafana_dashboard, :datasource, :panel, :query_params
-
-          UNSUPPORTED_GRAFANA_GLOBAL_VARS = %w(
-            $__interval_ms
-            $__timeFilter
-            $__name
-            $timeFilter
-            $interval
-          ).freeze
-
-          def initialize(grafana_dashboard, datasource, panel, query_params)
-            @grafana_dashboard = grafana_dashboard
-            @datasource = datasource
-            @panel = panel
-            @query_params = query_params
-          end
-
-          def validate!
-            validate_query_params!
-            validate_datasource!
-            validate_panel_type!
-            validate_variable_definitions!
-            validate_global_variables!
-          end
-
-          private
-
-          def validate_datasource!
-            return if datasource[:access] == 'proxy' && datasource[:type] == 'prometheus'
-
-            raise_error 'Only Prometheus datasources with proxy access in Grafana are supported.'
-          end
-
-          def validate_query_params!
-            return if [:panelId, :from, :to].all? { |param| query_params.include?(param) }
-
-            raise_error 'Grafana query parameters must include panelId, from, and to.'
-          end
-
-          def validate_panel_type!
-            return if panel[:type] == 'graph' && panel[:lines]
-
-            raise_error 'Panel type must be a line graph.'
-          end
-
-          def validate_variable_definitions!
-            return unless grafana_dashboard[:dashboard][:templating]
-
-            return if grafana_dashboard[:dashboard][:templating][:list].all? do |variable|
-              query_params[:"var-#{variable[:name]}"].present?
-            end
-
-            raise_error 'All Grafana variables must be defined in the query parameters.'
-          end
-
-          def validate_global_variables!
-            return unless panel_contains_unsupported_vars?
-
-            raise_error 'Prometheus must not include'
-          end
-
-          def panel_contains_unsupported_vars?
-            panel[:targets].any? do |target|
-              UNSUPPORTED_GRAFANA_GLOBAL_VARS.any? do |variable|
-                target[:expr].include?(variable)
-              end
-            end
-          end
-
-          def raise_error(message)
-            raise DashboardProcessingError.new(message)
-          end
-        end
       end
     end
   end

lib/grafana/validator.rb

+# frozen_string_literal: true
+
+# Performs checks on whether resources from Grafana can be handled
+# We have certain restrictions on which formats we accept.
+# Some are technical requirements, others are simplifications.
+module Grafana
+  class Validator
+    Error = Class.new(StandardError)
+
+    attr_reader :grafana_dashboard, :datasource, :panel, :query_params
+
+    UNSUPPORTED_GRAFANA_GLOBAL_VARS = %w(
+      $__interval_ms
+      $__timeFilter
+      $__name
+      $timeFilter
+      $interval
+    ).freeze
+
+    def initialize(grafana_dashboard, datasource, panel, query_params)
+      @grafana_dashboard = grafana_dashboard
+      @datasource = datasource
+      @panel = panel
+      @query_params = query_params
+    end
+
+    def validate!
+      validate_query_params!
+      validate_panel_type!
+      validate_variable_definitions!
+      validate_global_variables!
+      validate_datasource! if datasource
+    end
+
+    def valid?
+      validate!
+
+      true
+    rescue ::Grafana::Validator::Error
+      false
+    end
+
+    private
+
+    # See defaults in Banzai::Filter::InlineGrafanaMetricsFilter.
+    def validate_query_params!
+      return if [:from, :to].all? { |param| query_params.include?(param) }
+
+      raise_error 'Grafana query parameters must include from and to.'
+    end
+
+    # We may choose to support other panel types in future.
+    def validate_panel_type!
+      return if panel && panel[:type] == 'graph' && panel[:lines]
+
+      raise_error 'Panel type must be a line graph.'
+    end
+
+    # We must require variable definitions to create valid prometheus queries.
+    def validate_variable_definitions!
+      return unless grafana_dashboard[:dashboard][:templating]
+
+      return if grafana_dashboard[:dashboard][:templating][:list].all? do |variable|
+        query_params[:"var-#{variable[:name]}"].present?
+      end
+
+      raise_error 'All Grafana variables must be defined in the query parameters.'
+    end
+
+    # We may choose to support further Grafana variables in future.
+    def validate_global_variables!
+      return unless panel_contains_unsupported_vars?
+
+      raise_error "Prometheus must not include #{UNSUPPORTED_GRAFANA_GLOBAL_VARS}"
+    end
+
+    # We may choose to support additional datasources in future.
+    def validate_datasource!
+      return if datasource[:access] == 'proxy' && datasource[:type] == 'prometheus'
+
+      raise_error 'Only Prometheus datasources with proxy access in Grafana are supported.'
+    end
+
+    def panel_contains_unsupported_vars?
+      panel[:targets].any? do |target|
+        UNSUPPORTED_GRAFANA_GLOBAL_VARS.any? do |variable|
+          target[:expr].include?(variable)
+        end
+      end
+    end
+
+    def raise_error(message)
+      raise Validator::Error, message
+    end
+  end
+end

spec/lib/banzai/filter/inline_grafana_metrics_filter_spec.rb

@@ -46,11 +46,9 @@ describe Banzai::Filter::InlineGrafanaMetricsFilter do
   end
 
   context 'when "panelId" parameter is missing' do
-    let(:dashboard_path) { '/d/XDaNK6amz/gitlab-omnibus-redis' }
+    let(:dashboard_path) { '/d/XDaNK6amz/gitlab-omnibus-redis?from=1570397739557&to=1570484139557' }
 
-    it 'leaves the markdown unchanged' do
-      expect(unescape(doc.to_s)).to eq(input)
-    end
+    it_behaves_like 'a metrics embed filter'
   end
 
   context 'when time window parameters are missing' do
@@ -86,6 +84,14 @@ describe Banzai::Filter::InlineGrafanaMetricsFilter do
     end
   end
 
+  context 'when no parameters are provided' do
+    let(:dashboard_path) { '/d/XDaNK6amz/gitlab-omnibus-redis' }
+
+    it 'inserts a placeholder' do
+      expect(embed_url).to be_present
+    end
+  end
+
   private
 
   # Nokogiri escapes the URLs, but we don't care about that

spec/lib/gitlab/metrics/dashboard/stages/grafana_formatter_spec.rb

@@ -11,8 +11,9 @@ describe Gitlab::Metrics::Dashboard::Stages::GrafanaFormatter do
   describe '#transform!' do
     let(:grafana_dashboard) { JSON.parse(fixture_file('grafana/simplified_dashboard_response.json'), symbolize_names: true) }
     let(:datasource) { JSON.parse(fixture_file('grafana/datasource_response.json'), symbolize_names: true) }
+    let(:expected_dashboard) { JSON.parse(fixture_file('grafana/expected_grafana_embed.json'), symbolize_names: true) }
 
-    let(:dashboard) { described_class.new(project, {}, params).transform! }
+    subject(:dashboard) { described_class.new(project, {}, params).transform! }
 
     let(:params) do
       {
@@ -23,83 +24,34 @@ describe Gitlab::Metrics::Dashboard::Stages::GrafanaFormatter do
     end
 
     context 'when the query and resources are configured correctly' do
-      let(:expected_dashboard) { JSON.parse(fixture_file('grafana/expected_grafana_embed.json'), symbolize_names: true) }
-
-      it 'generates a gitlab-yml formatted dashboard' do
-        expect(dashboard).to eq(expected_dashboard)
-      end
+      it { is_expected.to eq expected_dashboard }
     end
 
-    context 'when the inputs are invalid' do
-      shared_examples_for 'processing error' do
-        it 'raises a processing error' do
-          expect { dashboard }
-            .to raise_error(Gitlab::Metrics::Dashboard::Stages::InputFormatValidator::DashboardProcessingError)
-        end
-      end
-
-      context 'when the datasource is not proxyable' do
-        before do
-          params[:datasource][:access] = 'not-proxy'
-        end
-
-        it_behaves_like 'processing error'
+    context 'when a panelId is not included in the grafana_url' do
+      before do
+        params[:grafana_url].gsub('&panelId=8', '')
       end
 
-      context 'when query param "panelId" is not specified' do
-        before do
-          params[:grafana_url].gsub!('panelId=8', '')
-        end
-
-        it_behaves_like 'processing error'
-      end
-
-      context 'when query param "from" is not specified' do
-        before do
-          params[:grafana_url].gsub!('from=1570397739557', '')
-        end
-
-        it_behaves_like 'processing error'
-      end
+      it { is_expected.to eq expected_dashboard }
 
-      context 'when query param "to" is not specified' do
+      context 'when there is also no valid panel in the dashboard' do
         before do
-          params[:grafana_url].gsub!('to=1570484139557', '')
+          params[:grafana_dashboard][:dashboard][:panels] = []
         end
 
-        it_behaves_like 'processing error'
-      end
-
-      context 'when the panel is not a graph' do
-        before do
-          params[:grafana_dashboard][:dashboard][:panels][0][:type] = 'singlestat'
+        it 'raises a processing error' do
+          expect { dashboard }.to raise_error(::Gitlab::Metrics::Dashboard::Errors::DashboardProcessingError)
         end
-
-        it_behaves_like 'processing error'
       end
+    end
 
-      context 'when the panel is not a line graph' do
-        before do
-          params[:grafana_dashboard][:dashboard][:panels][0][:lines] = false
-        end
-
-        it_behaves_like 'processing error'
-      end
-
-      context 'when the query dashboard includes undefined variables' do
-        before do
-          params[:grafana_url].gsub!('&var-instance=localhost:9121', '')
-        end
-
-        it_behaves_like 'processing error'
+    context 'when an input is invalid' do
+      before do
+        params[:datasource][:access] = 'not-proxy'
       end
 
-      context 'when the expression contains unsupported global variables' do
-        before do
-          params[:grafana_dashboard][:dashboard][:panels][0][:targets][0][:expr] = 'sum(important_metric[$__interval_ms])'
-        end
-
-        it_behaves_like 'processing error'
+      it 'raises a processing error' do
+        expect { dashboard }.to raise_error(::Gitlab::Metrics::Dashboard::Errors::DashboardProcessingError)
       end
     end
   end

spec/lib/grafana/validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Grafana::Validator do
+  let(:grafana_dashboard) { JSON.parse(fixture_file('grafana/simplified_dashboard_response.json'), symbolize_names: true) }
+  let(:datasource) { JSON.parse(fixture_file('grafana/datasource_response.json'), symbolize_names: true) }
+  let(:panel) { grafana_dashboard[:dashboard][:panels].first }
+
+  let(:query_params) do
+    {
+      from: '1570397739557',
+      to: '1570484139557',
+      panelId: '8',
+      'var-instance': 'localhost:9121'
+    }
+  end
+
+  describe 'validate!' do
+    shared_examples_for 'processing error' do |message|
+      it 'raises a processing error' do
+        expect { subject }
+          .to raise_error(::Grafana::Validator::Error, message)
+      end
+    end
+
+    subject { described_class.new(grafana_dashboard, datasource, panel, query_params).validate! }
+
+    it 'does not raise an error' do
+      expect { subject }.not_to raise_error
+    end
+
+    context 'when query param "from" is not specified' do
+      before do
+        query_params.delete(:from)
+      end
+
+      it_behaves_like 'processing error', 'Grafana query parameters must include from and to.'
+    end
+
+    context 'when query param "to" is not specified' do
+      before do
+        query_params.delete(:to)
+      end
+
+      it_behaves_like 'processing error', 'Grafana query parameters must include from and to.'
+    end
+
+    context 'when the panel is not provided' do
+      let(:panel) { nil }
+
+      it_behaves_like 'processing error', 'Panel type must be a line graph.'
+    end
+
+    context 'when the panel is not a graph' do
+      before do
+        panel[:type] = 'singlestat'
+      end
+
+      it_behaves_like 'processing error', 'Panel type must be a line graph.'
+    end
+
+    context 'when the panel is not a line graph' do
+      before do
+        panel[:lines] = false
+      end
+
+      it_behaves_like 'processing error', 'Panel type must be a line graph.'
+    end
+
+    context 'when the query dashboard includes undefined variables' do
+      before do
+        query_params.delete(:'var-instance')
+      end
+
+      it_behaves_like 'processing error', 'All Grafana variables must be defined in the query parameters.'
+    end
+
+    context 'when the expression contains unsupported global variables' do
+      before do
+        grafana_dashboard[:dashboard][:panels][0][:targets][0][:expr] = 'sum(important_metric[$__interval_ms])'
+      end
+
+      it_behaves_like 'processing error', "Prometheus must not include #{described_class::UNSUPPORTED_GRAFANA_GLOBAL_VARS}"
+    end
+
+    context 'when the datasource is not proxyable' do
+      before do
+        datasource[:access] = 'not-proxy'
+      end
+
+      it_behaves_like 'processing error', 'Only Prometheus datasources with proxy access in Grafana are supported.'
+    end
+
+    # Skipping datasource validation allows for checks to be
+    # run without a secondary call to Grafana API
+    context 'when the datasource is not provided' do
+      let(:datasource) { nil }
+
+      it 'does not raise an error' do
+        expect { subject }.not_to raise_error
+      end
+    end
+  end
+
+  describe 'valid?' do
+    subject { described_class.new(grafana_dashboard, datasource, panel, query_params).valid? }
+
+    context 'with valid arguments' do
+      it { is_expected.to be true }
+    end
+
+    context 'with invalid arguments' do
+      let(:query_params) { {} }
+
+      it { is_expected.to be false }
+    end
+  end
+end