Merge branch '218312-change-variables-parameter-format' into 'master'

Send variables parameter as a Hash to the Prometheus proxy API

See merge request gitlab-org/gitlab!33062

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

@@ -218,13 +218,16 @@ export const fetchPrometheusMetric = (
   { commit, state, getters },
   { metric, defaultQueryParams },
 ) => {
-  const queryParams = { ...defaultQueryParams };
+  let queryParams = { ...defaultQueryParams };
   if (metric.step) {
     queryParams.step = metric.step;
   }
 
   if (Object.keys(state.variables).length > 0) {
-    queryParams.variables = getters.getCustomVariablesArray;
+    queryParams = {
+      ...queryParams,
+      ...getters.getCustomVariablesParams,
+    };
   }
 
   commit(types.REQUEST_METRIC_RESULT, { metricId: metric.metricId });

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

-import { flatMap } from 'lodash';
 import { NOT_IN_DB_PREFIX } from '../constants';
+import { addPrefixToCustomVariableParams } from './utils';
 
 const metricsIdsInPanel = panel =>
   panel.metrics.filter(metric => metric.metricId && metric.result).map(metric => metric.metricId);
@@ -116,13 +116,27 @@ export const filteredEnvironments = state =>
  * Maps an variables object to an array along with stripping
  * the variable prefix.
  *
+ * This method outputs an object in the below format
+ *
+ * {
+ *   variables[key1]=value1,
+ *   variables[key2]=value2,
+ * }
+ *
+ * This is done so that the backend can identify the custom
+ * user-defined variables coming through the URL and differentiate
+ * from other variables used for Prometheus API endpoint.
+ *
  * @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]
+ * in the format of {variables[key1]=value1, variables[key2]=value2}
  */
 
-export const getCustomVariablesArray = state =>
-  flatMap(state.variables, (variable, key) => [key, variable.value]);
+export const getCustomVariablesParams = state =>
+  Object.keys(state.variables).reduce((acc, variable) => {
+    acc[addPrefixToCustomVariableParams(variable)] = state.variables[variable]?.value;
+    return acc;
+  }, {});
 
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

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

@@ -249,3 +249,19 @@ export const normalizeQueryResult = timeSeries => {
 
   return normalizedResult;
 };
+
+/**
+ * Custom variables defined in the dashboard yml file are
+ * eventually passed over the wire to the backend Prometheus
+ * API proxy.
+ *
+ * This method adds a prefix to the URL param keys so that
+ * the backend can differential these variables from the other
+ * variables.
+ *
+ * This is currently only used by getters/getCustomVariablesParams
+ *
+ * @param {String} key Variable key that needs to be prefixed
+ * @returns {String}
+ */
+export const addPrefixToCustomVariableParams = key => `variables[${key}]`;

app/services/prometheus/proxy_variable_substitution_service.rb

@@ -32,8 +32,8 @@ module Prometheus
     def validate_variables(_result)
       return success unless variables
 
-      unless variables.is_a?(Array) && variables.size.even?
-        return error(_('Optional parameter "variables" must be an array of keys and values. Ex: [key1, value1, key2, value2]'))
+      unless variables.is_a?(ActionController::Parameters)
+        return error(_('Optional parameter "variables" must be a Hash. Ex: variables[key1]=value1'))
       end
 
       success
@@ -88,12 +88,7 @@ module Prometheus
     end
 
     def variables_hash
-      # .each_slice(2) converts ['key1', 'value1', 'key2', 'value2'] into
-      # [['key1', 'value1'], ['key2', 'value2']] which is then converted into
-      # a hash by to_h: {'key1' => 'value1', 'key2' => 'value2'}
-      # to_h will raise an ArgumentError if the number of elements in the original
-      # array is not even.
-      variables&.each_slice(2).to_h
+      variables.to_h
     end
 
     def query(result)

changelogs/unreleased/218312-change-variables-parameter-format.yml

+---
+title: Change format of variables parameter in Prometheus proxy API for metrics dashboard
+merge_request: 33062
+author:
+type: fixed

locale/gitlab.pot

@@ -15060,7 +15060,7 @@ msgstr ""
 msgid "Optional"
 msgstr ""
 
-msgid "Optional parameter \"variables\" must be an array of keys and values. Ex: [key1, value1, key2, value2]"
+msgid "Optional parameter \"variables\" must be a Hash. Ex: variables[key1]=value1"
 msgstr ""
 
 msgid "Optionally, you can %{link_to_customize} how FogBugz email addresses and usernames are imported into GitLab."

spec/controllers/projects/environments/prometheus_api_controller_spec.rb

@@ -84,12 +84,12 @@ describe Projects::Environments::PrometheusApiController do
 
           before do
             expected_params[:query] = %{up{pod_name="#{pod_name}"}}
-            expected_params[:variables] = ['pod_name', pod_name]
+            expected_params[:variables] = { 'pod_name' => pod_name }
           end
 
           it 'replaces variables with values' do
             get :proxy, params: environment_params.merge(
-              query: 'up{pod_name="{{pod_name}}"}', variables: ['pod_name', pod_name]
+              query: 'up{pod_name="{{pod_name}}"}', variables: { 'pod_name' => pod_name }
             )
 
             expect(response).to have_gitlab_http_status(:success)

spec/frontend/monitoring/store/getters_spec.js

@@ -329,7 +329,7 @@ describe('Monitoring store Getters', () => {
     });
   });
 
-  describe('getCustomVariablesArray', () => {
+  describe('getCustomVariablesParams', () => {
     let state;
 
     beforeEach(() => {
@@ -340,25 +340,21 @@ describe('Monitoring store Getters', () => {
 
     it('transforms the variables object to an array in the [variable, variable_value] format for all variable types', () => {
       mutations[types.SET_VARIABLES](state, mockTemplatingDataResponses.allVariableTypes);
-      const variablesArray = getters.getCustomVariablesArray(state);
-
-      expect(variablesArray).toEqual([
-        'simpleText',
-        'Simple text',
-        'advText',
-        'default',
-        'simpleCustom',
-        'value1',
-        'advCustomNormal',
-        'value2',
-      ]);
+      const variablesArray = getters.getCustomVariablesParams(state);
+
+      expect(variablesArray).toEqual({
+        'variables[advCustomNormal]': 'value2',
+        'variables[advText]': 'default',
+        'variables[simpleCustom]': 'value1',
+        'variables[simpleText]': 'Simple text',
+      });
     });
 
     it('transforms the variables object to an empty array when no keys are present', () => {
       mutations[types.SET_VARIABLES](state, {});
-      const variablesArray = getters.getCustomVariablesArray(state);
+      const variablesArray = getters.getCustomVariablesParams(state);
 
-      expect(variablesArray).toEqual([]);
+      expect(variablesArray).toEqual({});
     });
   });
 

spec/services/prometheus/proxy_variable_substitution_service_spec.rb

@@ -64,7 +64,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
       let(:params_keys) do
         {
           query: 'up{pod_name="{{pod_name}}"}',
-          variables: ['pod_name', pod_name]
+          variables: { 'pod_name' => pod_name }
         }
       end
 
@@ -76,7 +76,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
         let(:params_keys) do
           {
             query: 'up{pod_name="{{pod_name}}",env="{{ci_environment_slug}}"}',
-            variables: ['pod_name', pod_name, 'ci_environment_slug', 'custom_value']
+            variables: { 'pod_name' => pod_name, 'ci_environment_slug' => 'custom_value' }
           }
         end
 
@@ -95,8 +95,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
           }
         end
 
-        it_behaves_like 'error', 'Optional parameter "variables" must be an ' \
-          'array of keys and values. Ex: [key1, value1, key2, value2]'
+        it_behaves_like 'error', 'Optional parameter "variables" must be a Hash. Ex: variables[key1]=value1'
       end
 
       context 'with nil variables' do