Merge branch 'dbodicherla-add-prefix-for-templating-variables' into 'master'

Add prefix to template vars in url

See merge request gitlab-org/gitlab!31690

app/assets/javascripts/monitoring/constants.js

@@ -216,3 +216,14 @@ export const VARIABLE_TYPES = {
   custom: 'custom',
   text: 'text',
 };
+
+/**
+ * The names of templating variables defined in the dashboard yml
+ * file are prefixed with a constant so that it doesn't collide with
+ * other URL params that the monitoring dashboard relies on for
+ * features like panel fullscreen etc.
+ *
+ * The prefix is added before it is appended to the URL and removed
+ * before passing the data to the backend.
+ */
+export const VARIABLE_PREFIX = 'var-';

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

+import { flatMap } from 'lodash';
+import { removePrefixFromLabels } from './utils';
 import { NOT_IN_DB_PREFIX } from '../constants';
 
 const metricsIdsInPanel = panel =>
@@ -110,16 +112,19 @@ export const filteredEnvironments = state =>
   );
 
 /**
- * Maps an variables object to an array
+ * Maps an variables object to an array along with stripping
+ * the variable prefix.
+ *
  * @param {Object} variables - Custom variables provided by the user
  * @returns {Array} The custom variables array to be send to the API
  * in the format of [variable1, variable1_value]
  */
 
 export const getCustomVariablesArray = state =>
-  Object.entries(state.promVariables)
-    .flat()
-    .map(encodeURIComponent);
+  flatMap(state.promVariables, (val, key) => [
+    encodeURIComponent(removePrefixFromLabels(key)),
+    encodeURIComponent(val),
+  ]);
 
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

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

@@ -2,7 +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';
+import { NOT_IN_DB_PREFIX, VARIABLE_PREFIX } from '../constants';
 
 export const gqClient = createGqClient(
   {},
@@ -229,3 +229,22 @@ export const normalizeQueryResult = timeSeries => {
 
   return normalizedResult;
 };
+
+/**
+ * Variable labels are used as names for the dropdowns and also
+ * as URL params. Prefixing the name reduces the risk of
+ * collision with other URL params
+ *
+ * @param {String} label label for the template variable
+ * @returns {String}
+ */
+export const addPrefixToLabels = label => `${VARIABLE_PREFIX}${label}`;
+
+/**
+ * Before the templating variables are passed to the backend the
+ * prefix needs to be removed.
+ *
+ * @param {String} label label to remove prefix from
+ * @returns {String}
+ */
+export const removePrefixFromLabels = label => label.replace(VARIABLE_PREFIX, '');

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

 import { isString } from 'lodash';
+import { addPrefixToLabels } from './utils';
 import { VARIABLE_TYPES } from '../constants';
 
 /**
@@ -149,7 +150,7 @@ export const parseTemplatingVariables = ({ variables = {} } = {}) =>
     if (parsedVar) {
       acc[key] = {
         ...parsedVar,
-        label: parsedVar.label || key,
+        label: addPrefixToLabels(parsedVar.label || key),
       };
     }
     return acc;

app/assets/javascripts/monitoring/utils.js

-import { omit } from 'lodash';
+import { pickBy } from 'lodash';
 import { queryToObject, mergeUrlParams, removeParams } from '~/lib/utils/url_utility';
 import {
   timeRangeParamNames,
   timeRangeFromParams,
   timeRangeToParams,
 } from '~/lib/utils/datetime_range';
+import { VARIABLE_PREFIX } from './constants';
 
 /**
  * List of non time range url parameters
@@ -122,19 +123,16 @@ export const timeRangeFromUrl = (search = window.location.search) => {
 };
 
 /**
- * Returns an array with user defined variables from the URL
+ * User-defined variables from the URL are extracted. The variables
+ * begin with a constant prefix so that it doesn't collide with
+ * other URL params.
  *
- * @returns {Array} The custom variables defined by the
- * user in the URL
  * @param {String} New URL
+ * @returns {Object} The custom variables defined by the user in the URL
  */
 
-export const promCustomVariablesFromUrl = (search = window.location.search) => {
-  const params = queryToObject(search);
-  const paramsToRemove = timeRangeParamNames.concat(dashboardParams);
-
-  return omit(params, paramsToRemove);
-};
+export const promCustomVariablesFromUrl = (search = window.location.search) =>
+  pickBy(queryToObject(search), (val, key) => key.startsWith(VARIABLE_PREFIX));
 
 /**
  * Returns a URL with no time range based on the current URL.

changelogs/unreleased/dbodicherla-add-prefix-for-templating-variables.yml

+---
+title: Add prefix to template variables in URL in the monitoring dashboard
+merge_request: 31690
+author:
+type: changed

spec/frontend/monitoring/components/variables_section_spec.js

@@ -15,8 +15,8 @@ describe('Metrics dashboard/variables section component', () => {
   let store;
   let wrapper;
   const sampleVariables = {
-    label1: 'pod',
-    label2: 'main',
+    'var-label1': 'pod',
+    'var-label2': 'main',
   };
 
   const createShallowWrapper = () => {

spec/frontend/monitoring/mock_data.js

@@ -642,7 +642,7 @@ const generateMockTemplatingData = data => {
 
 const responseForSimpleTextVariable = {
   simpleText: {
-    label: 'simpleText',
+    label: 'var-simpleText',
     type: 'text',
     value: 'Simple text',
   },
@@ -650,7 +650,7 @@ const responseForSimpleTextVariable = {
 
 const responseForAdvTextVariable = {
   advText: {
-    label: 'Variable 4',
+    label: 'var-Variable 4',
     type: 'text',
     value: 'default',
   },
@@ -658,7 +658,7 @@ const responseForAdvTextVariable = {
 
 const responseForSimpleCustomVariable = {
   simpleCustom: {
-    label: 'simpleCustom',
+    label: 'var-simpleCustom',
     options: [
       {
         default: false,
@@ -682,7 +682,7 @@ const responseForSimpleCustomVariable = {
 
 const responseForAdvancedCustomVariableWithoutOptions = {
   advCustomWithoutOpts: {
-    label: 'advCustomWithoutOpts',
+    label: 'var-advCustomWithoutOpts',
     options: [],
     type: 'custom',
   },
@@ -690,7 +690,7 @@ const responseForAdvancedCustomVariableWithoutOptions = {
 
 const responseForAdvancedCustomVariableWithoutLabel = {
   advCustomWithoutLabel: {
-    label: 'advCustomWithoutLabel',
+    label: 'var-advCustomWithoutLabel',
     options: [
       {
         default: false,
@@ -710,7 +710,7 @@ const responseForAdvancedCustomVariableWithoutLabel = {
 const responseForAdvancedCustomVariable = {
   ...responseForSimpleCustomVariable,
   advCustomNormal: {
-    label: 'Advanced Var',
+    label: 'var-Advanced Var',
     options: [
       {
         default: false,

spec/frontend/monitoring/store/getters_spec.js

@@ -327,7 +327,8 @@ describe('Monitoring store Getters', () => {
   describe('getCustomVariablesArray', () => {
     let state;
     const sampleVariables = {
-      label1: 'pod',
+      'var-label1': 'pod',
+      'var-label2': 'env',
     };
 
     beforeEach(() => {
@@ -340,7 +341,7 @@ describe('Monitoring store Getters', () => {
       mutations[types.SET_PROM_QUERY_VARIABLES](state, sampleVariables);
       const variablesArray = getters.getCustomVariablesArray(state);
 
-      expect(variablesArray).toEqual(['label1', 'pod']);
+      expect(variablesArray).toEqual(['label1', 'pod', 'label2', 'env']);
     });
 
     it('transforms the promVariables object to an empty array when no keys are present', () => {

spec/frontend/monitoring/utils_spec.js

@@ -192,9 +192,10 @@ describe('monitoring/utils', () => {
         direction: 'left',
         anchor: 'top',
         pod: 'POD',
+        'var-pod': 'POD',
       });
 
-      expect(promCustomVariablesFromUrl()).toEqual(expect.objectContaining({ pod: 'POD' }));
+      expect(promCustomVariablesFromUrl()).toEqual(expect.objectContaining({ 'var-pod': 'POD' }));
     });
 
     it('returns an empty object when no custom variables are present', () => {