Merge branch '214627-fix-incorrect-csv-export' into 'master'

Fix CSV downloads for multiple series in the same chart

Closes #214627

See merge request gitlab-org/gitlab!36556

app/assets/javascripts/helpers/monitor_helper.js

@@ -49,7 +49,7 @@ const multiMetricLabel = metricAttributes => {
  * @param {Object} metricAttributes - Default metric attribute values (e.g. method, instance)
  * @returns {String} The formatted query label
  */
-const getSeriesLabel = (queryLabel, metricAttributes) => {
+export const getSeriesLabel = (queryLabel, metricAttributes) => {
   return (
     singleAttributeLabel(queryLabel, metricAttributes) ||
     templatedLabel(queryLabel, metricAttributes) ||
@@ -63,7 +63,6 @@ const getSeriesLabel = (queryLabel, metricAttributes) => {
  * @param {Object} defaultConfig - Default chart config values (e.g. lineStyle, name)
  * @returns {Array} The formatted values
  */
-// eslint-disable-next-line import/prefer-default-export
 export const makeDataSeries = (queryResults, defaultConfig) =>
   queryResults.map(result => {
     return {

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

@@ -30,6 +30,7 @@ import MonitorStackedColumnChart from './charts/stacked_column.vue';
 import TrackEventDirective from '~/vue_shared/directives/track_event';
 import AlertWidget from './alert_widget.vue';
 import { timeRangeToUrl, downloadCSVOptions, generateLinkToChartOptions } from '../utils';
+import { graphDataToCsv } from '../csv_export';
 
 const events = {
   timeRangeZoom: 'timerangezoom',
@@ -148,13 +149,10 @@ export default {
       return null;
     },
     csvText() {
-      const chartData = this.graphData?.metrics[0].result[0].values || [];
-      const yLabel = this.graphData.y_label;
-      const header = `timestamp,${yLabel}\r\n`; // eslint-disable-line @gitlab/require-i18n-strings
-      return chartData.reduce((csv, data) => {
-        const row = data.join(',');
-        return `${csv}${row}\r\n`;
-      }, header);
+      if (this.graphData) {
+        return graphDataToCsv(this.graphData);
+      }
+      return null;
     },
     downloadCsv() {
       const data = new Blob([this.csvText], { type: 'text/plain' });

app/assets/javascripts/monitoring/csv_export.js

+import { getSeriesLabel } from '~/helpers/monitor_helper';
+
+/**
+ * Returns a label for a header of the csv.
+ *
+ * Includes double quotes ("") in case the header includes commas or other separator.
+ *
+ * @param {String} axisLabel
+ * @param {String} metricLabel
+ * @param {Object} metricAttributes
+ */
+const csvHeader = (axisLabel, metricLabel, metricAttributes = {}) =>
+  `${axisLabel} > ${getSeriesLabel(metricLabel, metricAttributes)}`;
+
+/**
+ * Returns an array with the header labels given a list of metrics
+ *
+ * ```
+ * metrics = [
+ *   {
+ *      label: "..." // user-defined label
+ *      result: [
+ *        {
+ *           metric: { ... } // metricAttributes
+ *        },
+ *        ...
+ *      ]
+ *   },
+ *   ...
+ * ]
+ * ```
+ *
+ * When metrics have a `label` or `metricAttributes`, they are
+ * used to generate the column name.
+ *
+ * @param {String} axisLabel - Main label
+ * @param {Array} metrics - Metrics with results
+ */
+const csvMetricHeaders = (axisLabel, metrics) =>
+  metrics.flatMap(({ label, result }) =>
+    // The `metric` in a `result` is a map of `metricAttributes`
+    // contains key-values to identify the series, rename it
+    // here for clarity.
+    result.map(({ metric: metricAttributes }) => {
+      return csvHeader(axisLabel, label, metricAttributes);
+    }),
+  );
+
+/**
+ * Returns a (flat) array with all the values arrays in each
+ * metric and series
+ *
+ * ```
+ * metrics = [
+ *   {
+ *      result: [
+ *        {
+ *           values: [ ... ] // `values`
+ *        },
+ *        ...
+ *      ]
+ *   },
+ *   ...
+ * ]
+ * ```
+ *
+ * @param {Array} metrics - Metrics with results
+ */
+const csvMetricValues = metrics =>
+  metrics.flatMap(({ result }) => result.map(res => res.values || []));
+
+/**
+ * Returns headers and rows for csv, sorted by their timestamp.
+ *
+ * {
+ *   headers: ["timestamp", "<col_1_name>", "col_2_name"],
+ *   rows: [
+ *     [ <timestamp>, <col_1_value>, <col_2_value> ],
+ *     [ <timestamp>, <col_1_value>, <col_2_value> ]
+ *     ...
+ *   ]
+ * }
+ *
+ * @param {Array} metricHeaders
+ * @param {Array} metricValues
+ */
+const csvData = (metricHeaders, metricValues) => {
+  const rowsByTimestamp = {};
+
+  metricValues.forEach((values, colIndex) => {
+    values.forEach(([timestamp, value]) => {
+      if (!rowsByTimestamp[timestamp]) {
+        rowsByTimestamp[timestamp] = [];
+      }
+      // `value` should be in the right column
+      rowsByTimestamp[timestamp][colIndex] = value;
+    });
+  });
+
+  const rows = Object.keys(rowsByTimestamp)
+    .sort()
+    .map(timestamp => {
+      // force each row to have the same number of entries
+      rowsByTimestamp[timestamp].length = metricHeaders.length;
+      // add timestamp as the first entry
+      return [timestamp, ...rowsByTimestamp[timestamp]];
+    });
+
+  // Escape double quotes and enclose headers:
+  // "If double-quotes are used to enclose fields, then a double-quote
+  // appearing inside a field must be escaped by preceding it with
+  // another double quote."
+  // https://tools.ietf.org/html/rfc4180#page-2
+  const headers = metricHeaders.map(header => `"${header.replace(/"/g, '""')}"`);
+
+  return {
+    headers: ['timestamp', ...headers],
+    rows,
+  };
+};
+
+/**
+ * Returns dashboard panel's data in a string in CSV format
+ *
+ * @param {Object} graphData - Panel contents
+ * @returns {String}
+ */
+// eslint-disable-next-line import/prefer-default-export
+export const graphDataToCsv = graphData => {
+  const delimiter = ',';
+  const br = '\r\n';
+  const { metrics = [], y_label: axisLabel } = graphData;
+
+  const metricsWithResults = metrics.filter(metric => metric.result);
+  const metricHeaders = csvMetricHeaders(axisLabel, metricsWithResults);
+  const metricValues = csvMetricValues(metricsWithResults);
+  const { headers, rows } = csvData(metricHeaders, metricValues);
+
+  if (rows.length === 0) {
+    return '';
+  }
+
+  const headerLine = headers.join(delimiter) + br;
+  const lines = rows.map(row => row.join(delimiter));
+
+  return headerLine + lines.join(br) + br;
+};

changelogs/unreleased/214627-fix-incorrect-csv-export.yml

+---
+title: Fix CSV downloads for multiple series in the same chart
+merge_request: 36556
+author:
+type: fixed

spec/frontend/helpers/monitor_helper_spec.js

-import * as monitorHelper from '~/helpers/monitor_helper';
+import { getSeriesLabel, makeDataSeries } from '~/helpers/monitor_helper';
 
 describe('monitor helper', () => {
   const defaultConfig = { default: true, name: 'default name' };
   const name = 'data name';
   const series = [[1, 1], [2, 2], [3, 3]];
-  const data = ({ metric = { default_name: name }, values = series } = {}) => [{ metric, values }];
+
+  describe('getSeriesLabel', () => {
+    const metricAttributes = { __name__: 'up', app: 'prometheus' };
+
+    it('gets a single attribute label', () => {
+      expect(getSeriesLabel('app', metricAttributes)).toBe('app: prometheus');
+    });
+
+    it('gets a templated label', () => {
+      expect(getSeriesLabel('{{__name__}}', metricAttributes)).toBe('up');
+      expect(getSeriesLabel('{{app}}', metricAttributes)).toBe('prometheus');
+      expect(getSeriesLabel('{{missing}}', metricAttributes)).toBe('{{missing}}');
+    });
+
+    it('gets a multiple label', () => {
+      expect(getSeriesLabel(null, metricAttributes)).toBe('__name__: up, app: prometheus');
+      expect(getSeriesLabel('', metricAttributes)).toBe('__name__: up, app: prometheus');
+    });
+
+    it('gets a simple label', () => {
+      expect(getSeriesLabel('A label', {})).toBe('A label');
+    });
+  });
 
   describe('makeDataSeries', () => {
+    const data = ({ metric = { default_name: name }, values = series } = {}) => [
+      { metric, values },
+    ];
+
     const expectedDataSeries = [
       {
         ...defaultConfig,
@@ -15,19 +41,17 @@ describe('monitor helper', () => {
     ];
 
     it('converts query results to data series', () => {
-      expect(monitorHelper.makeDataSeries(data({ metric: {} }), defaultConfig)).toEqual(
-        expectedDataSeries,
-      );
+      expect(makeDataSeries(data({ metric: {} }), defaultConfig)).toEqual(expectedDataSeries);
     });
 
     it('returns an empty array if no query results exist', () => {
-      expect(monitorHelper.makeDataSeries([], defaultConfig)).toEqual([]);
+      expect(makeDataSeries([], defaultConfig)).toEqual([]);
     });
 
     it('handles multi-series query results', () => {
       const expectedData = { ...expectedDataSeries[0], name: 'default name: data name' };
 
-      expect(monitorHelper.makeDataSeries([...data(), ...data()], defaultConfig)).toEqual([
+      expect(makeDataSeries([...data(), ...data()], defaultConfig)).toEqual([
         expectedData,
         expectedData,
       ]);
@@ -39,10 +63,7 @@ describe('monitor helper', () => {
         name: '{{cmd}}',
       };
 
-      const [result] = monitorHelper.makeDataSeries(
-        [{ metric: { cmd: 'brpop' }, values: series }],
-        config,
-      );
+      const [result] = makeDataSeries([{ metric: { cmd: 'brpop' }, values: series }], config);
 
       expect(result.name).toEqual('brpop');
     });
@@ -53,7 +74,7 @@ describe('monitor helper', () => {
         name: '',
       };
 
-      const [result] = monitorHelper.makeDataSeries(
+      const [result] = makeDataSeries(
         [
           {
             metric: {
@@ -79,7 +100,7 @@ describe('monitor helper', () => {
         name: 'backend: {{ backend }}',
       };
 
-      const [result] = monitorHelper.makeDataSeries(
+      const [result] = makeDataSeries(
         [{ metric: { backend: 'HA Server' }, values: series }],
         config,
       );
@@ -90,10 +111,7 @@ describe('monitor helper', () => {
     it('supports repeated template variables', () => {
       const config = { ...defaultConfig, name: '{{cmd}}, {{cmd}}' };
 
-      const [result] = monitorHelper.makeDataSeries(
-        [{ metric: { cmd: 'brpop' }, values: series }],
-        config,
-      );
+      const [result] = makeDataSeries([{ metric: { cmd: 'brpop' }, values: series }], config);
 
       expect(result.name).toEqual('brpop, brpop');
     });
@@ -101,7 +119,7 @@ describe('monitor helper', () => {
     it('supports hyphenated template variables', () => {
       const config = { ...defaultConfig, name: 'expired - {{ test-attribute }}' };
 
-      const [result] = monitorHelper.makeDataSeries(
+      const [result] = makeDataSeries(
         [{ metric: { 'test-attribute': 'test-attribute-value' }, values: series }],
         config,
       );
@@ -115,7 +133,7 @@ describe('monitor helper', () => {
         name: '{{job}}: {{cmd}}',
       };
 
-      const [result] = monitorHelper.makeDataSeries(
+      const [result] = makeDataSeries(
         [{ metric: { cmd: 'brpop', job: 'redis' }, values: series }],
         config,
       );
@@ -129,7 +147,7 @@ describe('monitor helper', () => {
         name: '{{cmd}}',
       };
 
-      const [firstSeries, secondSeries] = monitorHelper.makeDataSeries(
+      const [firstSeries, secondSeries] = makeDataSeries(
         [
           { metric: { cmd: 'brpop' }, values: series },
           { metric: { cmd: 'zrangebyscore' }, values: series },

spec/frontend/monitoring/components/dashboard_panel_spec.js

@@ -444,7 +444,7 @@ describe('Dashboard Panel', () => {
 
     describe('csvText', () => {
       it('converts metrics data from json to csv', () => {
-        const header = `timestamp,${graphData.y_label}`;
+        const header = `timestamp,"${graphData.y_label} > ${graphData.metrics[0].label}"`;
         const data = graphData.metrics[0].result[0].values;
         const firstRow = `${data[0][0]},${data[0][1]}`;
         const secondRow = `${data[1][0]},${data[1][1]}`;

spec/frontend/monitoring/csv_export_spec.js

+import { timeSeriesGraphData } from './graph_data';
+import { graphDataToCsv } from '~/monitoring/csv_export';
+
+describe('monitoring export_csv', () => {
+  describe('graphDataToCsv', () => {
+    const expectCsvToMatchLines = (csv, lines) => expect(`${lines.join('\r\n')}\r\n`).toEqual(csv);
+
+    it('should return a csv with 0 metrics', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 0 });
+
+      expect(graphDataToCsv(data)).toEqual('');
+    });
+
+    it('should return a csv with 1 metric with no data', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 1 });
+
+      // When state is NO_DATA, result is null
+      data.metrics[0].result = null;
+
+      expect(graphDataToCsv(data)).toEqual('');
+    });
+
+    it('should return a csv with multiple metrics and one with no data', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 2 });
+
+      // When state is NO_DATA, result is null
+      data.metrics[0].result = null;
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > Metric 2"`,
+        '2015-07-01T20:10:51.781Z,1',
+        '2015-07-01T20:11:06.781Z,2',
+        '2015-07-01T20:11:21.781Z,3',
+      ]);
+    });
+
+    it('should return a csv when not all metrics have the same timestamps', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 3 });
+
+      // Add an "odd" timestamp that is not in the dataset
+      Object.assign(data.metrics[2].result[0], {
+        value: ['2016-01-01T00:00:00.000Z', 9],
+        values: [['2016-01-01T00:00:00.000Z', 9]],
+      });
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > Metric 1","Y Axis > Metric 2","Y Axis > Metric 3"`,
+        '2015-07-01T20:10:51.781Z,1,1,',
+        '2015-07-01T20:11:06.781Z,2,2,',
+        '2015-07-01T20:11:21.781Z,3,3,',
+        '2016-01-01T00:00:00.000Z,,,9',
+      ]);
+    });
+
+    it('should return a csv with 1 metric', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 1 });
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > Metric 1"`,
+        '2015-07-01T20:10:51.781Z,1',
+        '2015-07-01T20:11:06.781Z,2',
+        '2015-07-01T20:11:21.781Z,3',
+      ]);
+    });
+
+    it('should escape double quotes in metric labels with two double quotes ("")', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 1 });
+
+      data.metrics[0].label = 'My "quoted" metric';
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > My ""quoted"" metric"`,
+        '2015-07-01T20:10:51.781Z,1',
+        '2015-07-01T20:11:06.781Z,2',
+        '2015-07-01T20:11:21.781Z,3',
+      ]);
+    });
+
+    it('should return a csv with multiple metrics', () => {
+      const data = timeSeriesGraphData({}, { metricCount: 3 });
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > Metric 1","Y Axis > Metric 2","Y Axis > Metric 3"`,
+        '2015-07-01T20:10:51.781Z,1,1,1',
+        '2015-07-01T20:11:06.781Z,2,2,2',
+        '2015-07-01T20:11:21.781Z,3,3,3',
+      ]);
+    });
+
+    it('should return a csv with 1 metric and multiple series with labels', () => {
+      const data = timeSeriesGraphData({}, { isMultiSeries: true });
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > Metric 1","Y Axis > Metric 1"`,
+        '2015-07-01T20:10:51.781Z,1,4',
+        '2015-07-01T20:11:06.781Z,2,5',
+        '2015-07-01T20:11:21.781Z,3,6',
+      ]);
+    });
+
+    it('should return a csv with 1 metric and multiple series', () => {
+      const data = timeSeriesGraphData({}, { isMultiSeries: true, withLabels: false });
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > __name__: up, job: prometheus, instance: localhost:9090","Y Axis > __name__: up, job: node, instance: localhost:9091"`,
+        '2015-07-01T20:10:51.781Z,1,4',
+        '2015-07-01T20:11:06.781Z,2,5',
+        '2015-07-01T20:11:21.781Z,3,6',
+      ]);
+    });
+
+    it('should return a csv with multiple metrics and multiple series', () => {
+      const data = timeSeriesGraphData(
+        {},
+        { metricCount: 3, isMultiSeries: true, withLabels: false },
+      );
+
+      expectCsvToMatchLines(graphDataToCsv(data), [
+        `timestamp,"Y Axis > __name__: up, job: prometheus, instance: localhost:9090","Y Axis > __name__: up, job: node, instance: localhost:9091","Y Axis > __name__: up, job: prometheus, instance: localhost:9090","Y Axis > __name__: up, job: node, instance: localhost:9091","Y Axis > __name__: up, job: prometheus, instance: localhost:9090","Y Axis > __name__: up, job: node, instance: localhost:9091"`,
+        '2015-07-01T20:10:51.781Z,1,4,1,4,1,4',
+        '2015-07-01T20:11:06.781Z,2,5,2,5,2,5',
+        '2015-07-01T20:11:21.781Z,3,6,3,6,3,6',
+      ]);
+    });
+  });
+});

spec/frontend/monitoring/graph_data.js

@@ -82,7 +82,7 @@ const matrixMultiResult = ({ values1 = ['1', '2', '3'], values2 = ['4', '5', '6'
  * @param {Object} dataOptions.isMultiSeries
  */
 export const timeSeriesGraphData = (panelOptions = {}, dataOptions = {}) => {
-  const { metricCount = 1, isMultiSeries = false } = dataOptions;
+  const { metricCount = 1, isMultiSeries = false, withLabels = true } = dataOptions;
 
   return mapPanelToViewModel({
     title: 'Time Series Panel',
@@ -90,7 +90,7 @@ export const timeSeriesGraphData = (panelOptions = {}, dataOptions = {}) => {
     x_label: 'X Axis',
     y_label: 'Y Axis',
     metrics: Array.from(Array(metricCount), (_, i) => ({
-      label: `Metric ${i + 1}`,
+      label: withLabels ? `Metric ${i + 1}` : undefined,
       state: metricStates.OK,
       result: isMultiSeries ? matrixMultiResult() : matrixSingleResult(),
     })),