Merge branch '227929-metrics-dashboard-embeds-with-new-urls' into 'master'

Make metrics dashboard embeds work with new and old URLs

See merge request gitlab-org/gitlab!39876

changelogs/unreleased/227929-metrics-dashboard-embeds-with-new-urls.yml

+---
+title: Fix Metrics dashboard embeds when using new URLs
+merge_request: 39876
+author:
+type: fixed

doc/operations/metrics/embed.md

@@ -20,15 +20,28 @@ metrics to others, and you want to have relevant information directly available.
 NOTE: **Note:**
 Requires [Kubernetes](../../user/project/integrations/prometheus_library/kubernetes.md) metrics.
 
+Note: **Note:**
+In GitLab versions 13.3 and earlier, metrics dashboard links were in the form
+`https://<root_url>/<project>/-/environments/<environment_id>/metrics`. These links
+are still supported, and can be used to embed metric charts.
+
 To display metric charts, include a link of the form
-`https://<root_url>/<project>/-/environments/<environment_id>/metrics` in a field
+`https://<root_url>/<project>/-/metrics?environment=<environment_id>` in a field
 that supports GitLab-flavored Markdown:
 
-![Embedded Metrics Markdown](img/embedded_metrics_markdown_v12_8.png)
+```markdown
+### Summary
+
+**Start time:** 2020-01-21T12:00:31+00:00
+
+### Metrics
+
+https://gitlab.com/gitlab-org/monitor/tanuki-inc/-/metrics?environment=1118134
+```
 
 GitLab unfurls the link as an embedded metrics panel:
 
-![Embedded Metrics Rendered](img/embedded_metrics_rendered_v12_8.png)
+![Embedded Metrics Rendered](img/embedded_metrics_rendered_v13_4.png)
 
 You can also embed a single chart. To get a link to a chart, click the
 **{ellipsis_v}** **More actions** menu in the upper right corner of the chart,

lib/banzai/filter/inline_metrics_filter.rb

@@ -10,7 +10,6 @@ module Banzai
       # the cost of doing a full regex match.
       def xpath_search
         "descendant-or-self::a[contains(@href,'metrics') and \
-          contains(@href,'environments') and \
           starts-with(@href, '#{gitlab_domain}')]"
       end
 
@@ -29,7 +28,7 @@ module Banzai
           params['project'],
           params['environment'],
           embedded: true,
-          **query_params(params['url'])
+          **query_params(params['url']).except(:environment)
         )
       end
     end

lib/gitlab/metrics/dashboard/url.rb

@@ -10,20 +10,23 @@ module Gitlab
 
           QUERY_PATTERN = '(?<query>\?[a-zA-Z0-9%.()+_=-]+(&[a-zA-Z0-9%.()+_=-]+)*)?'
           ANCHOR_PATTERN = '(?<anchor>\#[a-z0-9_-]+)?'
-          OPTIONAL_DASH_PATTERN = '(?:/-)?'
+          DASH_PATTERN = '(?:/-)'
 
-          # Matches urls for a metrics dashboard. This could be
-          # either the /metrics endpoint or the /metrics_dashboard
-          # endpoint.
+          # Matches urls for a metrics dashboard.
+          # This regex needs to match the old metrics URL, the new metrics URL,
+          # and the dashboard URL (inline_metrics_redactor_filter.rb
+          # uses this regex to match against the dashboard URL.)
           #
-          # EX - https://<host>/<namespace>/<project>/environments/<env_id>/metrics
+          # EX - Old URL: https://<host>/<namespace>/<project>/environments/<env_id>/metrics
+          # OR
+          # New URL: https://<host>/<namespace>/<project>/-/metrics?environment=<env_id>
+          # OR
+          # dashboard URL: https://<host>/<namespace>/<project>/environments/<env_id>/metrics_dashboard
           def metrics_regex
             strong_memoize(:metrics_regex) do
               regex_for_project_metrics(
                 %r{
-                    /environments
-                    /(?<environment>\d+)
-                    /(metrics_dashboard|metrics)
+                    ( #{environment_metrics_regex} ) | ( #{non_environment_metrics_regex} )
                 }x
               )
             end
@@ -36,6 +39,7 @@ module Gitlab
             strong_memoize(:grafana_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /grafana
                   /metrics_dashboard
                 }x
@@ -44,16 +48,22 @@ module Gitlab
           end
 
           # Matches dashboard urls for a metric chart embed
-          # for cluster metrics
+          # for cluster metrics.
+          # This regex needs to match the dashboard URL as well, not just the trigger URL.
+          # The inline_metrics_redactor_filter.rb uses this regex to match against
+          # the dashboard URL.
           #
           # EX - https://<host>/<namespace>/<project>/-/clusters/<cluster_id>/?group=Cluster%20Health&title=Memory%20Usage&y_label=Memory%20(GiB)
+          # dashboard URL - https://<host>/<namespace>/<project>/-/clusters/<cluster_id>/metrics_dashboard?group=Cluster%20Health&title=Memory%20Usage&y_label=Memory%20(GiB)
           def clusters_regex
             strong_memoize(:clusters_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /clusters
                   /(?<cluster_id>\d+)
                   /?
+                  ( (/metrics) | ( /metrics_dashboard\.json ) )?
                 }x
               )
             end
@@ -67,10 +77,11 @@ module Gitlab
             strong_memoize(:alert_regex) do
               regex_for_project_metrics(
                 %r{
+                  #{DASH_PATTERN}?
                   /prometheus
                   /alerts
                   /(?<alert>\d+)
-                  /metrics_dashboard
+                  /metrics_dashboard(\.json)?
                 }x
               )
             end
@@ -95,16 +106,37 @@ module Gitlab
 
           private
 
+          def environment_metrics_regex
+            %r{
+              #{DASH_PATTERN}?
+              /environments
+              /(?<environment>\d+)
+              /(metrics_dashboard|metrics)
+            }x
+          end
+
+          def non_environment_metrics_regex
+            %r{
+              #{DASH_PATTERN}
+              /metrics
+              (?=                             # Lookahead to ensure there is an environment query param
+                \?
+                .*
+                environment=(?<environment>\d+)
+                .*
+              )
+            }x
+          end
+
           def regex_for_project_metrics(path_suffix_pattern)
             %r{
-              (?<url>
+              ^(?<url>
                 #{gitlab_host_pattern}
                 #{project_path_pattern}
-                #{OPTIONAL_DASH_PATTERN}
                 #{path_suffix_pattern}
                 #{QUERY_PATTERN}
                 #{ANCHOR_PATTERN}
-              )
+              )$
             }x
           end
 

spec/lib/banzai/filter/inline_metrics_filter_spec.rb

@@ -5,25 +5,68 @@ require 'spec_helper'
 RSpec.describe Banzai::Filter::InlineMetricsFilter do
   include FilterSpecHelper
 
-  let(:params) { ['foo', 'bar', 12] }
-  let(:query_params) { {} }
-
-  let(:trigger_url) { urls.metrics_namespace_project_environment_url(*params, query_params) }
+  let(:environment_id) { 12 }
   let(:dashboard_url) { urls.metrics_dashboard_namespace_project_environment_url(*params, **query_params, embedded: true) }
 
-  it_behaves_like 'a metrics embed filter'
+  let(:query_params) do
+    {
+      dashboard: 'config/prometheus/common_metrics.yml',
+      group: 'System metrics (Kubernetes)',
+      title: 'Core Usage (Pod Average)',
+      y_label: 'Cores per Pod'
+    }
+  end
+
+  context 'with /-/environments/:environment_id/metrics URL' do
+    let(:params) { ['group', 'project', environment_id] }
+    let(:trigger_url) { urls.metrics_namespace_project_environment_url(*params, **query_params) }
+
+    context 'with no query params' do
+      let(:query_params) { {} }
+
+      it_behaves_like 'a metrics embed filter'
+    end
+
+    context 'with query params' do
+      it_behaves_like 'a metrics embed filter'
+    end
+  end
 
-  context 'with query params specified' do
-    let(:query_params) do
-      {
-        dashboard: 'config/prometheus/common_metrics.yml',
-        group: 'System metrics (Kubernetes)',
-        title: 'Core Usage (Pod Average)',
-        y_label: 'Cores per Pod'
-      }
+  context 'with /-/metrics?environment=:environment_id URL' do
+    let(:params) { %w(group project) }
+    let(:trigger_url) { urls.namespace_project_metrics_dashboard_url(*params, **query_params) }
+    let(:dashboard_url) do
+      urls.metrics_dashboard_namespace_project_environment_url(
+        *params.append(environment_id),
+        **query_params.except(:environment),
+        embedded: true
+      )
     end
 
-    it_behaves_like 'a metrics embed filter'
+    context 'with query params' do
+      it_behaves_like 'a metrics embed filter' do
+        before do
+          query_params.merge!(environment: environment_id)
+        end
+      end
+    end
+
+    context 'with only environment in query params' do
+      let(:query_params) { { environment: environment_id } }
+
+      it_behaves_like 'a metrics embed filter'
+    end
+
+    context 'with no query params' do
+      let(:query_params) { {} }
+
+      it 'ignores metrics URL without environment parameter' do
+        input = %(<a href="#{trigger_url}">example</a>)
+        filtered_input = filter(input).to_s
+
+        expect(CGI.unescape_html(filtered_input)).to eq(input)
+      end
+    end
   end
 
   it 'leaves links to other dashboards unchanged' do

spec/lib/banzai/filter/inline_metrics_redactor_filter_spec.rb

@@ -22,6 +22,13 @@ RSpec.describe Banzai::Filter::InlineMetricsRedactorFilter do
     it_behaves_like 'redacts the embed placeholder'
     it_behaves_like 'retains the embed placeholder when applicable'
 
+    context 'with /-/metrics?environment=:environment_id URL' do
+      let(:url) { urls.project_metrics_dashboard_url(project, embedded: true, environment: 1) }
+
+      it_behaves_like 'redacts the embed placeholder'
+      it_behaves_like 'retains the embed placeholder when applicable'
+    end
+
     context 'for a grafana dashboard' do
       let(:url) { urls.project_grafana_api_metrics_dashboard_url(project, embedded: true) }
 
@@ -33,7 +40,7 @@ RSpec.describe Banzai::Filter::InlineMetricsRedactorFilter do
       let_it_be(:cluster) { create(:cluster, :provided_by_gcp, :project, projects: [project]) }
       let(:params) { [project.namespace.path, project.path, cluster.id] }
       let(:query_params) { { group: 'Cluster Health', title: 'CPU Usage', y_label: 'CPU (cores)' } }
-      let(:url) { urls.metrics_dashboard_namespace_project_cluster_url(*params, **query_params) }
+      let(:url) { urls.metrics_dashboard_namespace_project_cluster_url(*params, **query_params, format: :json) }
 
       context 'with user who can read cluster' do
         it_behaves_like 'redacts the embed placeholder'

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

@@ -6,11 +6,12 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   include Gitlab::Routing.url_helpers
 
   describe '#metrics_regex' do
+    let(:environment_id) { 1 }
     let(:url_params) do
       [
         'foo',
         'bar',
-        1,
+        environment_id,
         {
           start: '2019-08-02T05:43:09.000Z',
           dashboard: 'config/prometheus/common_metrics.yml',
@@ -33,12 +34,42 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
 
     subject { described_class.metrics_regex }
 
-    context 'for metrics route' do
+    context 'for /-/environments/:environment_id/metrics route' do
       let(:url) { metrics_namespace_project_environment_url(*url_params) }
 
       it_behaves_like 'regex which matches url when expected'
     end
 
+    context 'for /-/metrics?environment=:environment_id route' do
+      let(:url) { namespace_project_metrics_dashboard_url(*url_params) }
+      let(:url_params) do
+        [
+          'namespace1',
+          'project1',
+          {
+            environment: environment_id,
+            start: '2019-08-02T05:43:09.000Z',
+            dashboard: 'config/prometheus/common_metrics.yml',
+            group: 'awesome group',
+            anchor: 'title'
+          }
+        ]
+      end
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'namespace1',
+          'project' => 'project1',
+          'environment' => "#{environment_id}",
+          'query' => "?dashboard=config%2Fprometheus%2Fcommon_metrics.yml&environment=#{environment_id}&group=awesome+group&start=2019-08-02T05%3A43%3A09.000Z",
+          'anchor' => '#title'
+        }
+      end
+
+      it_behaves_like 'regex which matches url when expected'
+    end
+
     context 'for metrics_dashboard route' do
       let(:url) { metrics_dashboard_namespace_project_environment_url(*url_params) }
 
@@ -47,16 +78,19 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   end
 
   describe '#clusters_regex' do
-    let(:url) do
-      Gitlab::Routing.url_helpers.namespace_project_cluster_url(
+    let(:url) { Gitlab::Routing.url_helpers.namespace_project_cluster_url(*url_params) }
+    let(:url_params) do
+      [
         'foo',
         'bar',
         '1',
-        group: 'Cluster Health',
-        title: 'Memory Usage',
-        y_label: 'Memory 20(GiB)',
-        anchor: 'title'
-      )
+        {
+          group: 'Cluster Health',
+          title: 'Memory Usage',
+          y_label: 'Memory 20(GiB)',
+          anchor: 'title'
+        }
+      ]
     end
 
     let(:expected_params) do
@@ -73,6 +107,27 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
     subject { described_class.clusters_regex }
 
     it_behaves_like 'regex which matches url when expected'
+
+    context 'for metrics_dashboard route' do
+      let(:url) do
+        metrics_dashboard_namespace_project_cluster_url(
+          *url_params, cluster_type: :project, embedded: true, format: :json
+        )
+      end
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'foo',
+          'project' => 'bar',
+          'cluster_id' => '1',
+          'query' => '?cluster_type=project&embedded=true',
+          'anchor' => nil
+        }
+      end
+
+      it_behaves_like 'regex which matches url when expected'
+    end
   end
 
   describe '#grafana_regex' do
@@ -103,15 +158,18 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
   end
 
   describe '#alert_regex' do
-    let(:url) do
-      Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(
+    let(:url) { Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(*url_params) }
+    let(:url_params) do
+      [
         'foo',
         'bar',
         '1',
-        start: '2020-02-10T12:59:49.938Z',
-        end: '2020-02-10T20:59:49.938Z',
-        anchor: "anchor"
-      )
+        {
+          start: '2020-02-10T12:59:49.938Z',
+          end: '2020-02-10T20:59:49.938Z',
+          anchor: "anchor"
+        }
+      ]
     end
 
     let(:expected_params) do
@@ -128,6 +186,21 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do
     subject { described_class.alert_regex }
 
     it_behaves_like 'regex which matches url when expected'
+
+    it_behaves_like 'regex which matches url when expected' do
+      let(:url) { Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url(*url_params, format: :json) }
+
+      let(:expected_params) do
+        {
+          'url' => url,
+          'namespace' => 'foo',
+          'project' => 'bar',
+          'alert' => '1',
+          'query' => nil,
+          'anchor' => nil
+        }
+      end
+    end
   end
 
   describe '#build_dashboard_url' do