Merge branch 'no-double-metrics' into 'master'

Pipeline Graph Metrics: Collect Perf Data in Wrapper

See merge request gitlab-org/gitlab!60713

app/assets/javascripts/pipelines/components/graph_shared/drawing_utils.js

@@ -15,6 +15,7 @@ export const createUniqueLinkId = (stageName, jobName) => `${stageName}-${jobNam
 
 export const generateLinksData = ({ links }, containerID, modifier = '') => {
   const containerEl = document.getElementById(containerID);
+
   return links.map((link) => {
     const path = d3.path();
 

app/assets/javascripts/pipelines/components/graph_shared/links_inner.vue

 <script>
 import { isEmpty } from 'lodash';
-import {
-  PIPELINES_DETAIL_LINKS_MARK_CALCULATE_START,
-  PIPELINES_DETAIL_LINKS_MARK_CALCULATE_END,
-  PIPELINES_DETAIL_LINKS_MEASURE_CALCULATION,
-  PIPELINES_DETAIL_LINK_DURATION,
-  PIPELINES_DETAIL_LINKS_TOTAL,
-  PIPELINES_DETAIL_LINKS_JOB_RATIO,
-} from '~/performance/constants';
-import { performanceMarkAndMeasure } from '~/performance/utils';
 import { DRAW_FAILURE } from '../../constants';
 import { createJobsHash, generateJobNeedsDict, reportToSentry } from '../../utils';
 import { STAGE_VIEW } from '../graph/constants';
-import { parseData } from '../parsing_utils';
-import { reportPerformance } from './api';
 import { generateLinksData } from './drawing_utils';
 
 export default {
@@ -28,6 +17,10 @@ export default {
       type: Object,
       required: true,
     },
+    parsedData: {
+      type: Object,
+      required: true,
+    },
     pipelineId: {
       type: Number,
       required: true,
@@ -36,15 +29,6 @@ export default {
       type: Array,
       required: true,
     },
-    totalGroups: {
-      type: Number,
-      required: true,
-    },
-    metricsConfig: {
-      type: Object,
-      required: false,
-      default: () => ({}),
-    },
     defaultLinkColor: {
       type: String,
       required: false,
@@ -65,13 +49,9 @@ export default {
     return {
       links: [],
       needsObject: null,
-      parsedData: {},
     };
   },
   computed: {
-    shouldCollectMetrics() {
-      return this.metricsConfig.collectMetrics && this.metricsConfig.path;
-    },
     hasHighlightedJob() {
       return Boolean(this.highlightedJob);
     },
@@ -115,13 +95,16 @@ export default {
     highlightedJobs(jobs) {
       this.$emit('highlightedJobsChange', jobs);
     },
+    parsedData() {
+      this.calculateLinkData();
+    },
     viewType() {
       /*
         We need to wait a tick so that the layout reflows
         before the links refresh.
       */
       this.$nextTick(() => {
-        this.refreshLinks();
+        this.calculateLinkData();
       });
     },
   },
@@ -129,69 +112,21 @@ export default {
     reportToSentry(this.$options.name, `error: ${err}, info: ${info}`);
   },
   mounted() {
-    if (!isEmpty(this.pipelineData)) {
-      this.prepareLinkData();
+    if (!isEmpty(this.parsedData)) {
+      this.calculateLinkData();
     }
   },
   methods: {
-    beginPerfMeasure() {
-      if (this.shouldCollectMetrics) {
-        performanceMarkAndMeasure({ mark: PIPELINES_DETAIL_LINKS_MARK_CALCULATE_START });
-      }
-    },
-    finishPerfMeasureAndSend() {
-      if (this.shouldCollectMetrics) {
-        performanceMarkAndMeasure({
-          mark: PIPELINES_DETAIL_LINKS_MARK_CALCULATE_END,
-          measures: [
-            {
-              name: PIPELINES_DETAIL_LINKS_MEASURE_CALCULATION,
-              start: PIPELINES_DETAIL_LINKS_MARK_CALCULATE_START,
-            },
-          ],
-        });
-      }
-
-      window.requestAnimationFrame(() => {
-        const duration = window.performance.getEntriesByName(
-          PIPELINES_DETAIL_LINKS_MEASURE_CALCULATION,
-        )[0]?.duration;
-
-        if (!duration) {
-          return;
-        }
-
-        const data = {
-          histograms: [
-            { name: PIPELINES_DETAIL_LINK_DURATION, value: duration / 1000 },
-            { name: PIPELINES_DETAIL_LINKS_TOTAL, value: this.links.length },
-            {
-              name: PIPELINES_DETAIL_LINKS_JOB_RATIO,
-              value: this.links.length / this.totalGroups,
-            },
-          ],
-        };
-
-        reportPerformance(this.metricsConfig.path, data);
-      });
-    },
     isLinkHighlighted(linkRef) {
       return this.highlightedLinks.includes(linkRef);
     },
-    prepareLinkData() {
-      this.beginPerfMeasure();
+    calculateLinkData() {
       try {
-        const arrayOfJobs = this.pipelineData.flatMap(({ groups }) => groups);
-        this.parsedData = parseData(arrayOfJobs);
-        this.refreshLinks();
+        this.links = generateLinksData(this.parsedData, this.containerId, `-${this.pipelineId}`);
       } catch (err) {
         this.$emit('error', { type: DRAW_FAILURE, reportToSentry: false });
         reportToSentry(this.$options.name, err);
       }
-      this.finishPerfMeasureAndSend();
-    },
-    refreshLinks() {
-      this.links = generateLinksData(this.parsedData, this.containerId, `-${this.pipelineId}`);
     },
     getLinkClasses(link) {
       return [

app/assets/javascripts/pipelines/components/graph_shared/links_layer.vue

@@ -43,6 +43,7 @@ export default {
   data() {
     return {
       alertDismissed: false,
+      parsedData: {},
       showLinksOverride: false,
     };
   },
@@ -72,14 +73,7 @@ export default {
     reportToSentry(this.$options.name, `error: ${err}, info: ${info}`);
   },
   mounted() {
-    /*
-      This is code to get metrics for the graph (to observe links performance).
-      It is currently here because we want values for links without drawing them.
-      It can be removed when https://gitlab.com/gitlab-org/gitlab/-/issues/298930
-      is closed and functionality is enabled by default.
-    */
-
-    if (!this.showLinks && !isEmpty(this.pipelineData)) {
+    if (!isEmpty(this.pipelineData)) {
       window.requestAnimationFrame(() => {
         this.prepareLinkData();
       });
@@ -132,7 +126,8 @@ export default {
       let numLinks;
       try {
         const arrayOfJobs = this.pipelineData.flatMap(({ groups }) => groups);
-        numLinks = parseData(arrayOfJobs).links.length;
+        this.parsedData = parseData(arrayOfJobs);
+        numLinks = this.parsedData.links.length;
       } catch (err) {
         reportToSentry(this.$options.name, err);
       }
@@ -145,9 +140,9 @@ export default {
   <links-inner
     v-if="showLinkedLayers"
     :container-measurements="containerMeasurements"
+    :parsed-data="parsedData"
     :pipeline-data="pipelineData"
     :total-groups="numGroups"
-    :metrics-config="metricsConfig"
     v-bind="$attrs"
     v-on="$listeners"
   >

spec/frontend/pipelines/graph_shared/__snapshots__/links_inner_spec.js.snap

 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`Links Inner component with a large number of needs matches snapshot and has expected path 1`] = `
-"<div class=\\"gl-display-flex gl-relative\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
+"<div class=\\"gl-display-flex gl-relative\\" totalgroups=\\"10\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
     <path d=\\"M202,118L42,118C72,118,72,138,102,138\\" stroke-width=\\"2\\" class=\\"gl-fill-transparent gl-transition-duration-slow gl-transition-timing-function-ease gl-stroke-gray-200\\"></path>
     <path d=\\"M202,118L52,118C82,118,82,148,112,148\\" stroke-width=\\"2\\" class=\\"gl-fill-transparent gl-transition-duration-slow gl-transition-timing-function-ease gl-stroke-gray-200\\"></path>
     <path d=\\"M222,138L62,138C92,138,92,158,122,158\\" stroke-width=\\"2\\" class=\\"gl-fill-transparent gl-transition-duration-slow gl-transition-timing-function-ease gl-stroke-gray-200\\"></path>
@@ -11,13 +11,13 @@ exports[`Links Inner component with a large number of needs matches snapshot and
 `;
 
 exports[`Links Inner component with a parallel need matches snapshot and has expected path 1`] = `
-"<div class=\\"gl-display-flex gl-relative\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
+"<div class=\\"gl-display-flex gl-relative\\" totalgroups=\\"10\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
     <path d=\\"M192,108L22,108C52,108,52,118,82,118\\" stroke-width=\\"2\\" class=\\"gl-fill-transparent gl-transition-duration-slow gl-transition-timing-function-ease gl-stroke-gray-200\\"></path>
   </svg> </div>"
 `;
 
 exports[`Links Inner component with one need matches snapshot and has expected path 1`] = `
-"<div class=\\"gl-display-flex gl-relative\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
+"<div class=\\"gl-display-flex gl-relative\\" totalgroups=\\"10\\"><svg id=\\"link-svg\\" viewBox=\\"0,0,1019,445\\" width=\\"1019px\\" height=\\"445px\\" class=\\"gl-absolute gl-pointer-events-none\\">
     <path d=\\"M202,118L42,118C72,118,72,138,102,138\\" stroke-width=\\"2\\" class=\\"gl-fill-transparent gl-transition-duration-slow gl-transition-timing-function-ease gl-stroke-gray-200\\"></path>
   </svg> </div>"
 `;

spec/frontend/pipelines/graph_shared/links_inner_spec.js

 import { shallowMount } from '@vue/test-utils';
-import MockAdapter from 'axios-mock-adapter';
 import { setHTMLFixture } from 'helpers/fixtures';
-import axios from '~/lib/utils/axios_utils';
-import {
-  PIPELINES_DETAIL_LINK_DURATION,
-  PIPELINES_DETAIL_LINKS_TOTAL,
-  PIPELINES_DETAIL_LINKS_JOB_RATIO,
-} from '~/performance/constants';
-import * as perfUtils from '~/performance/utils';
-import * as Api from '~/pipelines/components/graph_shared/api';
 import LinksInner from '~/pipelines/components/graph_shared/links_inner.vue';
-import * as sentryUtils from '~/pipelines/utils';
+import { parseData } from '~/pipelines/components/parsing_utils';
 import { createJobsHash } from '~/pipelines/utils';
 import {
   jobRect,
@@ -34,8 +25,13 @@ describe('Links Inner component', () => {
   let wrapper;
 
   const createComponent = (props) => {
+    const currentPipelineData = props?.pipelineData || defaultProps.pipelineData;
     wrapper = shallowMount(LinksInner, {
-      propsData: { ...defaultProps, ...props },
+      propsData: {
+        ...defaultProps,
+        ...props,
+        parsedData: parseData(currentPipelineData.flatMap(({ groups }) => groups)),
+      },
     });
   };
 
@@ -206,141 +202,4 @@ describe('Links Inner component', () => {
       expect(firstLink.classes(hoverColorClass)).toBe(true);
     });
   });
-
-  describe('performance metrics', () => {
-    let markAndMeasure;
-    let reportToSentry;
-    let reportPerformance;
-    let mock;
-
-    beforeEach(() => {
-      mock = new MockAdapter(axios);
-      jest.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => cb());
-      markAndMeasure = jest.spyOn(perfUtils, 'performanceMarkAndMeasure');
-      reportToSentry = jest.spyOn(sentryUtils, 'reportToSentry');
-      reportPerformance = jest.spyOn(Api, 'reportPerformance');
-    });
-
-    afterEach(() => {
-      mock.restore();
-    });
-
-    describe('with no metrics config object', () => {
-      beforeEach(() => {
-        setFixtures(pipelineData);
-        createComponent({
-          pipelineData: pipelineData.stages,
-        });
-      });
-
-      it('is not called', () => {
-        expect(markAndMeasure).not.toHaveBeenCalled();
-        expect(reportToSentry).not.toHaveBeenCalled();
-        expect(reportPerformance).not.toHaveBeenCalled();
-      });
-    });
-
-    describe('with metrics config set to false', () => {
-      beforeEach(() => {
-        setFixtures(pipelineData);
-        createComponent({
-          pipelineData: pipelineData.stages,
-          metricsConfig: {
-            collectMetrics: false,
-            metricsPath: '/path/to/metrics',
-          },
-        });
-      });
-
-      it('is not called', () => {
-        expect(markAndMeasure).not.toHaveBeenCalled();
-        expect(reportToSentry).not.toHaveBeenCalled();
-        expect(reportPerformance).not.toHaveBeenCalled();
-      });
-    });
-
-    describe('with no metrics path', () => {
-      beforeEach(() => {
-        setFixtures(pipelineData);
-        createComponent({
-          pipelineData: pipelineData.stages,
-          metricsConfig: {
-            collectMetrics: true,
-            metricsPath: '',
-          },
-        });
-      });
-
-      it('is not called', () => {
-        expect(markAndMeasure).not.toHaveBeenCalled();
-        expect(reportToSentry).not.toHaveBeenCalled();
-        expect(reportPerformance).not.toHaveBeenCalled();
-      });
-    });
-
-    describe('with metrics path and collect set to true', () => {
-      const metricsPath = '/root/project/-/ci/prometheus_metrics/histograms.json';
-      const duration = 0.0478;
-      const numLinks = 1;
-      const metricsData = {
-        histograms: [
-          { name: PIPELINES_DETAIL_LINK_DURATION, value: duration / 1000 },
-          { name: PIPELINES_DETAIL_LINKS_TOTAL, value: numLinks },
-          {
-            name: PIPELINES_DETAIL_LINKS_JOB_RATIO,
-            value: numLinks / defaultProps.totalGroups,
-          },
-        ],
-      };
-
-      describe('when no duration is obtained', () => {
-        beforeEach(() => {
-          jest.spyOn(window.performance, 'getEntriesByName').mockImplementation(() => {
-            return [];
-          });
-
-          setFixtures(pipelineData);
-
-          createComponent({
-            pipelineData: pipelineData.stages,
-            metricsConfig: {
-              collectMetrics: true,
-              path: metricsPath,
-            },
-          });
-        });
-
-        it('attempts to collect metrics', () => {
-          expect(markAndMeasure).toHaveBeenCalled();
-          expect(reportPerformance).not.toHaveBeenCalled();
-          expect(reportToSentry).not.toHaveBeenCalled();
-        });
-      });
-
-      describe('with duration and no error', () => {
-        beforeEach(() => {
-          jest.spyOn(window.performance, 'getEntriesByName').mockImplementation(() => {
-            return [{ duration }];
-          });
-
-          setFixtures(pipelineData);
-
-          createComponent({
-            pipelineData: pipelineData.stages,
-            metricsConfig: {
-              collectMetrics: true,
-              path: metricsPath,
-            },
-          });
-        });
-
-        it('it calls reportPerformance with expected arguments', () => {
-          expect(markAndMeasure).toHaveBeenCalled();
-          expect(reportPerformance).toHaveBeenCalled();
-          expect(reportPerformance).toHaveBeenCalledWith(metricsPath, metricsData);
-          expect(reportToSentry).not.toHaveBeenCalled();
-        });
-      });
-    });
-  });
 });

spec/frontend/pipelines/graph_shared/links_layer_spec.js

 import { shallowMount } from '@vue/test-utils';
+import MockAdapter from 'axios-mock-adapter';
+import axios from '~/lib/utils/axios_utils';
+import {
+  PIPELINES_DETAIL_LINK_DURATION,
+  PIPELINES_DETAIL_LINKS_TOTAL,
+  PIPELINES_DETAIL_LINKS_JOB_RATIO,
+} from '~/performance/constants';
+import * as perfUtils from '~/performance/utils';
+import * as Api from '~/pipelines/components/graph_shared/api';
 import LinksInner from '~/pipelines/components/graph_shared/links_inner.vue';
 import LinksLayer from '~/pipelines/components/graph_shared/links_layer.vue';
+import * as sentryUtils from '~/pipelines/utils';
 import { generateResponse, mockPipelineResponse } from '../graph/mock_data';
 
 describe('links layer component', () => {
@@ -37,7 +47,6 @@ describe('links layer component', () => {
 
   afterEach(() => {
     wrapper.destroy();
-    wrapper = null;
   });
 
   describe('with show links off', () => {
@@ -85,4 +94,137 @@ describe('links layer component', () => {
       expect(findLinksInner().exists()).toBe(false);
     });
   });
+
+  describe('performance metrics', () => {
+    let markAndMeasure;
+    let reportToSentry;
+    let reportPerformance;
+    let mock;
+
+    beforeEach(() => {
+      mock = new MockAdapter(axios);
+      jest.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => cb());
+      markAndMeasure = jest.spyOn(perfUtils, 'performanceMarkAndMeasure');
+      reportToSentry = jest.spyOn(sentryUtils, 'reportToSentry');
+      reportPerformance = jest.spyOn(Api, 'reportPerformance');
+    });
+
+    afterEach(() => {
+      mock.restore();
+    });
+
+    describe('with no metrics config object', () => {
+      beforeEach(() => {
+        createComponent();
+      });
+
+      it('is not called', () => {
+        expect(markAndMeasure).not.toHaveBeenCalled();
+        expect(reportToSentry).not.toHaveBeenCalled();
+        expect(reportPerformance).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('with metrics config set to false', () => {
+      beforeEach(() => {
+        createComponent({
+          props: {
+            metricsConfig: {
+              collectMetrics: false,
+              metricsPath: '/path/to/metrics',
+            },
+          },
+        });
+      });
+
+      it('is not called', () => {
+        expect(markAndMeasure).not.toHaveBeenCalled();
+        expect(reportToSentry).not.toHaveBeenCalled();
+        expect(reportPerformance).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('with no metrics path', () => {
+      beforeEach(() => {
+        createComponent({
+          props: {
+            metricsConfig: {
+              collectMetrics: true,
+              metricsPath: '',
+            },
+          },
+        });
+      });
+
+      it('is not called', () => {
+        expect(markAndMeasure).not.toHaveBeenCalled();
+        expect(reportToSentry).not.toHaveBeenCalled();
+        expect(reportPerformance).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('with metrics path and collect set to true', () => {
+      const metricsPath = '/root/project/-/ci/prometheus_metrics/histograms.json';
+      const duration = 875;
+      const numLinks = 7;
+      const totalGroups = 8;
+      const metricsData = {
+        histograms: [
+          { name: PIPELINES_DETAIL_LINK_DURATION, value: duration / 1000 },
+          { name: PIPELINES_DETAIL_LINKS_TOTAL, value: numLinks },
+          {
+            name: PIPELINES_DETAIL_LINKS_JOB_RATIO,
+            value: numLinks / totalGroups,
+          },
+        ],
+      };
+
+      describe('when no duration is obtained', () => {
+        beforeEach(() => {
+          jest.spyOn(window.performance, 'getEntriesByName').mockImplementation(() => {
+            return [];
+          });
+
+          createComponent({
+            props: {
+              metricsConfig: {
+                collectMetrics: true,
+                path: metricsPath,
+              },
+            },
+          });
+        });
+
+        it('attempts to collect metrics', () => {
+          expect(markAndMeasure).toHaveBeenCalled();
+          expect(reportPerformance).not.toHaveBeenCalled();
+          expect(reportToSentry).not.toHaveBeenCalled();
+        });
+      });
+
+      describe('with duration and no error', () => {
+        beforeEach(() => {
+          jest.spyOn(window.performance, 'getEntriesByName').mockImplementation(() => {
+            return [{ duration }];
+          });
+
+          createComponent({
+            props: {
+              metricsConfig: {
+                collectMetrics: true,
+                path: metricsPath,
+              },
+            },
+          });
+        });
+
+        it('it calls reportPerformance with expected arguments', () => {
+          expect(markAndMeasure).toHaveBeenCalled();
+          expect(reportPerformance).toHaveBeenCalled();
+          expect(reportPerformance).toHaveBeenCalledWith(metricsPath, metricsData);
+          expect(reportToSentry).not.toHaveBeenCalled();
+        });
+      });
+    });
+  });
 });

spec/frontend/pipelines/pipeline_graph/pipeline_graph_spec.js

 import { GlAlert } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
+import { setHTMLFixture } from 'helpers/fixtures';
 import { CI_CONFIG_STATUS_VALID } from '~/pipeline_editor/constants';
 import LinksInner from '~/pipelines/components/graph_shared/links_inner.vue';
 import LinksLayer from '~/pipelines/components/graph_shared/links_layer.vue';
 import JobPill from '~/pipelines/components/pipeline_graph/job_pill.vue';
 import PipelineGraph from '~/pipelines/components/pipeline_graph/pipeline_graph.vue';
 import StagePill from '~/pipelines/components/pipeline_graph/stage_pill.vue';
-import { DRAW_FAILURE } from '~/pipelines/constants';
-import { invalidNeedsData, pipelineData, singleStageData } from './mock_data';
+import { pipelineData, singleStageData } from './mock_data';
 
 describe('pipeline graph component', () => {
   const defaultProps = { pipelineData };
   let wrapper;
 
+  const containerId = 'pipeline-graph-container-0';
+  setHTMLFixture(`<div id="${containerId}"></div>`);
+
   const createComponent = (props = defaultProps) => {
     return shallowMount(PipelineGraph, {
       propsData: {
@@ -55,18 +58,7 @@ describe('pipeline graph component', () => {
     it('renders the graph with no status error', () => {
       expect(findAlert().exists()).toBe(false);
       expect(findPipelineGraph().exists()).toBe(true);
-    });
-  });
-
-  describe('with error while rendering the links with needs', () => {
-    beforeEach(() => {
-      wrapper = createComponent({ pipelineData: invalidNeedsData });
-    });
-
-    it('renders the error that link could not be drawn', () => {
       expect(findLinksLayer().exists()).toBe(true);
-      expect(findAlert().exists()).toBe(true);
-      expect(findAlert().text()).toBe(wrapper.vm.$options.errorTexts[DRAW_FAILURE]);
     });
   });