Merge branch '228760-define-fe-service-layer-for-prometheus' into 'master'

Define separate layer for prometheus requests

See merge request gitlab-org/gitlab!37455

app/assets/javascripts/monitoring/requests/index.js

+import axios from '~/lib/utils/axios_utils';
+import statusCodes from '~/lib/utils/http_status';
+import { backOff } from '~/lib/utils/common_utils';
+import { PROMETHEUS_TIMEOUT } from '../constants';
+
+const backOffRequest = makeRequestCallback =>
+  backOff((next, stop) => {
+    makeRequestCallback()
+      .then(resp => {
+        if (resp.status === statusCodes.NO_CONTENT) {
+          next();
+        } else {
+          stop(resp);
+        }
+      })
+      .catch(stop);
+  }, PROMETHEUS_TIMEOUT);
+
+export const getDashboard = (dashboardEndpoint, params) =>
+  backOffRequest(() => axios.get(dashboardEndpoint, { params })).then(
+    axiosResponse => axiosResponse.data,
+  );
+
+export const getPrometheusQueryData = (prometheusEndpoint, params) =>
+  backOffRequest(() => axios.get(prometheusEndpoint, { params }))
+    .then(axiosResponse => axiosResponse.data)
+    .then(prometheusResponse => prometheusResponse.data)
+    .catch(error => {
+      // Prometheus returns errors in specific cases
+      // https://prometheus.io/docs/prometheus/latest/querying/api/#format-overview
+      const { response = {} } = error;
+      if (
+        response.status === statusCodes.BAD_REQUEST ||
+        response.status === statusCodes.UNPROCESSABLE_ENTITY ||
+        response.status === statusCodes.SERVICE_UNAVAILABLE
+      ) {
+        const { data } = response;
+        if (data?.status === 'error' && data?.error) {
+          throw new Error(data.error);
+        }
+      }
+      throw error;
+    });

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

@@ -13,16 +13,11 @@ import trackDashboardLoad from '../monitoring_tracking_helper';
 import getEnvironments from '../queries/getEnvironments.query.graphql';
 import getAnnotations from '../queries/getAnnotations.query.graphql';
 import getDashboardValidationWarnings from '../queries/getDashboardValidationWarnings.query.graphql';
-import statusCodes from '../../lib/utils/http_status';
-import { backOff, convertObjectPropsToCamelCase } from '../../lib/utils/common_utils';
+import { convertObjectPropsToCamelCase } from '../../lib/utils/common_utils';
 import { s__, sprintf } from '../../locale';
+import { getDashboard, getPrometheusQueryData } from '../requests';
 
-import {
-  PROMETHEUS_TIMEOUT,
-  ENVIRONMENT_AVAILABLE_STATE,
-  DEFAULT_DASHBOARD_PATH,
-  VARIABLE_TYPES,
-} from '../constants';
+import { ENVIRONMENT_AVAILABLE_STATE, DEFAULT_DASHBOARD_PATH, VARIABLE_TYPES } from '../constants';
 
 function prometheusMetricQueryParams(timeRange) {
   const { start, end } = convertToFixedRange(timeRange);
@@ -38,31 +33,6 @@ function prometheusMetricQueryParams(timeRange) {
   };
 }
 
-function backOffRequest(makeRequestCallback) {
-  return backOff((next, stop) => {
-    makeRequestCallback()
-      .then(resp => {
-        if (resp.status === statusCodes.NO_CONTENT) {
-          next();
-        } else {
-          stop(resp);
-        }
-      })
-      .catch(stop);
-  }, PROMETHEUS_TIMEOUT);
-}
-
-function getPrometheusQueryData(prometheusEndpoint, params) {
-  return backOffRequest(() => axios.get(prometheusEndpoint, { params }))
-    .then(res => res.data)
-    .then(response => {
-      if (response.status === 'error') {
-        throw new Error(response.error);
-      }
-      return response.data;
-    });
-}
-
 // Setup
 
 export const setGettingStartedEmptyState = ({ commit }) => {
@@ -126,8 +96,7 @@ export const fetchDashboard = ({ state, commit, dispatch, getters }) => {
     params.dashboard = getters.fullDashboardPath;
   }
 
-  return backOffRequest(() => axios.get(state.dashboardEndpoint, { params }))
-    .then(resp => resp.data)
+  return getDashboard(state.dashboardEndpoint, params)
     .then(response => {
       dispatch('receiveMetricsDashboardSuccess', { response });
       /**
@@ -484,12 +453,10 @@ export const fetchVariableMetricLabelValues = ({ state, commit }, { defaultQuery
     if (variable.type === VARIABLE_TYPES.metric_label_values) {
       const { prometheusEndpointPath, label } = variable.options;
 
-      const optionsRequest = backOffRequest(() =>
-        axios.get(prometheusEndpointPath, {
-          params: { start_time, end_time },
-        }),
-      )
-        .then(({ data }) => data.data)
+      const optionsRequest = getPrometheusQueryData(prometheusEndpointPath, {
+        start_time,
+        end_time,
+      })
         .then(data => {
           commit(types.UPDATE_VARIABLE_METRIC_LABEL_VALUES, { variable, label, data });
         })

spec/frontend/helpers/backoff_helper.js

+/**
+ * A mock version of a commonUtils `backOff` to test multiple
+ * retries.
+ *
+ * Usage:
+ *
+ * ```
+ * import * as commonUtils from '~/lib/utils/common_utils';
+ * import { backoffMockImplementation } from '../../helpers/backoff_helper';
+ *
+ * beforeEach(() => {
+ *   // ...
+ *   jest.spyOn(commonUtils, 'backOff').mockImplementation(backoffMockImplementation);
+ * });
+ * ```
+ *
+ * @param {Function} callback
+ */
+export const backoffMockImplementation = callback => {
+  const q = new Promise((resolve, reject) => {
+    const stop = arg => (arg instanceof Error ? reject(arg) : resolve(arg));
+    const next = () => callback(next, stop);
+    // Define a timeout based on a mock timer
+    setTimeout(() => {
+      callback(next, stop);
+    });
+  });
+  // Run all resolved promises in chain
+  jest.runOnlyPendingTimers();
+  return q;
+};
+
+export default { backoffMockImplementation };

spec/frontend/monitoring/requests/index_spec.js

+import MockAdapter from 'axios-mock-adapter';
+import axios from '~/lib/utils/axios_utils';
+import statusCodes from '~/lib/utils/http_status';
+import * as commonUtils from '~/lib/utils/common_utils';
+import { backoffMockImplementation } from 'jest/helpers/backoff_helper';
+import { metricsDashboardResponse } from '../fixture_data';
+import { getDashboard, getPrometheusQueryData } from '~/monitoring/requests';
+
+describe('monitoring metrics_requests', () => {
+  let mock;
+
+  beforeEach(() => {
+    mock = new MockAdapter(axios);
+    jest.spyOn(commonUtils, 'backOff').mockImplementation(backoffMockImplementation);
+  });
+
+  afterEach(() => {
+    mock.reset();
+
+    commonUtils.backOff.mockReset();
+  });
+
+  describe('getDashboard', () => {
+    const response = metricsDashboardResponse;
+    const dashboardEndpoint = '/dashboard';
+    const params = {
+      start_time: 'start_time',
+      end_time: 'end_time',
+    };
+
+    it('returns a dashboard response', () => {
+      mock.onGet(dashboardEndpoint).reply(statusCodes.OK, response);
+
+      return getDashboard(dashboardEndpoint, params).then(data => {
+        expect(data).toEqual(metricsDashboardResponse);
+      });
+    });
+
+    it('returns a dashboard response after retrying twice', () => {
+      mock.onGet(dashboardEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(dashboardEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(dashboardEndpoint).reply(statusCodes.OK, response);
+
+      return getDashboard(dashboardEndpoint, params).then(data => {
+        expect(data).toEqual(metricsDashboardResponse);
+        expect(mock.history.get).toHaveLength(3);
+      });
+    });
+
+    it('rejects after getting an error', () => {
+      mock.onGet(dashboardEndpoint).reply(500);
+
+      return getDashboard(dashboardEndpoint, params).catch(error => {
+        expect(error).toEqual(expect.any(Error));
+        expect(mock.history.get).toHaveLength(1);
+      });
+    });
+  });
+
+  describe('getPrometheusQueryData', () => {
+    const response = {
+      status: 'success',
+      data: {
+        resultType: 'matrix',
+        result: [],
+      },
+    };
+    const prometheusEndpoint = '/query_range';
+    const params = {
+      start_time: 'start_time',
+      end_time: 'end_time',
+    };
+
+    it('returns a dashboard response', () => {
+      mock.onGet(prometheusEndpoint).reply(statusCodes.OK, response);
+
+      return getPrometheusQueryData(prometheusEndpoint, params).then(data => {
+        expect(data).toEqual(response.data);
+      });
+    });
+
+    it('returns a dashboard response after retrying twice', () => {
+      // Mock multiple attempts while the cache is filling up
+      mock.onGet(prometheusEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(prometheusEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(prometheusEndpoint).reply(statusCodes.OK, response); // 3rd attempt
+
+      return getPrometheusQueryData(prometheusEndpoint, params).then(data => {
+        expect(data).toEqual(response.data);
+        expect(mock.history.get).toHaveLength(3);
+      });
+    });
+
+    it('rejects after getting an HTTP 500 error', () => {
+      mock.onGet(prometheusEndpoint).reply(500, {
+        status: 'error',
+        error: 'An error ocurred',
+      });
+
+      return getPrometheusQueryData(prometheusEndpoint, params).catch(error => {
+        expect(error).toEqual(new Error('Request failed with status code 500'));
+      });
+    });
+
+    it('rejects after retrying twice and getting an HTTP 401 error', () => {
+      // Mock multiple attempts while the cache is filling up and fails
+      mock.onGet(prometheusEndpoint).reply(statusCodes.UNAUTHORIZED, {
+        status: 'error',
+        error: 'An error ocurred',
+      });
+
+      return getPrometheusQueryData(prometheusEndpoint, params).catch(error => {
+        expect(error).toEqual(new Error('Request failed with status code 401'));
+      });
+    });
+
+    it('rejects after retrying twice and getting an HTTP 500 error', () => {
+      // Mock multiple attempts while the cache is filling up and fails
+      mock.onGet(prometheusEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(prometheusEndpoint).replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet(prometheusEndpoint).reply(500, {
+        status: 'error',
+        error: 'An error ocurred',
+      }); // 3rd attempt
+
+      return getPrometheusQueryData(prometheusEndpoint, params).catch(error => {
+        expect(error).toEqual(new Error('Request failed with status code 500'));
+        expect(mock.history.get).toHaveLength(3);
+      });
+    });
+
+    test.each`
+      code                                | reason
+      ${statusCodes.BAD_REQUEST}          | ${'Parameters are missing or incorrect'}
+      ${statusCodes.UNPROCESSABLE_ENTITY} | ${"Expression can't be executed"}
+      ${statusCodes.SERVICE_UNAVAILABLE}  | ${'Query timed out or aborted'}
+    `('rejects with details: "$reason" after getting an HTTP $code error', ({ code, reason }) => {
+      mock.onGet(prometheusEndpoint).reply(code, {
+        status: 'error',
+        error: reason,
+      });
+
+      return getPrometheusQueryData(prometheusEndpoint, params).catch(error => {
+        expect(error).toEqual(new Error(reason));
+        expect(mock.history.get).toHaveLength(1);
+      });
+    });
+  });
+});

spec/frontend/monitoring/store/actions_spec.js

@@ -8,6 +8,7 @@ import createFlash from '~/flash';
 import { defaultTimeRange } from '~/vue_shared/constants';
 import * as getters from '~/monitoring/stores/getters';
 import { ENVIRONMENT_AVAILABLE_STATE } from '~/monitoring/constants';
+import { backoffMockImplementation } from 'jest/helpers/backoff_helper';
 
 import { createStore } from '~/monitoring/stores';
 import * as types from '~/monitoring/stores/mutation_types';
@@ -73,19 +74,7 @@ describe('Monitoring store actions', () => {
     commit = jest.fn();
     dispatch = jest.fn();
 
-    jest.spyOn(commonUtils, 'backOff').mockImplementation(callback => {
-      const q = new Promise((resolve, reject) => {
-        const stop = arg => (arg instanceof Error ? reject(arg) : resolve(arg));
-        const next = () => callback(next, stop);
-        // Define a timeout based on a mock timer
-        setTimeout(() => {
-          callback(next, stop);
-        });
-      });
-      // Run all resolved promises in chain
-      jest.runOnlyPendingTimers();
-      return q;
-    });
+    jest.spyOn(commonUtils, 'backOff').mockImplementation(backoffMockImplementation);
   });
 
   afterEach(() => {
@@ -483,7 +472,6 @@ describe('Monitoring store actions', () => {
         ],
         [],
         () => {
-          expect(mock.history.get).toHaveLength(1);
           done();
         },
       ).catch(done.fail);
@@ -569,46 +557,8 @@ describe('Monitoring store actions', () => {
       });
     });
 
-    it('commits result, when waiting for results', done => {
-      // Mock multiple attempts while the cache is filling up
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).reply(200, { data }); // 4th attempt
-
-      testAction(
-        fetchPrometheusMetric,
-        { metric, defaultQueryParams },
-        state,
-        [
-          {
-            type: types.REQUEST_METRIC_RESULT,
-            payload: {
-              metricId: metric.metricId,
-            },
-          },
-          {
-            type: types.RECEIVE_METRIC_RESULT_SUCCESS,
-            payload: {
-              metricId: metric.metricId,
-              data,
-            },
-          },
-        ],
-        [],
-        () => {
-          expect(mock.history.get).toHaveLength(4);
-          done();
-        },
-      ).catch(done.fail);
-    });
-
     it('commits failure, when waiting for results and getting a server error', done => {
-      // Mock multiple attempts while the cache is filling up and fails
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).replyOnce(statusCodes.NO_CONTENT);
-      mock.onGet(prometheusEndpointPath).reply(500); // 4th attempt
+      mock.onGet(prometheusEndpointPath).reply(500);
 
       const error = new Error('Request failed with status code 500');
 
@@ -633,7 +583,6 @@ describe('Monitoring store actions', () => {
         ],
         [],
       ).catch(e => {
-        expect(mock.history.get).toHaveLength(4);
         expect(e).toEqual(error);
         done();
       });