Merge branch 'hly-performance-bar-lazy' into 'master'

Lazily load performance bar request details

See merge request gitlab-org/gitlab!82187

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

@@ -134,6 +134,7 @@ export default {
   methods: {
     changeCurrentRequest(newRequestId) {
       this.currentRequest = newRequestId;
+      this.$emit('change-request', newRequestId);
     },
     flamegraphPath(mode) {
       return mergeUrlParams(

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

 <script>
-import { GlPopover, GlSafeHtmlDirective } from '@gitlab/ui';
-import { glEmojiTag } from '~/emoji';
-import { n__ } from '~/locale';
-
 export default {
-  components: {
-    GlPopover,
-  },
-  directives: {
-    SafeHtml: GlSafeHtmlDirective,
-  },
   props: {
     currentRequest: {
       type: Object,
@@ -25,27 +15,11 @@ 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);
     },
   },
-  methods: {
-    glEmojiTag,
-  },
-  safeHtmlConfig: { ADD_TAGS: ['gl-emoji'] },
 };
 </script>
 <template>
@@ -58,19 +32,7 @@ export default {
         data-qa-selector="request_dropdown_option"
       >
         {{ request.truncatedUrl }}
-        <span v-if="request.hasWarnings">(!)</span>
       </option>
     </select>
-    <span v-if="requestsWithWarnings.length" class="gl-cursor-default">
-      <span
-        id="performance-bar-request-selector-warning"
-        v-safe-html:[$options.safeHtmlConfig]="glEmojiTag('warning')"
-      ></span>
-      <gl-popover
-        placement="bottom"
-        target="performance-bar-request-selector-warning"
-        :content="warningMessage"
-      />
-    </span>
   </div>
 </template>

app/assets/javascripts/performance_bar/index.js

 import '../webpack';
 
+import { isEmpty } from 'lodash';
 import Vue from 'vue';
 import axios from '~/lib/utils/axios_utils';
 import { numberToHumanSize } from '~/lib/utils/number_utils';
@@ -37,9 +38,10 @@ const initPerformanceBar = (el) => {
       };
     },
     mounted() {
-      PerformanceBarService.registerInterceptor(this.peekUrl, this.loadRequestDetails);
+      PerformanceBarService.registerInterceptor(this.peekUrl, this.addRequest);
 
-      this.loadRequestDetails(this.requestId, window.location.href);
+      this.addRequest(this.requestId, window.location.href);
+      this.loadRequestDetails(this.requestId);
     },
     beforeDestroy() {
       PerformanceBarService.removeInterceptor();
@@ -51,26 +53,32 @@ const initPerformanceBar = (el) => {
           // want to trace the request.
           axios.get(urlOrRequestId);
         } else {
-          this.loadRequestDetails(urlOrRequestId, urlOrRequestId);
+          this.addRequest(urlOrRequestId, urlOrRequestId);
         }
       },
-      loadRequestDetails(requestId, requestUrl) {
+      addRequest(requestId, requestUrl) {
         if (!this.store.canTrackRequest(requestUrl)) {
           return;
         }
 
         this.store.addRequest(requestId, requestUrl);
+      },
+      loadRequestDetails(requestId) {
+        const request = this.store.findRequest(requestId);
 
-        PerformanceBarService.fetchRequestDetails(this.peekUrl, requestId)
+        if (request && isEmpty(request.details)) {
+          return PerformanceBarService.fetchRequestDetails(this.peekUrl, requestId)
             .then((res) => {
               this.store.addRequestDetails(requestId, res.data);
-
               if (this.requestId === requestId) this.collectFrontendPerformanceMetrics();
             })
             .catch(() =>
               // eslint-disable-next-line no-console
               console.warn(`Error getting performance bar results for ${requestId}`),
             );
+        }
+
+        return Promise.resolve();
       },
       collectFrontendPerformanceMetrics() {
         if (performance) {
@@ -143,6 +151,7 @@ const initPerformanceBar = (el) => {
         },
         on: {
           'add-request': this.addRequestManually,
+          'change-request': this.loadRequestDetails,
         },
       });
     },

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

@@ -12,7 +12,6 @@ export default class PerformanceBarStore {
         url: requestUrl,
         truncatedUrl: shortUrl,
         details: {},
-        hasWarnings: false,
       });
     }
 
@@ -27,7 +26,6 @@ export default class PerformanceBarStore {
     const request = this.findRequest(requestId);
 
     request.details = requestDetails.data;
-    request.hasWarnings = requestDetails.has_warnings;
 
     return request;
   }

doc/administration/monitoring/performance/performance_bar.md

@@ -95,18 +95,14 @@ For non-administrators to display the performance bar, it must be
 
 ## Request warnings
 
+> [Warning icon in the request selector removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82187) in GitLab 14.9.
+
 Requests that exceed predefined limits display a warning **{warning}** icon and
 explanation next to the metric. 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 warning icon displays
-next to the **Requests** selector menu. In this selector menu, an exclamation `(!)`
-appears next to requests with warnings.
-
-![Request selector showing two requests with warnings](img/performance_bar_request_selector_warning.png)
-
 ## Enable the performance bar for non-administrators
 
 The performance bar is disabled by default for non-administrators. To enable it

locale/gitlab.pot

@@ -365,11 +365,6 @@ msgid_plural "%d projects selected"
 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/frontend/performance_bar/components/performance_bar_app_spec.js

@@ -18,4 +18,15 @@ describe('performance bar app', () => {
   it('sets the class to match the environment', () => {
     expect(wrapper.element.getAttribute('class')).toContain('development');
   });
+
+  describe('changeCurrentRequest', () => {
+    it('emits a change-request event', () => {
+      expect(wrapper.emitted('change-request')).toBeUndefined();
+
+      wrapper.vm.changeCurrentRequest('123');
+
+      expect(wrapper.emitted('change-request')).toBeDefined();
+      expect(wrapper.emitted('change-request')[0]).toEqual(['123']);
+    });
+  });
 });

spec/frontend/performance_bar/components/request_selector_spec.js

-import { shallowMount } from '@vue/test-utils';
-import RequestSelector from '~/performance_bar/components/request_selector.vue';
-
-describe('request selector', () => {
-  const requests = [
-    {
-      id: 'warningReq',
-      url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1/discussions.json',
-      truncatedUrl: 'discussions.json',
-      hasWarnings: true,
-    },
-  ];
-
-  const wrapper = shallowMount(RequestSelector, {
-    propsData: {
-      requests,
-      currentRequest: requests[0],
-    },
-  });
-
-  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="warningReq"]').text();
-
-    expect(requestValue).toContain('discussions.json');
-    expect(requestValue).toContain('(!)');
-  });
-});

spec/frontend/performance_bar/index_spec.js

@@ -51,7 +51,7 @@ describe('performance bar wrapper', () => {
     mock.restore();
   });
 
-  describe('loadRequestDetails', () => {
+  describe('addRequest', () => {
     beforeEach(() => {
       jest.spyOn(vm.store, 'addRequest');
     });
@@ -59,26 +59,46 @@ describe('performance bar wrapper', () => {
     it('does nothing if the request cannot be tracked', () => {
       jest.spyOn(vm.store, 'canTrackRequest').mockImplementation(() => false);
 
-      vm.loadRequestDetails('123', 'https://gitlab.com/');
+      vm.addRequest('123', 'https://gitlab.com/');
 
       expect(vm.store.addRequest).not.toHaveBeenCalled();
     });
 
     it('adds the request immediately', () => {
-      vm.loadRequestDetails('123', 'https://gitlab.com/');
+      vm.addRequest('123', 'https://gitlab.com/');
 
       expect(vm.store.addRequest).toHaveBeenCalledWith('123', 'https://gitlab.com/');
     });
+  });
 
-    it('makes an HTTP request for the request details', () => {
+  describe('loadRequestDetails', () => {
+    beforeEach(() => {
       jest.spyOn(PerformanceBarService, 'fetchRequestDetails');
+    });
 
-      vm.loadRequestDetails('456', 'https://gitlab.com/');
+    it('makes an HTTP request for the request details', () => {
+      vm.addRequest('456', 'https://gitlab.com/');
+      vm.loadRequestDetails('456');
 
       expect(PerformanceBarService.fetchRequestDetails).toHaveBeenCalledWith(
         '/-/peek/results',
         '456',
       );
     });
+
+    it('does not make a request if request was not added', () => {
+      vm.loadRequestDetails('456');
+
+      expect(PerformanceBarService.fetchRequestDetails).not.toHaveBeenCalled();
+    });
+
+    it('makes an HTTP request only once for the same request', async () => {
+      vm.addRequest('456', 'https://gitlab.com/');
+      await vm.loadRequestDetails('456');
+
+      vm.loadRequestDetails('456');
+
+      expect(PerformanceBarService.fetchRequestDetails).toHaveBeenCalledTimes(1);
+    });
   });
 });