Merge branch 'id-add-bullet-to-performance-bar' into 'master'

Add Bullet notifications to Performance bar

See merge request gitlab-org/gitlab!30827

Gemfile

@@ -343,7 +343,7 @@ group :development do
 end
 
 group :development, :test do
-  gem 'bullet', '~> 6.0.2', require: !!ENV['ENABLE_BULLET']
+  gem 'bullet', '~> 6.0.2'
   gem 'pry-byebug', '~> 3.5.1', platform: :mri
   gem 'pry-rails', '~> 0.3.9'
 

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

@@ -39,6 +39,11 @@ export default {
     metricDetails() {
       return this.currentRequest.details[this.metric];
     },
+    metricDetailsLabel() {
+      return this.metricDetails.duration
+        ? `${this.metricDetails.duration} / ${this.metricDetails.calls}`
+        : this.metricDetails.calls;
+    },
     detailsList() {
       return this.metricDetails.details;
     },
@@ -68,7 +73,7 @@ export default {
       type="button"
       data-toggle="modal"
     >
-      {{ metricDetails.duration }} / {{ metricDetails.calls }}
+      {{ metricDetailsLabel }}
     </button>
     <gl-modal
       :id="`modal-peek-${metric}-details`"
@@ -80,7 +85,9 @@ export default {
         <template v-if="detailsList.length">
           <tr v-for="(item, index) in detailsList" :key="index">
             <td>
-              <span>{{ sprintf(__('%{duration}ms'), { duration: item.duration }) }}</span>
+              <span v-if="item.duration">{{
+                sprintf(__('%{duration}ms'), { duration: item.duration })
+              }}</span>
             </td>
             <td>
               <div class="js-toggle-container">

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

@@ -37,6 +37,11 @@ export default {
       header: s__('PerformanceBar|SQL queries'),
       keys: ['sql'],
     },
+    {
+      metric: 'bullet',
+      header: s__('PerformanceBar|Bullet notifications'),
+      keys: ['notification'],
+    },
     {
       metric: 'gitaly',
       header: s__('PerformanceBar|Gitaly calls'),

config/initializers/bullet.rb

-if defined?(Bullet) && ENV['ENABLE_BULLET']
+def bullet_enabled?
+  Gitlab::Utils.to_boolean(ENV['ENABLE_BULLET'].to_s)
+end
+
+if defined?(Bullet) && (bullet_enabled? || Rails.env.development?)
   Rails.application.configure do
     config.after_initialize do
       Bullet.enable = true
 
-      Bullet.bullet_logger = true
-      Bullet.console = true
+      Bullet.bullet_logger = bullet_enabled?
+      Bullet.console = bullet_enabled?
+
       Bullet.raise = Rails.env.test?
     end
   end

config/initializers/peek.rb

@@ -10,5 +10,6 @@ Peek.into Peek::Views::ActiveRecord
 Peek.into Peek::Views::Gitaly
 Peek.into Peek::Views::RedisDetailed
 Peek.into Peek::Views::Rugged
+Peek.into Peek::Views::BulletDetailed if defined?(Bullet)
 
 Peek.into Peek::Views::Tracing if Labkit::Tracing.tracing_url_enabled?

doc/development/profiling.md

@@ -107,9 +107,13 @@ Recorded transactions can be found by navigating to `/sherlock/transactions`.
 
 ## Bullet
 
-Bullet is a Gem that can be used to track down N+1 query problems. Because
-Bullet adds quite a bit of logging noise it's disabled by default. To enable
-Bullet, set the environment variable `ENABLE_BULLET` to a non-empty value before
+Bullet is a Gem that can be used to track down N+1 query problems. Bullet section is
+displayed on the [performance-bar](../administration/monitoring/performance/performance_bar.md).
+
+![Bullet](img/bullet_v13_0.png)
+
+Because Bullet adds quite a bit of logging noise the logging is disabled by default.
+To enable the logging, set the environment variable `ENABLE_BULLET` to a non-empty value before
 starting GitLab. For example:
 
 ```shell

lib/peek/views/bullet_detailed.rb

+# frozen_string_literal: true
+
+module Peek
+  module Views
+    class BulletDetailed < DetailedView
+      WARNING_MESSAGE = "Unoptimized queries detected"
+
+      def key
+        'bullet'
+      end
+
+      def results
+        return {} unless ::Bullet.enable?
+        return {} unless calls > 0
+
+        {
+          calls: calls,
+          details: details,
+          warnings: [WARNING_MESSAGE]
+        }
+      end
+
+      private
+
+      def details
+        notifications.map do |notification|
+          # there is no public method which returns pure backtace:
+          # https://github.com/flyerhzm/bullet/blob/9cda9c224a46786ecfa894480c4dd4d304db2adb/lib/bullet/notification/n_plus_one_query.rb
+          backtrace = notification.body_with_caller
+
+          {
+            notification: "#{notification.title}: #{notification.body}",
+            backtrace: backtrace
+          }
+        end
+      end
+
+      def calls
+        notifications.size
+      end
+
+      def notifications
+        ::Bullet.notification_collector&.collection || []
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -15374,6 +15374,9 @@ msgstr ""
 msgid "Performance optimization"
 msgstr ""
 
+msgid "PerformanceBar|Bullet notifications"
+msgstr ""
+
 msgid "PerformanceBar|Download"
 msgstr ""
 

spec/frontend/performance_bar/components/detailed_metric_spec.js

 import { shallowMount } from '@vue/test-utils';
 import DetailedMetric from '~/performance_bar/components/detailed_metric.vue';
 import RequestWarning from '~/performance_bar/components/request_warning.vue';
+import { trimText } from 'helpers/text_helper';
 
 describe('detailedMetric', () => {
-  const createComponent = props =>
-    shallowMount(DetailedMetric, {
+  let wrapper;
+
+  const createComponent = props => {
+    wrapper = shallowMount(DetailedMetric, {
       propsData: {
         ...props,
       },
     });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
 
   describe('when the current request has no details', () => {
-    const wrapper = createComponent({
-      currentRequest: {},
-      metric: 'gitaly',
-      header: 'Gitaly calls',
-      details: 'details',
-      keys: ['feature', 'request'],
+    beforeEach(() => {
+      createComponent({
+        currentRequest: {},
+        metric: 'gitaly',
+        header: 'Gitaly calls',
+        details: 'details',
+        keys: ['feature', 'request'],
+      });
     });
 
     it('does not render the element', () => {
@@ -31,20 +41,22 @@ describe('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'],
+      beforeEach(() => {
+        createComponent({
+          currentRequest: {
+            details: {
+              gitaly: {
+                duration: '123ms',
+                calls: '456',
+                details: requestDetails,
+                warnings: ['gitaly calls: 456 over 30'],
+              },
             },
           },
-        },
-        metric: 'gitaly',
-        header: 'Gitaly calls',
-        keys: ['feature', 'request'],
+          metric: 'gitaly',
+          header: 'Gitaly calls',
+          keys: ['feature', 'request'],
+        });
       });
 
       it('displays details', () => {
@@ -87,25 +99,49 @@ describe('detailedMetric', () => {
     });
 
     describe('when using a custom metric title', () => {
-      const wrapper = createComponent({
+      beforeEach(() => {
+        createComponent({
+          currentRequest: {
+            details: {
+              gitaly: {
+                duration: '123ms',
+                calls: '456',
+                details: requestDetails,
+              },
+            },
+          },
+          metric: 'gitaly',
+          title: 'custom',
+          header: 'Gitaly calls',
+          keys: ['feature', 'request'],
+        });
+      });
+
+      it('displays the custom title', () => {
+        expect(wrapper.text()).toContain('custom');
+      });
+    });
+  });
+
+  describe('when the details has no duration', () => {
+    beforeEach(() => {
+      createComponent({
         currentRequest: {
           details: {
-            gitaly: {
-              duration: '123ms',
+            bullet: {
               calls: '456',
-              details: requestDetails,
+              details: [{ notification: 'notification', backtrace: 'backtrace' }],
             },
           },
         },
-        metric: 'gitaly',
-        title: 'custom',
-        header: 'Gitaly calls',
-        keys: ['feature', 'request'],
+        metric: 'bullet',
+        header: 'Bullet notifications',
+        keys: ['notification'],
       });
+    });
 
-      it('displays the custom title', () => {
-        expect(wrapper.text()).toContain('custom');
-      });
+    it('renders only the number of calls', () => {
+      expect(trimText(wrapper.text())).toEqual('456 notification backtrace bullet');
     });
   });
 });

spec/lib/peek/views/bullet_detailed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Peek::Views::BulletDetailed do
+  subject { described_class.new }
+
+  before do
+    allow(Bullet).to receive(:enable?).and_return(bullet_enabled)
+  end
+
+  context 'bullet disabled' do
+    let(:bullet_enabled) { false }
+
+    it 'returns empty results' do
+      expect(subject.results).to eq({})
+    end
+  end
+
+  context 'bullet enabled' do
+    let(:bullet_enabled) { true }
+
+    before do
+      allow(Bullet).to receive_message_chain(:notification_collector, :collection).and_return(notifications)
+    end
+
+    context 'where there are no notifications' do
+      let(:notifications) { [] }
+
+      it 'returns empty results' do
+        expect(subject.results).to eq({})
+      end
+    end
+
+    context 'when notifications exist' do
+      let(:notifications) do
+        [
+          double(title: 'Title 1', body: 'Body 1', body_with_caller: "first\nsecond\n"),
+          double(title: 'Title 2', body: 'Body 2', body_with_caller: "first\nsecond\n")
+        ]
+      end
+
+      it 'returns empty results' do
+        expect(subject.key).to eq('bullet')
+        expect(subject.results[:calls]).to eq(2)
+        expect(subject.results[:warnings]).to eq([Peek::Views::BulletDetailed::WARNING_MESSAGE])
+        expect(subject.results[:details]).to eq([
+          { notification: 'Title 1: Body 1', backtrace: "first\nsecond\n" },
+          { notification: 'Title 2: Body 2', backtrace: "first\nsecond\n" }
+        ])
+      end
+    end
+  end
+end