Merge branch 'qmnguyen0711/rollout-performance-bar-sort-order' into 'master'

Enable chronological sort order for other items in the performance bar

See merge request gitlab-org/gitlab!58572

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

@@ -54,11 +54,17 @@ export default {
       return this.currentRequest.details[this.metric];
     },
     metricDetailsSummary() {
-      return {
-        [s__('Total')]: this.metricDetails.calls,
-        [s__('PerformanceBar|Total duration')]: this.metricDetails.duration,
-        ...(this.metricDetails.summary || {}),
-      };
+      const summary = {};
+
+      if (!this.metricDetails.summaryOptions?.hideTotal) {
+        summary[s__('Total')] = this.metricDetails.calls;
+      }
+
+      if (!this.metricDetails.summaryOptions?.hideDuration) {
+        summary[s__('PerformanceBar|Total duration')] = this.metricDetails.duration;
+      }
+
+      return { ...summary, ...(this.metricDetails.summary || {}) };
     },
     metricDetailsLabel() {
       if (this.metricDetails.duration && this.metricDetails.calls) {

app/assets/javascripts/performance_bar/index.js

-/* eslint-disable @gitlab/require-i18n-strings */
 import Vue from 'vue';
 import axios from '~/lib/utils/axios_utils';
+import { numberToHumanSize } from '~/lib/utils/number_utils';
+import { s__ } from '~/locale';
 import Translate from '~/vue_shared/translate';
 
 import initPerformanceBarLog from './performance_bar_log';
@@ -75,40 +76,53 @@ const initPerformanceBar = (el) => {
           const resourceEntries = performance.getEntriesByType('resource');
 
           let durationString = '';
+          let summary = {};
           if (navigationEntries.length > 0) {
-            durationString = `${Math.round(navigationEntries[0].responseEnd)} | `;
-            durationString += `${Math.round(paintEntries[1].startTime)} | `;
-            durationString += ` ${Math.round(navigationEntries[0].domContentLoadedEventEnd)}`;
+            const backend = Math.round(navigationEntries[0].responseEnd);
+            const firstContentfulPaint = Math.round(paintEntries[1].startTime);
+            const domContentLoaded = Math.round(navigationEntries[0].domContentLoadedEventEnd);
+
+            summary = {
+              [s__('PerformanceBar|Backend')]: backend,
+              [s__('PerformanceBar|First Contentful Paint')]: firstContentfulPaint,
+              [s__('PerformanceBar|DOM Content Loaded')]: domContentLoaded,
+            };
+
+            durationString = `${backend} | ${firstContentfulPaint} | ${domContentLoaded}`;
           }
 
           let newEntries = resourceEntries.map(this.transformResourceEntry);
 
-          this.updateFrontendPerformanceMetrics(durationString, newEntries);
+          this.updateFrontendPerformanceMetrics(durationString, summary, newEntries);
 
           if ('PerformanceObserver' in window) {
             // We start observing for more incoming timings
             const observer = new PerformanceObserver((list) => {
               newEntries = newEntries.concat(list.getEntries().map(this.transformResourceEntry));
-              this.updateFrontendPerformanceMetrics(durationString, newEntries);
+              this.updateFrontendPerformanceMetrics(durationString, summary, newEntries);
             });
 
             observer.observe({ entryTypes: ['resource'] });
           }
         }
       },
-      updateFrontendPerformanceMetrics(durationString, requestEntries) {
+      updateFrontendPerformanceMetrics(durationString, summary, requestEntries) {
         this.store.setRequestDetailsData(this.requestId, 'total', {
           duration: durationString,
           calls: requestEntries.length,
           details: requestEntries,
+          summaryOptions: {
+            hideDuration: true,
+          },
+          summary,
         });
       },
       transformResourceEntry(entry) {
-        const nf = new Intl.NumberFormat();
         return {
+          start: entry.startTime,
           name: entry.name.replace(document.location.origin, ''),
           duration: Math.round(entry.duration),
-          size: entry.transferSize ? `${nf.format(entry.transferSize)} bytes` : 'cached',
+          size: entry.transferSize ? numberToHumanSize(entry.transferSize) : 'cached',
         };
       },
     },

changelogs/unreleased/qmnguyen0711-rollout-performance-bar-sort-order.yml

+---
+title: Enable chronological sort order for other items in the performance bar
+merge_request: 58572
+author:
+type: changed

lib/gitlab/gitaly_client/call.rb

@@ -50,11 +50,11 @@ module Gitlab
       end
 
       def recording_request
-        start = Gitlab::Metrics::System.monotonic_time
+        @start = Gitlab::Metrics::System.monotonic_time
 
         yield
       ensure
-        @duration += Gitlab::Metrics::System.monotonic_time - start
+        @duration += Gitlab::Metrics::System.monotonic_time - @start
       end
 
       def store_timings
@@ -64,8 +64,14 @@ module Gitlab
 
         request_hash = @request.is_a?(Google::Protobuf::MessageExts) ? @request.to_h : {}
 
-        GitalyClient.add_call_details(feature: "#{@service}##{@rpc}", duration: @duration, request: request_hash, rpc: @rpc,
-                                      backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller))
+        GitalyClient.add_call_details(
+          start: @start,
+          feature: "#{@service}##{@rpc}",
+          duration: @duration,
+          request: request_hash,
+          rpc: @rpc,
+          backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller)
+        )
       end
     end
   end

lib/gitlab/metrics/subscribers/external_http.rb

@@ -37,7 +37,7 @@ module Gitlab
 
         def request(event)
           payload = event.payload
-          add_to_detail_store(payload)
+          add_to_detail_store(event.time, payload)
           add_to_request_store(payload)
           expose_metrics(payload)
         end
@@ -48,10 +48,11 @@ module Gitlab
           ::Gitlab::Metrics::Transaction.current
         end
 
-        def add_to_detail_store(payload)
+        def add_to_detail_store(start, payload)
           return unless Gitlab::PerformanceBar.enabled_for_request?
 
           self.class.detail_store << {
+            start: start,
             duration: payload[:duration],
             scheme: payload[:scheme],
             method: payload[:method],

locale/gitlab.pot

@@ -22634,9 +22634,15 @@ msgstr ""
 msgid "Performance optimization"
 msgstr ""
 
+msgid "PerformanceBar|Backend"
+msgstr ""
+
 msgid "PerformanceBar|Bullet notifications"
 msgstr ""
 
+msgid "PerformanceBar|DOM Content Loaded"
+msgstr ""
+
 msgid "PerformanceBar|Download"
 msgstr ""
 
@@ -22646,6 +22652,9 @@ msgstr ""
 msgid "PerformanceBar|External Http calls"
 msgstr ""
 
+msgid "PerformanceBar|First Contentful Paint"
+msgstr ""
+
 msgid "PerformanceBar|Frontend resources"
 msgstr ""
 

spec/frontend/performance_bar/components/detailed_metric_spec.js

@@ -120,6 +120,73 @@ describe('detailedMetric', () => {
       });
     });
 
+    describe('when the details have summaryOptions option', () => {
+      const gitalyDetails = {
+        duration: '123ms',
+        calls: 456,
+        details: requestDetails,
+        warnings: ['gitaly calls: 456 over 30'],
+      };
+
+      describe('when the details have summaryOptions > hideTotal option', () => {
+        beforeEach(() => {
+          createComponent({
+            currentRequest: {
+              details: {
+                gitaly: { ...gitalyDetails, summaryOptions: { hideTotal: true } },
+              },
+            },
+          });
+        });
+
+        it('displays a summary section', () => {
+          expect(findAllSummaryItems()).toEqual(['Total duration 123ms']);
+        });
+      });
+
+      describe('when the details have summaryOptions > hideDuration option', () => {
+        beforeEach(() => {
+          createComponent({
+            currentRequest: {
+              details: {
+                gitaly: { ...gitalyDetails, summaryOptions: { hideDuration: true } },
+              },
+            },
+          });
+        });
+
+        it('displays a summary section', () => {
+          expect(findAllSummaryItems()).toEqual(['Total 456']);
+        });
+      });
+
+      describe('when the details have both summary and summaryOptions field', () => {
+        beforeEach(() => {
+          createComponent({
+            currentRequest: {
+              details: {
+                gitaly: {
+                  ...gitalyDetails,
+                  summary: {
+                    'In controllers': 100,
+                    'In middlewares': 20,
+                  },
+                  summaryOptions: {
+                    hideDuration: true,
+                    hideTotal: true,
+                  },
+                },
+              },
+            },
+          });
+        });
+
+        it('displays a summary section', () => {
+          expect(findAllSummaryItems()).toEqual(['In controllers 100', 'In middlewares 20']);
+        });
+      });
+    });
+
     describe("when the details don't have a start field", () => {
       beforeEach(() => {
         createComponent({

spec/lib/gitlab/gitaly_client/call_spec.rb

@@ -24,11 +24,14 @@ RSpec.describe Gitlab::GitalyClient::Call do
     def expect_call_details_to_match(duration_higher_than: 0)
       expect(client.list_call_details.size).to eq(1)
       expect(client.list_call_details.first)
-        .to match a_hash_including(feature: "#{service}##{rpc}",
-                                   duration: a_value > duration_higher_than,
-                                   request: an_instance_of(Hash),
-                                   rpc: rpc,
-                                   backtrace: an_instance_of(Array))
+        .to match a_hash_including(
+          start: a_value > 0,
+          feature: "#{service}##{rpc}",
+          duration: a_value > duration_higher_than,
+          request: an_instance_of(Hash),
+          rpc: rpc,
+          backtrace: an_instance_of(Array)
+        )
     end
 
     context 'when the response is not an enumerator' do

spec/lib/gitlab/metrics/subscribers/external_http_spec.rb

@@ -6,29 +6,45 @@ RSpec.describe Gitlab::Metrics::Subscribers::ExternalHttp, :request_store do
   let(:transaction) { Gitlab::Metrics::Transaction.new }
   let(:subscriber) { described_class.new }
 
+  around do |example|
+    freeze_time { example.run }
+  end
+
   let(:event_1) do
-    double(:event, payload: {
-      method: 'POST', code: "200", duration: 0.321,
-      scheme: 'https', host: 'gitlab.com', port: 80, path: '/api/v4/projects',
-      query: 'current=true'
-    })
+    double(
+      :event,
+      payload: {
+        method: 'POST', code: "200", duration: 0.321,
+        scheme: 'https', host: 'gitlab.com', port: 80, path: '/api/v4/projects',
+        query: 'current=true'
+      },
+      time: Time.current
+    )
   end
 
   let(:event_2) do
-    double(:event, payload: {
-      method: 'GET', code: "301", duration: 0.12,
-      scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2',
-      query: 'current=true'
-    })
+    double(
+      :event,
+      payload: {
+        method: 'GET', code: "301", duration: 0.12,
+        scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2',
+        query: 'current=true'
+      },
+      time: Time.current
+    )
   end
 
   let(:event_3) do
-    double(:event, payload: {
-      method: 'POST', duration: 5.3,
-      scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2/issues',
-      query: 'current=true',
-      exception_object: Net::ReadTimeout.new
-    })
+    double(
+      :event,
+      payload: {
+        method: 'POST', duration: 5.3,
+        scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2/issues',
+        query: 'current=true',
+        exception_object: Net::ReadTimeout.new
+      },
+      time: Time.current
+    )
   end
 
   describe '.detail_store' do
@@ -134,19 +150,22 @@ RSpec.describe Gitlab::Metrics::Subscribers::ExternalHttp, :request_store do
       subscriber.request(event_3)
 
       expect(Gitlab::SafeRequestStore[:external_http_detail_store].length).to eq(3)
-      expect(Gitlab::SafeRequestStore[:external_http_detail_store][0]).to include(
+      expect(Gitlab::SafeRequestStore[:external_http_detail_store][0]).to match a_hash_including(
+        start: be_like_time(Time.current),
         method: 'POST', code: "200", duration: 0.321,
         scheme: 'https', host: 'gitlab.com', port: 80, path: '/api/v4/projects',
         query: 'current=true', exception_object: nil,
         backtrace: be_a(Array)
       )
-      expect(Gitlab::SafeRequestStore[:external_http_detail_store][1]).to include(
+      expect(Gitlab::SafeRequestStore[:external_http_detail_store][1]).to match a_hash_including(
+        start: be_like_time(Time.current),
         method: 'GET', code: "301", duration: 0.12,
         scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2',
         query: 'current=true', exception_object: nil,
         backtrace: be_a(Array)
       )
-      expect(Gitlab::SafeRequestStore[:external_http_detail_store][2]).to include(
+      expect(Gitlab::SafeRequestStore[:external_http_detail_store][2]).to match a_hash_including(
+        start: be_like_time(Time.current),
         method: 'POST', duration: 5.3,
         scheme: 'http', host: 'gitlab.com', port: 80, path: '/api/v4/projects/2/issues',
         query: 'current=true',

spec/lib/peek/views/external_http_spec.rb

@@ -11,6 +11,10 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     allow(Gitlab::PerformanceBar).to receive(:enabled_for_request?).and_return(true)
   end
 
+  around do |example|
+    freeze_time { example.run }
+  end
+
   let(:event_1) do
     {
       method: 'POST', code: "200", duration: 0.03,
@@ -44,9 +48,9 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
   end
 
   it 'returns aggregated results' do
-    subscriber.request(double(:event, payload: event_1))
-    subscriber.request(double(:event, payload: event_2))
-    subscriber.request(double(:event, payload: event_3))
+    subscriber.request(double(:event, payload: event_1, time: Time.current))
+    subscriber.request(double(:event, payload: event_2, time: Time.current))
+    subscriber.request(double(:event, payload: event_3, time: Time.current))
 
     results = subject.results
     expect(results[:calls]).to eq(3)
@@ -55,6 +59,7 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
 
     expected = [
       {
+        start: be_like_time(Time.current),
         duration: 30.0,
         label: "POST https://gitlab.com:80/api/v4/projects?current=true",
         code: "Response status: 200",
@@ -63,6 +68,7 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
         warnings: []
       },
       {
+        start: be_like_time(Time.current),
         duration: 1300,
         label: "POST http://gitlab.com:80/api/v4/projects/2/issues?current=true",
         code: nil,
@@ -71,6 +77,7 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
         warnings: ["1300.0 over 100"]
       },
       {
+        start: be_like_time(Time.current),
         duration: 5.0,
         label: "GET http://gitlab.com:80/api/v4/projects/2?current=true",
         code: "Response status: 301",
@@ -81,7 +88,7 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     ]
 
     expect(
-      results[:details].map { |data| data.slice(:duration, :label, :code, :proxy, :error, :warnings) }
+      results[:details].map { |data| data.slice(:start, :duration, :label, :code, :proxy, :error, :warnings) }
     ).to match_array(expected)
   end
 
@@ -91,10 +98,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'displays IPv4 in the label' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST https://1.2.3.4:80/api/v4/projects?current=true",
           code: "Response status: 200",
@@ -112,10 +120,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'displays IPv6 in the label' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST https://[2606:4700:90:0:f22e:fbec:5bed:a9b9]:80/api/v4/projects?current=true",
           code: "Response status: 200",
@@ -133,10 +142,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'converts query hash into a query string' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST https://gitlab.com:80/api/v4/projects?current=true&item1=string&item2%5B%5D=1&item2%5B%5D=2",
           code: "Response status: 200",
@@ -154,10 +164,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'displays unknown in the label' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST unknown",
           code: "Response status: 200",
@@ -176,10 +187,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'displays unknown in the label' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST unknown",
           code: "Response status: 200",
@@ -198,10 +210,11 @@ RSpec.describe Peek::Views::ExternalHttp, :request_store do
     end
 
     it 'displays unknown in the label' do
-      subscriber.request(double(:event, payload: event_1))
+      subscriber.request(double(:event, payload: event_1, time: Time.current))
 
       expect(subject.results[:details]).to contain_exactly(
         a_hash_including(
+          start: be_like_time(Time.current),
           duration: 30.0,
           label: "POST unknown",
           code: "Response status: 200",