Merge branch '204796-hide-create-alert-on-custom-metrics' into 'master'

Hide ability to create alert by checking if metric is added to the DB

Closes #204796

See merge request gitlab-org/gitlab!28180

app/assets/javascripts/monitoring/components/panel_type.vue

@@ -101,7 +101,8 @@ export default {
       return this.graphData.title || '';
     },
     alertWidgetAvailable() {
-      return IS_EE && this.prometheusAlertsAvailable && this.alertsEndpoint && this.graphData;
+      // This method is extended by ee functionality
+      return false;
     },
     graphDataHasMetrics() {
       return (
@@ -209,7 +210,7 @@ export default {
       >
         <div class="d-flex align-items-center">
           <alert-widget
-            v-if="alertWidgetAvailable && graphData"
+            v-if="alertWidgetAvailable"
             :modal-id="`alert-modal-${index}`"
             :alerts-endpoint="alertsEndpoint"
             :relevant-queries="graphData.metrics"

app/assets/javascripts/monitoring/constants.js

@@ -104,3 +104,8 @@ export const endpointKeys = [
  * as Vue props.
  */
 export const initialStateKeys = [...endpointKeys, 'currentEnvironmentName'];
+
+/**
+ * Constant to indicate if a metric exists in the database
+ */
+export const NOT_IN_DB_PREFIX = 'NO_DB';

app/assets/javascripts/monitoring/stores/getters.js

+import { NOT_IN_DB_PREFIX } from '../constants';
+
 const metricsIdsInPanel = panel =>
   panel.metrics.filter(metric => metric.metricId && metric.result).map(metric => metric.metricId);
 
@@ -58,6 +60,29 @@ export const metricsWithData = state => groupKey => {
   return res;
 };
 
+/**
+ * Metrics loaded from project-defined dashboards do not have a metric_id.
+ * This getter checks which metrics are stored in the db (have a metric id)
+ * This is hopefully a temporary solution until BE processes metrics before passing to FE
+ *
+ * Related:
+ * https://gitlab.com/gitlab-org/gitlab/-/issues/28241
+ * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27447
+ */
+export const metricsSavedToDb = state => {
+  const metricIds = [];
+  state.dashboard.panelGroups.forEach(({ panels }) => {
+    panels.forEach(({ metrics }) => {
+      const metricIdsInDb = metrics
+        .filter(({ metricId }) => !metricId.startsWith(NOT_IN_DB_PREFIX))
+        .map(({ metricId }) => metricId);
+
+      metricIds.push(...metricIdsInDb);
+    });
+  });
+  return metricIds;
+};
+
 /**
  * Filter environments by names.
  *

app/assets/javascripts/monitoring/stores/utils.js

@@ -2,6 +2,7 @@ import { slugify } from '~/lib/utils/text_utility';
 import createGqClient, { fetchPolicies } from '~/lib/graphql';
 import { SUPPORTED_FORMATS } from '~/lib/utils/unit_format';
 import { getIdFromGraphQLId } from '~/graphql_shared/utils';
+import { NOT_IN_DB_PREFIX } from '../constants';
 
 export const gqClient = createGqClient(
   {},
@@ -14,11 +15,18 @@ export const gqClient = createGqClient(
  * Metrics loaded from project-defined dashboards do not have a metric_id.
  * This method creates a unique ID combining metric_id and id, if either is present.
  * This is hopefully a temporary solution until BE processes metrics before passing to FE
+ *
+ * Related:
+ * https://gitlab.com/gitlab-org/gitlab/-/issues/28241
+ * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27447
+ *
  * @param {Object} metric - metric
+ * @param {Number} metric.metric_id - Database metric id
+ * @param {String} metric.id - User-defined identifier
  * @returns {Object} - normalized metric with a uniqueID
  */
 // eslint-disable-next-line babel/camelcase
-export const uniqMetricsId = ({ metric_id, id }) => `${metric_id}_${id}`;
+export const uniqMetricsId = ({ metric_id, id }) => `${metric_id || NOT_IN_DB_PREFIX}_${id}`;
 
 /**
  * Project path has a leading slash that doesn't work well

ee/app/assets/javascripts/monitoring/components/panel_type.vue

 <script>
+import { mapGetters } from 'vuex';
 import CustomMetricsFormFields from 'ee/custom_metrics/components/custom_metrics_form_fields.vue';
 import CePanelType from '~/monitoring/components/panel_type.vue';
 import AlertWidget from './alert_widget.vue';
@@ -20,17 +21,27 @@ export default {
       required: false,
       default: false,
     },
-    groupId: {
-      type: String,
-      required: false,
-      default: 'panel-type-chart',
-    },
   },
   data() {
     return {
       allAlerts: {},
     };
   },
+  computed: {
+    ...mapGetters('monitoringDashboard', ['metricsSavedToDb']),
+    hasMetricsInDb() {
+      const { metrics = [] } = this.graphData;
+      return metrics.some(({ metricId }) => this.metricsSavedToDb.includes(metricId));
+    },
+    alertWidgetAvailable() {
+      return (
+        this.prometheusAlertsAvailable &&
+        this.alertsEndpoint &&
+        this.graphData &&
+        this.hasMetricsInDb
+      );
+    },
+  },
   methods: {
     setAlerts(alertPath, alertAttributes) {
       if (alertAttributes) {

ee/changelogs/unreleased/204796-hide-create-alert-on-custom-metrics.yml

+---
+title: Hide ability to create alert on custom metrics dashboard
+merge_request: 28180
+author:
+type: fixed

ee/spec/frontend/monitoring/components/panel_type_spec.js

 import Vuex from 'vuex';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
-import AxiosMockAdapter from 'axios-mock-adapter';
-import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
+import { GlDropdownItem } from '@gitlab/ui';
+import { monitoringDashboard } from '~/monitoring/stores';
 import PanelType from 'ee/monitoring/components/panel_type.vue';
 import AlertWidget from 'ee/monitoring/components/alert_widget.vue';
-import { createStore } from '~/monitoring/stores';
-import axios from '~/lib/utils/axios_utils';
 import { graphDataPrometheusQueryRange } from 'jest/monitoring/mock_data';
 
-global.URL.createObjectURL = jest.fn();
-
 const localVue = createLocalVue();
+
 localVue.use(Vuex);
 
+global.URL.createObjectURL = jest.fn();
+
 describe('Panel Type', () => {
-  let axiosMock;
-  let panelType;
   let store;
-  const exampleText = 'example_text';
+  let wrapper;
+
+  const setMetricsSavedToDb = val =>
+    monitoringDashboard.getters.metricsSavedToDb.mockReturnValue(val);
+  const findAlertsWidget = () => wrapper.find(AlertWidget);
+  const findMenuItemAlert = () =>
+    wrapper.findAll(GlDropdownItem).filter(i => i.text() === 'Alerts');
+
+  const mockPropsData = {
+    graphData: graphDataPrometheusQueryRange,
+    clipboardText: 'example_text',
+    alertsEndpoint: '/endpoint',
+    prometheusAlertsAvailable: true,
+  };
 
   const createWrapper = propsData => {
-    store = createStore();
-    panelType = shallowMount(PanelType, {
-      propsData,
+    wrapper = shallowMount(PanelType, {
+      propsData: {
+        ...mockPropsData,
+        ...propsData,
+      },
       store,
       localVue,
     });
   };
 
   beforeEach(() => {
-    axiosMock = new AxiosMockAdapter(axios);
-    window.gon = {
-      ...window.gon,
-      ee: true,
-    };
-  });
+    jest.spyOn(monitoringDashboard.getters, 'metricsSavedToDb').mockReturnValue([]);
 
-  afterEach(() => {
-    axiosMock.reset();
+    store = new Vuex.Store({
+      modules: {
+        monitoringDashboard,
+      },
+    });
   });
 
-  describe('metrics with alert', () => {
-    describe('with license', () => {
-      beforeEach(() => {
-        createWrapper({
-          clipboardText: exampleText,
-          graphData: graphDataPrometheusQueryRange,
-          alertsEndpoint: '/endpoint',
-          prometheusAlertsAvailable: true,
-        });
-      });
-
-      afterEach(() => {
-        panelType.destroy();
-      });
-
-      it('shows alert widget and dropdown item', done => {
-        localVue.nextTick(() => {
-          expect(panelType.find(AlertWidget).exists()).toBe(true);
-          expect(
-            panelType
-              .findAll(GlDropdownItem)
-              .filter(i => i.text() === 'Alerts')
-              .exists(),
-          ).toBe(true);
-
-          done();
-        });
-      });
-
-      it('shows More actions dropdown on chart', done => {
-        localVue.nextTick(() => {
-          expect(
-            panelType
-              .findAll(GlDropdown)
-              .filter(d => d.attributes('title') === 'More actions')
-              .exists(),
-          ).toBe(true);
+  describe('panel type alerts', () => {
+    describe.each`
+      desc                                           | metricsSavedToDb                                       | propsData                               | isShown
+      ${'with license and no metrics in db'}         | ${[]}                                                  | ${{}}                                   | ${false}
+      ${'with license and related metrics in db'}    | ${[graphDataPrometheusQueryRange.metrics[0].metricId]} | ${{}}                                   | ${true}
+      ${'without license and related metrics in db'} | ${[graphDataPrometheusQueryRange.metrics[0].metricId]} | ${{ prometheusAlertsAvailable: false }} | ${false}
+      ${'with license and unrelated metrics in db'}  | ${['another_metric_id']}                               | ${{}}                                   | ${false}
+    `('$desc', ({ metricsSavedToDb, isShown, propsData }) => {
+      const showsDesc = isShown ? 'shows' : 'does not show';
 
-          done();
-        });
+      beforeEach(() => {
+        setMetricsSavedToDb(metricsSavedToDb);
+        createWrapper(propsData);
+        return wrapper.vm.$nextTick();
       });
-    });
 
-    describe('without license', () => {
-      beforeEach(() => {
-        createWrapper({
-          clipboardText: exampleText,
-          graphData: graphDataPrometheusQueryRange,
-          alertsEndpoint: '/endpoint',
-          prometheusAlertsAvailable: false,
-        });
+      it(`${showsDesc} alert widget`, () => {
+        expect(findAlertsWidget().exists()).toBe(isShown);
       });
 
-      it('does not show alert widget', () => {
-        expect(panelType.find(AlertWidget).exists()).toBe(false);
-        expect(
-          panelType
-            .findAll(GlDropdownItem)
-            .filter(i => i.text() === 'Alerts')
-            .exists(),
-        ).toBe(false);
+      it(`${showsDesc} alert configuration`, () => {
+        expect(findMenuItemAlert().exists()).toBe(isShown);
       });
     });
   });

spec/frontend/monitoring/mock_data.js

@@ -288,7 +288,7 @@ export const mockedEmptyResult = {
 };
 
 export const mockedEmptyThroughputResult = {
-  metricId: 'undefined_response_metrics_nginx_ingress_16_throughput_status_code',
+  metricId: 'NO_DB_response_metrics_nginx_ingress_16_throughput_status_code',
   result: [],
 };
 
@@ -304,12 +304,12 @@ export const mockedQueryResultPayloadCoresTotal = {
 
 export const mockedQueryResultFixture = {
   // First metric in fixture `metrics_dashboard/environment_metrics_dashboard.json`
-  metricId: 'undefined_response_metrics_nginx_ingress_throughput_status_code',
+  metricId: 'NO_DB_response_metrics_nginx_ingress_throughput_status_code',
   result: metricsResult,
 };
 
 export const mockedQueryResultFixtureStatusCode = {
-  metricId: 'undefined_response_metrics_nginx_ingress_latency_pod_average',
+  metricId: 'NO_DB_response_metrics_nginx_ingress_latency_pod_average',
   result: metricsResult,
 };
 
@@ -560,13 +560,11 @@ export const graphDataPrometheusQueryRange = {
   weight: 2,
   metrics: [
     {
-      id: 'metric_a1',
-      metricId: '2',
+      metricId: '2_metric_a',
       query_range:
         'avg(sum(container_memory_usage_bytes{container_name!="POD",pod_name=~"^%{ci_environment_slug}-(.*)",namespace="%{kube_namespace}"}) by (job)) without (job)  /1024/1024/1024',
       unit: 'MB',
       label: 'Total Consumption',
-      metric_id: 2,
       prometheus_endpoint_path:
         '/root/kubernetes-gke-project/environments/35/prometheus/api/v1/query?query=max%28go_memstats_alloc_bytes%7Bjob%3D%22prometheus%22%7D%29+by+%28job%29+%2F1024%2F1024',
       result: [
@@ -587,13 +585,12 @@ export const graphDataPrometheusQueryRangeMultiTrack = {
   y_label: 'Time',
   metrics: [
     {
-      metricId: '1',
+      metricId: '1_metric_b',
       id: 'response_metrics_nginx_ingress_throughput_status_code',
       query_range:
         'sum(rate(nginx_upstream_responses_total{upstream=~"%{kube_namespace}-%{ci_environment_slug}-.*"}[60m])) by (status_code)',
       unit: 'req / sec',
       label: 'Status Code',
-      metric_id: 1,
       prometheus_endpoint_path:
         '/root/rails_nodb/environments/3/prometheus/api/v1/query_range?query=sum%28rate%28nginx_upstream_responses_total%7Bupstream%3D~%22%25%7Bkube_namespace%7D-%25%7Bci_environment_slug%7D-.%2A%22%7D%5B2m%5D%29%29+by+%28status_code%29',
       result: [
@@ -669,8 +666,7 @@ export const stackedColumnMockedData = {
       series_name: 'group 1',
       prometheus_endpoint_path:
         '/root/autodevops-deploy-6/-/environments/24/prometheus/api/v1/query_range?query=avg%28sum%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+by+%28job%29%29+without+%28job%29+%2F+count%28avg%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+without+%28job%29%29+%2F1024%2F1024',
-      metric_id: 'undefined_metric_of_ages_1024',
-      metricId: 'undefined_metric_of_ages_1024',
+      metricId: 'NO_DB_metric_of_ages_1024',
       result: [
         {
           metric: {},
@@ -688,8 +684,7 @@ export const stackedColumnMockedData = {
       series_name: 'group 2',
       prometheus_endpoint_path:
         '/root/autodevops-deploy-6/-/environments/24/prometheus/api/v1/query_range?query=avg%28sum%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+by+%28job%29%29+without+%28job%29+%2F+count%28avg%28container_memory_usage_bytes%7Bcontainer_name%21%3D%22POD%22%2Cpod_name%3D~%22%5E%25%7Bci_environment_slug%7D-%28%5B%5Ec%5D.%2A%7Cc%28%5B%5Ea%5D%7Ca%28%5B%5En%5D%7Cn%28%5B%5Ea%5D%7Ca%28%5B%5Er%5D%7Cr%5B%5Ey%5D%29%29%29%29.%2A%7C%29-%28.%2A%29%22%2Cnamespace%3D%22%25%7Bkube_namespace%7D%22%7D%29+without+%28job%29%29+%2F1024%2F1024',
-      metric_id: 'undefined_metric_of_ages_1000',
-      metricId: 'undefined_metric_of_ages_1000',
+      metricId: 'NO_DB_metric_of_ages_1000',
       result: [
         {
           metric: {},
@@ -713,8 +708,7 @@ export const barMockData = {
     {
       id: 'sla_trends_primary_services',
       series_name: 'group 1',
-      metric_id: 'undefined_sla_trends_primary_services',
-      metricId: 'undefined_sla_trends_primary_services',
+      metricId: 'NO_DB_sla_trends_primary_services',
       query_range:
         'avg(avg_over_time(slo_observation_status{environment="gprd", stage=~"main|", type=~"api|web|git|registry|sidekiq|ci-runners"}[1d])) by (type)',
       unit: 'Percentile',

spec/frontend/monitoring/store/getters_spec.js

+import _ from 'lodash';
 import * as getters from '~/monitoring/stores/getters';
 import mutations from '~/monitoring/stores/mutations';
 import * as types from '~/monitoring/stores/mutation_types';
@@ -274,4 +275,56 @@ describe('Monitoring store Getters', () => {
       });
     });
   });
+
+  describe('metricsSavedToDb', () => {
+    let metricsSavedToDb;
+    let state;
+    let mockData;
+
+    beforeEach(() => {
+      mockData = _.cloneDeep(metricsDashboardPayload);
+      state = {
+        dashboard: {
+          panelGroups: [],
+        },
+      };
+    });
+
+    it('return no metrics when dashboard is not persisted', () => {
+      mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, mockData);
+      metricsSavedToDb = getters.metricsSavedToDb(state);
+
+      expect(metricsSavedToDb).toEqual([]);
+    });
+
+    it('return a metric id when one metric is persisted', () => {
+      const id = 99;
+
+      const [metric] = mockData.panel_groups[0].panels[0].metrics;
+
+      metric.metric_id = id;
+
+      mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, mockData);
+      metricsSavedToDb = getters.metricsSavedToDb(state);
+
+      expect(metricsSavedToDb).toEqual([`${id}_${metric.id}`]);
+    });
+
+    it('return a metric id when two metrics are persisted', () => {
+      const id1 = 101;
+      const id2 = 102;
+
+      const [metric1] = mockData.panel_groups[0].panels[0].metrics;
+      const [metric2] = mockData.panel_groups[0].panels[1].metrics;
+
+      // database persisted 2 metrics
+      metric1.metric_id = id1;
+      metric2.metric_id = id2;
+
+      mutations[types.RECEIVE_METRICS_DATA_SUCCESS](state, mockData);
+      metricsSavedToDb = getters.metricsSavedToDb(state);
+
+      expect(metricsSavedToDb).toEqual([`${id1}_${metric1.id}`, `${id2}_${metric2.id}`]);
+    });
+  });
 });

spec/frontend/monitoring/store/mutations_spec.js

@@ -66,13 +66,13 @@ describe('Monitoring mutations', () => {
       const groups = getGroups();
 
       expect(groups[0].panels[0].metrics[0].metricId).toEqual(
-        'undefined_system_metrics_kubernetes_container_memory_total',
+        'NO_DB_system_metrics_kubernetes_container_memory_total',
       );
       expect(groups[1].panels[0].metrics[0].metricId).toEqual(
-        'undefined_response_metrics_nginx_ingress_throughput_status_code',
+        'NO_DB_response_metrics_nginx_ingress_throughput_status_code',
       );
       expect(groups[2].panels[0].metrics[0].metricId).toEqual(
-        'undefined_response_metrics_nginx_ingress_16_throughput_status_code',
+        'NO_DB_response_metrics_nginx_ingress_16_throughput_status_code',
       );
     });
   });
@@ -184,7 +184,7 @@ describe('Monitoring mutations', () => {
   });
 
   describe('Individual panel/metric results', () => {
-    const metricId = 'undefined_response_metrics_nginx_ingress_throughput_status_code';
+    const metricId = 'NO_DB_response_metrics_nginx_ingress_throughput_status_code';
     const result = [
       {
         values: [[0, 1], [1, 1], [1, 3]],

spec/frontend/monitoring/store/utils_spec.js

@@ -307,7 +307,7 @@ describe('mapToDashboardViewModel', () => {
 
 describe('uniqMetricsId', () => {
   [
-    { input: { id: 1 }, expected: 'undefined_1' },
+    { input: { id: 1 }, expected: 'NO_DB_1' },
     { input: { metric_id: 2 }, expected: '2_undefined' },
     { input: { metric_id: 2, id: 21 }, expected: '2_21' },
     { input: { metric_id: 22, id: 1 }, expected: '22_1' },