Merge branch 'show-performance-bar-warnings' into 'master'

Show performance bar warnings

See merge request gitlab-org/gitlab!17612

app/assets/javascripts/performance_bar/components/detailed_metric.vue

 <script>
+import RequestWarning from './request_warning.vue';
+
 import DeprecatedModal2 from '~/vue_shared/components/deprecated_modal_2.vue';
 import Icon from '~/vue_shared/components/icon.vue';
 
 export default {
   components: {
+    RequestWarning,
     GlModal: DeprecatedModal2,
     Icon,
   },
@@ -39,6 +42,16 @@ export default {
     detailsList() {
       return this.metricDetails.details;
     },
+    warnings() {
+      return this.metricDetails.warnings || [];
+    },
+    htmlId() {
+      if (this.currentRequest) {
+        return `performance-bar-warning-${this.currentRequest.id}-${this.metric}`;
+      }
+
+      return '';
+    },
   },
 };
 </script>
@@ -105,5 +118,6 @@ export default {
       <div slot="footer"></div>
     </gl-modal>
     {{ title }}
+    <request-warning :html-id="htmlId" :warnings="warnings" />
   </div>
 </template>

app/assets/javascripts/performance_bar/components/performance_bar_app.vue

 <script>
 import { glEmojiTag } from '~/emoji';
 
-import detailedMetric from './detailed_metric.vue';
-import requestSelector from './request_selector.vue';
+import DetailedMetric from './detailed_metric.vue';
+import RequestSelector from './request_selector.vue';
 import { s__ } from '~/locale';
 
 export default {
   components: {
-    detailedMetric,
-    requestSelector,
+    DetailedMetric,
+    RequestSelector,
   },
   props: {
     store: {

app/assets/javascripts/performance_bar/components/request_selector.vue

 <script>
+import { glEmojiTag } from '~/emoji';
+import { n__ } from '~/locale';
+import { GlPopover } from '@gitlab/ui';
+
 export default {
+  components: {
+    GlPopover,
+  },
   props: {
     currentRequest: {
       type: Object,
@@ -15,6 +22,18 @@ export default {
       currentRequestId: this.currentRequest.id,
     };
   },
+  computed: {
+    requestsWithWarnings() {
+      return this.requests.filter(request => request.hasWarnings);
+    },
+    warningMessage() {
+      return n__(
+        '%d request with warnings',
+        '%d requests with warnings',
+        this.requestsWithWarnings.length,
+      );
+    },
+  },
   watch: {
     currentRequestId(newRequestId) {
       this.$emit('change-current-request', newRequestId);
@@ -31,6 +50,7 @@ export default {
 
       return truncated;
     },
+    glEmojiTag,
   },
 };
 </script>
@@ -44,7 +64,16 @@ export default {
         class="qa-performance-bar-request"
       >
         {{ truncatedUrl(request.url) }}
+        <span v-if="request.hasWarnings">(!)</span>
       </option>
     </select>
+    <span v-if="requestsWithWarnings.length">
+      <span id="performance-bar-request-selector-warning" v-html="glEmojiTag('warning')"></span>
+      <gl-popover
+        target="performance-bar-request-selector-warning"
+        :content="warningMessage"
+        triggers="hover focus"
+      />
+    </span>
   </div>
 </template>

app/assets/javascripts/performance_bar/components/request_warning.vue

+<script>
+import { glEmojiTag } from '~/emoji';
+import { GlPopover } from '@gitlab/ui';
+
+export default {
+  components: {
+    GlPopover,
+  },
+  props: {
+    htmlId: {
+      type: String,
+      required: true,
+    },
+    warnings: {
+      type: Array,
+      required: true,
+    },
+  },
+  computed: {
+    hasWarnings() {
+      return this.warnings && this.warnings.length;
+    },
+    warningMessage() {
+      if (!this.hasWarnings) {
+        return '';
+      }
+
+      return this.warnings.join('\n');
+    },
+  },
+  methods: {
+    glEmojiTag,
+  },
+};
+</script>
+<template>
+  <span v-if="hasWarnings">
+    <span :id="htmlId" v-html="glEmojiTag('warning')"></span>
+    <gl-popover :target="htmlId" :content="warningMessage" triggers="hover focus" />
+  </span>
+</template>

app/assets/javascripts/performance_bar/index.js

@@ -6,7 +6,7 @@ export default ({ container }) =>
   new Vue({
     el: container,
     components: {
-      performanceBarApp: () => import('./components/performance_bar_app.vue'),
+      PerformanceBarApp: () => import('./components/performance_bar_app.vue'),
     },
     data() {
       const performanceBarData = document.querySelector(this.$options.el).dataset;
@@ -41,7 +41,7 @@ export default ({ container }) =>
 
         PerformanceBarService.fetchRequestDetails(this.peekUrl, requestId)
           .then(res => {
-            this.store.addRequestDetails(requestId, res.data.data);
+            this.store.addRequestDetails(requestId, res.data);
           })
           .catch(() =>
             // eslint-disable-next-line no-console

app/assets/javascripts/performance_bar/stores/performance_bar_store.js

@@ -3,12 +3,13 @@ export default class PerformanceBarStore {
     this.requests = [];
   }
 
-  addRequest(requestId, requestUrl, requestDetails) {
+  addRequest(requestId, requestUrl) {
     if (!this.findRequest(requestId)) {
       this.requests.push({
         id: requestId,
         url: requestUrl,
-        details: requestDetails,
+        details: {},
+        hasWarnings: false,
       });
     }
 
@@ -22,7 +23,8 @@ export default class PerformanceBarStore {
   addRequestDetails(requestId, requestDetails) {
     const request = this.findRequest(requestId);
 
-    request.details = requestDetails;
+    request.details = requestDetails.data;
+    request.hasWarnings = requestDetails.has_warnings;
 
     return request;
   }

changelogs/unreleased/performance-bar-warnings.yml

+---
+title: Add warnings to performance bar when page shows signs of poor performance
+merge_request: 17612
+author:
+type: changed

doc/administration/monitoring/performance/performance_bar.md

@@ -21,6 +21,24 @@ On the far right is a request selector that allows you to view the same metrics
 (excluding the page timing and line profiler) for any requests made while the
 page was open. Only the first two requests per unique URL are captured.
 
+## Request warnings
+
+For requests exceeding pre-defined limits, a warning icon will be shown
+next to the failing metric, along with an explanation. In this example,
+the Gitaly call duration exceeded the threshold:
+
+![Gitaly call duration exceeded threshold](img/performance_bar_gitaly_threshold.png)
+
+If any requests on the current page generated warnings, the icon will
+appear next to the request selector:
+
+![Request selector showing two requests with warnings](img/performance_bar_request_selector_warning.png)
+
+And requests with warnings are indicated in the request selector with a
+`(!)` after their path:
+
+![Request selector showing dropdown](img/performance_bar_request_selector_warning_expanded.png)
+
 ## Enable the Performance Bar via the Admin panel
 
 GitLab Performance Bar is disabled by default. To enable it for a given group,

locale/gitlab.pot

@@ -150,6 +150,11 @@ msgid_plural "%d more comments"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "%d request with warnings"
+msgid_plural "%d requests with warnings"
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "%d second"
 msgid_plural "%d seconds"
 msgstr[0] ""

spec/javascripts/performance_bar/components/detailed_metric_spec.js → spec/frontend/performance_bar/components/detailed_metric_spec.js

-import Vue from 'vue';
-import detailedMetric from '~/performance_bar/components/detailed_metric.vue';
-import mountComponent from 'spec/helpers/vue_mount_component_helper';
+import DetailedMetric from '~/performance_bar/components/detailed_metric.vue';
+import RequestWarning from '~/performance_bar/components/request_warning.vue';
+import { shallowMount } from '@vue/test-utils';
 
 describe('detailedMetric', () => {
-  let vm;
-
-  afterEach(() => {
-    vm.$destroy();
-  });
+  const createComponent = props =>
+    shallowMount(DetailedMetric, {
+      propsData: {
+        ...props,
+      },
+    });
 
   describe('when the current request has no details', () => {
-    beforeEach(() => {
-      vm = mountComponent(Vue.extend(detailedMetric), {
-        currentRequest: {},
-        metric: 'gitaly',
-        header: 'Gitaly calls',
-        details: 'details',
-        keys: ['feature', 'request'],
-      });
+    const wrapper = createComponent({
+      currentRequest: {},
+      metric: 'gitaly',
+      header: 'Gitaly calls',
+      details: 'details',
+      keys: ['feature', 'request'],
     });
 
     it('does not render the element', () => {
-      expect(vm.$el.innerHTML).toEqual(undefined);
+      expect(wrapper.isEmpty()).toBe(true);
     });
   });
 
@@ -31,14 +30,15 @@ describe('detailedMetric', () => {
       { duration: '23', feature: 'rebase_in_progress', request: '', backtrace: ['world', 'hello'] },
     ];
 
-    beforeEach(() => {
-      vm = mountComponent(Vue.extend(detailedMetric), {
+    describe('with a default metric name', () => {
+      const wrapper = createComponent({
         currentRequest: {
           details: {
             gitaly: {
               duration: '123ms',
               calls: '456',
               details: requestDetails,
+              warnings: ['gitaly calls: 456 over 30'],
             },
           },
         },
@@ -46,63 +46,65 @@ describe('detailedMetric', () => {
         header: 'Gitaly calls',
         keys: ['feature', 'request'],
       });
-    });
 
-    it('diplays details', () => {
-      expect(vm.$el.innerText.replace(/\s+/g, ' ')).toContain('123ms / 456');
-    });
+      it('displays details', () => {
+        expect(wrapper.text().replace(/\s+/g, ' ')).toContain('123ms / 456');
+      });
 
-    it('adds a modal with a table of the details', () => {
-      vm.$el
-        .querySelectorAll('.performance-bar-modal td:nth-child(1)')
-        .forEach((duration, index) => {
-          expect(duration.innerText).toContain(requestDetails[index].duration);
-        });
+      it('adds a modal with a table of the details', () => {
+        wrapper
+          .findAll('.performance-bar-modal td:nth-child(1)')
+          .wrappers.forEach((duration, index) => {
+            expect(duration.text()).toContain(requestDetails[index].duration);
+          });
 
-      vm.$el
-        .querySelectorAll('.performance-bar-modal td:nth-child(2)')
-        .forEach((feature, index) => {
-          expect(feature.innerText).toContain(requestDetails[index].feature);
-        });
+        wrapper
+          .findAll('.performance-bar-modal td:nth-child(2)')
+          .wrappers.forEach((feature, index) => {
+            expect(feature.text()).toContain(requestDetails[index].feature);
+          });
 
-      vm.$el
-        .querySelectorAll('.performance-bar-modal td:nth-child(2)')
-        .forEach((request, index) => {
-          expect(request.innerText).toContain(requestDetails[index].request);
-        });
+        wrapper
+          .findAll('.performance-bar-modal td:nth-child(2)')
+          .wrappers.forEach((request, index) => {
+            expect(request.text()).toContain(requestDetails[index].request);
+          });
 
-      expect(vm.$el.querySelector('.text-expander.js-toggle-button')).not.toBeNull();
+        expect(wrapper.find('.text-expander.js-toggle-button')).not.toBeNull();
 
-      vm.$el.querySelectorAll('.performance-bar-modal td:nth-child(2)').forEach(request => {
-        expect(request.innerText).toContain('world');
+        wrapper.findAll('.performance-bar-modal td:nth-child(2)').wrappers.forEach(request => {
+          expect(request.text()).toContain('world');
+        });
       });
-    });
 
-    it('displays the metric title', () => {
-      expect(vm.$el.innerText).toContain('gitaly');
+      it('displays the metric title', () => {
+        expect(wrapper.text()).toContain('gitaly');
+      });
+
+      it('displays request warnings', () => {
+        expect(wrapper.find(RequestWarning).exists()).toBe(true);
+      });
     });
 
     describe('when using a custom metric title', () => {
-      beforeEach(() => {
-        vm = mountComponent(Vue.extend(detailedMetric), {
-          currentRequest: {
-            details: {
-              gitaly: {
-                duration: '123ms',
-                calls: '456',
-                details: requestDetails,
-              },
+      const wrapper = createComponent({
+        currentRequest: {
+          details: {
+            gitaly: {
+              duration: '123ms',
+              calls: '456',
+              details: requestDetails,
             },
           },
-          metric: 'gitaly',
-          title: 'custom',
-          header: 'Gitaly calls',
-          keys: ['feature', 'request'],
-        });
+        },
+        metric: 'gitaly',
+        title: 'custom',
+        header: 'Gitaly calls',
+        keys: ['feature', 'request'],
       });
 
       it('displays the custom title', () => {
-        expect(vm.$el.innerText).toContain('custom');
+        expect(wrapper.text()).toContain('custom');
       });
     });
   });

spec/javascripts/performance_bar/components/performance_bar_app_spec.js → spec/frontend/performance_bar/components/performance_bar_app_spec.js

-import Vue from 'vue';
-import performanceBarApp from '~/performance_bar/components/performance_bar_app.vue';
+import PerformanceBarApp from '~/performance_bar/components/performance_bar_app.vue';
 import PerformanceBarStore from '~/performance_bar/stores/performance_bar_store';
-
-import mountComponent from 'spec/helpers/vue_mount_component_helper';
+import { shallowMount } from '@vue/test-utils';
 
 describe('performance bar app', () => {
-  let vm;
-
-  beforeEach(() => {
-    const store = new PerformanceBarStore();
-
-    vm = mountComponent(Vue.extend(performanceBarApp), {
+  const store = new PerformanceBarStore();
+  const wrapper = shallowMount(PerformanceBarApp, {
+    propsData: {
       store,
       env: 'development',
       requestId: '123',
       peekUrl: '/-/peek/results',
       profileUrl: '?lineprofiler=true',
-    });
-  });
-
-  afterEach(() => {
-    vm.$destroy();
+    },
   });
 
   it('sets the class to match the environment', () => {
-    expect(vm.$el.getAttribute('class')).toContain('development');
+    expect(wrapper.element.getAttribute('class')).toContain('development');
   });
 });

spec/javascripts/performance_bar/components/request_selector_spec.js → spec/frontend/performance_bar/components/request_selector_spec.js

-import Vue from 'vue';
-import requestSelector from '~/performance_bar/components/request_selector.vue';
-import mountComponent from 'spec/helpers/vue_mount_component_helper';
+import RequestSelector from '~/performance_bar/components/request_selector.vue';
+import { shallowMount } from '@vue/test-utils';
 
 describe('request selector', () => {
   const requests = [
-    { id: '123', url: 'https://gitlab.com/' },
+    {
+      id: '123',
+      url: 'https://gitlab.com/',
+      hasWarnings: false,
+    },
     {
       id: '456',
       url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1',
+      hasWarnings: false,
     },
     {
       id: '789',
       url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1.json?serializer=widget',
+      hasWarnings: false,
+    },
+    {
+      id: 'abc',
+      url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1/discussions.json',
+      hasWarnings: true,
     },
   ];
 
-  let vm;
-
-  beforeEach(() => {
-    vm = mountComponent(Vue.extend(requestSelector), {
+  const wrapper = shallowMount(RequestSelector, {
+    propsData: {
       requests,
       currentRequest: requests[1],
-    });
-  });
-
-  afterEach(() => {
-    vm.$destroy();
+    },
   });
 
   function optionText(requestId) {
-    return vm.$el.querySelector(`[value='${requestId}']`).innerText.trim();
+    return wrapper
+      .find(`[value='${requestId}']`)
+      .text()
+      .trim();
   }
 
   it('displays the last component of the path', () => {
@@ -43,4 +50,15 @@ describe('request selector', () => {
   it('ignores trailing slashes', () => {
     expect(optionText(requests[0].id)).toEqual('gitlab.com');
   });
+
+  it('has a warning icon if any requests have warnings', () => {
+    expect(wrapper.find('span > gl-emoji').element.dataset.name).toEqual('warning');
+  });
+
+  it('adds a warning glyph to requests with warnings', () => {
+    const requestValue = wrapper.find('[value="abc"]').text();
+
+    expect(requestValue).toContain('discussions.json');
+    expect(requestValue).toContain('(!)');
+  });
 });

spec/frontend/performance_bar/components/request_warning_spec.js

+import RequestWarning from '~/performance_bar/components/request_warning.vue';
+import { shallowMount } from '@vue/test-utils';
+
+describe('request warning', () => {
+  const htmlId = 'request-123';
+
+  describe('when the request has warnings', () => {
+    const wrapper = shallowMount(RequestWarning, {
+      propsData: {
+        htmlId,
+        warnings: ['gitaly calls: 30 over 10', 'gitaly duration: 1500 over 1000'],
+      },
+    });
+
+    it('adds a warning emoji with the correct ID', () => {
+      expect(wrapper.find('span[id]').attributes('id')).toEqual(htmlId);
+      expect(wrapper.find('span[id] gl-emoji').element.dataset.name).toEqual('warning');
+    });
+  });
+
+  describe('when the request does not have warnings', () => {
+    const wrapper = shallowMount(RequestWarning, {
+      propsData: {
+        htmlId,
+        warnings: [],
+      },
+    });
+
+    it('does nothing', () => {
+      expect(wrapper.isEmpty()).toBe(true);
+    });
+  });
+});