Merge branch 'mb-ruby-sentry-upgrade' into 'master'

Update Sentry gem to version 5.1.1

See merge request gitlab-org/gitlab!72428

Gemfile

@@ -302,6 +302,9 @@ gem 'rack-attack', '~> 6.3.0'
 
 # Sentry integration
 gem 'sentry-raven', '~> 3.1'
+gem 'sentry-ruby', '~> 5.1.1'
+gem 'sentry-rails', '~> 5.1.1'
+gem 'sentry-sidekiq', '~> 5.1.1'
 
 # PostgreSQL query parsing
 #

Gemfile.lock

@@ -1174,8 +1174,19 @@ GEM
     selenium-webdriver (3.142.7)
       childprocess (>= 0.5, < 4.0)
       rubyzip (>= 1.2.2)
+    sentry-rails (5.1.1)
+      railties (>= 5.0)
+      sentry-ruby-core (~> 5.1.1)
     sentry-raven (3.1.2)
       faraday (>= 1.0)
+    sentry-ruby (5.1.1)
+      concurrent-ruby (~> 1.0, >= 1.0.2)
+      sentry-ruby-core (= 5.1.1)
+    sentry-ruby-core (5.1.1)
+      concurrent-ruby
+    sentry-sidekiq (5.1.1)
+      sentry-ruby-core (~> 5.1.1)
+      sidekiq (>= 3.0)
     set (1.0.1)
     settingslogic (2.0.9)
     sexp_processor (4.15.1)
@@ -1627,7 +1638,10 @@ DEPENDENCIES
   sd_notify (~> 0.1.0)
   seed-fu (~> 2.3.7)
   selenium-webdriver (~> 3.142)
+  sentry-rails (~> 5.1.1)
   sentry-raven (~> 3.1)
+  sentry-ruby (~> 5.1.1)
+  sentry-sidekiq (~> 5.1.1)
   settingslogic (~> 2.0.9)
   shoulda-matchers (~> 4.0.1)
   sidekiq (~> 6.4)

config/feature_flags/development/enable_new_sentry_integration.yml

+---
+name: enable_new_sentry_integration
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72428
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344832
+milestone: '14.9'
+type: development
+group: group::pipeline execution
+default_enabled: false

config/feature_flags/development/enable_old_sentry_integration.yml

+---
+name: enable_old_sentry_integration
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72428
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344832
+milestone: '14.9'
+type: development
+group: group::pipeline execution
+default_enabled: true

lib/gitlab/error_tracking.rb

@@ -23,7 +23,12 @@ module Gitlab
     ].freeze
 
     class << self
-      def configure
+      def configure(&block)
+        configure_raven(&block)
+        configure_sentry(&block)
+      end
+
+      def configure_raven
         Raven.configure do |config|
           config.dsn = sentry_dsn
           config.release = Gitlab.revision
@@ -34,7 +39,20 @@ module Gitlab
 
           # Sanitize authentication headers
           config.sanitize_http_headers = %w[Authorization Private-Token]
-          config.before_send = method(:before_send)
+          config.before_send = method(:before_send_raven)
+
+          yield config if block_given?
+        end
+      end
+
+      def configure_sentry
+        Sentry.init do |config|
+          config.dsn = new_sentry_dsn
+          config.release = Gitlab.revision
+          config.environment = new_sentry_environment
+          config.before_send = method(:before_send_sentry)
+          config.background_worker_threads = 0
+          config.send_default_pii = true
 
           yield config if block_given?
         end
@@ -96,6 +114,18 @@ module Gitlab
 
       private
 
+      def before_send_raven(event, hint)
+        return unless Feature.enabled?(:enable_old_sentry_integration, default_enabled: :yaml)
+
+        before_send(event, hint)
+      end
+
+      def before_send_sentry(event, hint)
+        return unless Feature.enabled?(:enable_new_sentry_integration, default_enabled: :yaml)
+
+        before_send(event, hint)
+      end
+
       def before_send(event, hint)
         inject_context_for_exception(event, hint[:exception])
         custom_fingerprinting(event, hint[:exception])
@@ -112,6 +142,13 @@ module Gitlab
           Raven.capture_exception(exception, **context_payload)
         end
 
+        # There is a possibility that this method is called before Sentry is
+        # configured. Since Sentry 4.0, some methods of Sentry are forwarded to
+        # to `nil`, hence we have to check the client as well.
+        if sentry && ::Sentry.get_current_client && ::Sentry.configuration.dsn
+          ::Sentry.capture_exception(exception, **context_payload)
+        end
+
         if logging
           formatter = Gitlab::ErrorTracking::LogFormatter.new
           log_hash = formatter.generate_log(exception, context_payload)
@@ -121,12 +158,30 @@ module Gitlab
       end
 
       def sentry_dsn
-        return unless Rails.env.production? || Rails.env.development?
+        return unless sentry_configurable?
         return unless Gitlab.config.sentry.enabled
 
         Gitlab.config.sentry.dsn
       end
 
+      def new_sentry_dsn
+        return unless sentry_configurable?
+        return unless Gitlab::CurrentSettings.respond_to?(:sentry_enabled?)
+        return unless Gitlab::CurrentSettings.sentry_enabled?
+
+        Gitlab::CurrentSettings.sentry_dsn
+      end
+
+      def new_sentry_environment
+        return unless Gitlab::CurrentSettings.respond_to?(:sentry_environment)
+
+        Gitlab::CurrentSettings.sentry_environment
+      end
+
+      def sentry_configurable?
+        Rails.env.production? || Rails.env.development?
+      end
+
       def should_raise_for_dev?
         Rails.env.development? || Rails.env.test?
       end

lib/gitlab/error_tracking/processor/grpc_error_processor.rb

@@ -18,7 +18,7 @@ module Gitlab
           # only the first one since that's what is used for grouping.
           def process_first_exception_value(event)
             # Better in new version, will be event.exception.values
-            exceptions = event.instance_variable_get(:@interfaces)[:exception]&.values
+            exceptions = extract_exceptions_from(event)
 
             return unless exceptions.is_a?(Array)
 
@@ -37,7 +37,13 @@ module Gitlab
             # instance variable
             if message.present?
               exceptions.each do |exception|
-                exception.value = message if valid_exception?(exception)
+                next unless valid_exception?(exception)
+
+                if exception.respond_to?(:value=)
+                  exception.value = message
+                else
+                  exception.instance_variable_set(:@value, message)
+                end
               end
             end
 
@@ -55,6 +61,14 @@ module Gitlab
 
           private
 
+          def extract_exceptions_from(event)
+            if event.is_a?(Raven::Event)
+              event.instance_variable_get(:@interfaces)[:exception]&.values
+            else
+              event.exception&.instance_variable_get(:@values)
+            end
+          end
+
           def custom_grpc_fingerprint?(fingerprint)
             fingerprint.is_a?(Array) && fingerprint.length == 2 && fingerprint[0].start_with?('GRPC::')
           end
@@ -71,7 +85,7 @@ module Gitlab
 
           def valid_exception?(exception)
             case exception
-            when Raven::SingleExceptionInterface
+            when Raven::SingleExceptionInterface, Sentry::SingleExceptionInterface
               exception&.value
             else
               false

spec/lib/gitlab/error_tracking/processor/grpc_error_processor_spec.rb

@@ -2,9 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
+RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor, :sentry do
   describe '.call' do
-    let(:required_options) do
+    let(:raven_required_options) do
       {
         configuration: Raven.configuration,
         context: Raven.context,
@@ -12,7 +12,15 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
       }
     end
 
-    let(:event) { Raven::Event.from_exception(exception, required_options.merge(data)) }
+    let(:raven_event) do
+      Raven::Event
+        .from_exception(exception, raven_required_options.merge(data))
+    end
+
+    let(:sentry_event) do
+      Sentry.get_current_client.event_from_exception(exception)
+    end
+
     let(:result_hash) { described_class.call(event).to_hash }
 
     let(:data) do
@@ -27,36 +35,43 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
       }
     end
 
+    before do
+      Sentry.get_current_scope.update_from_options(**data)
+      Sentry.get_current_scope.apply_to_event(sentry_event)
+    end
+
+    after do
+      Sentry.get_current_scope.clear
+    end
+
     context 'when there is no GRPC exception' do
       let(:exception) { RuntimeError.new }
       let(:data) { { fingerprint: ['ArgumentError', 'Missing arguments'] } }
 
-      it 'leaves data unchanged' do
-        expect(result_hash).to include(data)
+      shared_examples 'leaves data unchanged' do
+        it { expect(result_hash).to include(data) }
       end
-    end
 
-    context 'when there is a GRPC exception with a debug string' do
-      let(:exception) { GRPC::DeadlineExceeded.new('Deadline Exceeded', {}, '{"hello":1}') }
+      context 'with Raven event' do
+        let(:event) { raven_event }
 
-      it 'removes the debug error string and stores it as an extra field' do
-        expect(result_hash[:fingerprint])
-          .to eq(['GRPC::DeadlineExceeded', '4:Deadline Exceeded.'])
+        it_behaves_like 'leaves data unchanged'
+      end
 
-        expect(result_hash[:exception][:values].first)
-          .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
+      context 'with Sentry event' do
+        let(:event) { sentry_event }
 
-        expect(result_hash[:extra])
-          .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
+        it_behaves_like 'leaves data unchanged'
       end
+    end
 
-      context 'with no custom fingerprint' do
-        let(:data) do
-          { extra: { caller: 'test' } }
-        end
+    context 'when there is a GRPC exception with a debug string' do
+      let(:exception) { GRPC::DeadlineExceeded.new('Deadline Exceeded', {}, '{"hello":1}') }
 
+      shared_examples 'processes the exception' do
         it 'removes the debug error string and stores it as an extra field' do
-          expect(result_hash).not_to include(:fingerprint)
+          expect(result_hash[:fingerprint])
+            .to eq(['GRPC::DeadlineExceeded', '4:Deadline Exceeded.'])
 
           expect(result_hash[:exception][:values].first)
             .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
@@ -64,11 +79,42 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
           expect(result_hash[:extra])
             .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
         end
+
+        context 'with no custom fingerprint' do
+          let(:data) do
+            { extra: { caller: 'test' } }
+          end
+
+          it 'removes the debug error string and stores it as an extra field' do
+            expect(result_hash[:fingerprint]).to be_blank
+
+            expect(result_hash[:exception][:values].first)
+              .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
+
+            expect(result_hash[:extra])
+              .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
+          end
+        end
+      end
+
+      context 'with Raven event' do
+        let(:event) { raven_event }
+
+        it_behaves_like 'processes the exception'
+      end
+
+      context 'with Sentry event' do
+        let(:event) { sentry_event }
+
+        it_behaves_like 'processes the exception'
       end
     end
 
     context 'when there is a wrapped GRPC exception with a debug string' do
-      let(:inner_exception) { GRPC::DeadlineExceeded.new('Deadline Exceeded', {}, '{"hello":1}') }
+      let(:inner_exception) do
+        GRPC::DeadlineExceeded.new('Deadline Exceeded', {}, '{"hello":1}')
+      end
+
       let(:exception) do
         begin
           raise inner_exception
@@ -79,27 +125,10 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
         e
       end
 
-      it 'removes the debug error string and stores it as an extra field' do
-        expect(result_hash[:fingerprint])
-          .to eq(['GRPC::DeadlineExceeded', '4:Deadline Exceeded.'])
-
-        expect(result_hash[:exception][:values].first)
-          .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
-
-        expect(result_hash[:exception][:values].second)
-          .to include(type: 'StandardError', value: '4:Deadline Exceeded.')
-
-        expect(result_hash[:extra])
-          .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
-      end
-
-      context 'with no custom fingerprint' do
-        let(:data) do
-          { extra: { caller: 'test' } }
-        end
-
+      shared_examples 'processes the exception' do
         it 'removes the debug error string and stores it as an extra field' do
-          expect(result_hash).not_to include(:fingerprint)
+          expect(result_hash[:fingerprint])
+            .to eq(['GRPC::DeadlineExceeded', '4:Deadline Exceeded.'])
 
           expect(result_hash[:exception][:values].first)
             .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
@@ -110,6 +139,37 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do
           expect(result_hash[:extra])
             .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
         end
+
+        context 'with no custom fingerprint' do
+          let(:data) do
+            { extra: { caller: 'test' } }
+          end
+
+          it 'removes the debug error string and stores it as an extra field' do
+            expect(result_hash[:fingerprint]).to be_blank
+
+            expect(result_hash[:exception][:values].first)
+              .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.')
+
+            expect(result_hash[:exception][:values].second)
+              .to include(type: 'StandardError', value: '4:Deadline Exceeded.')
+
+            expect(result_hash[:extra])
+              .to include(caller: 'test', grpc_debug_error_string: '{"hello":1}')
+          end
+        end
+      end
+
+      context 'with Raven event' do
+        let(:event) { raven_event }
+
+        it_behaves_like 'processes the exception'
+      end
+
+      context 'with Sentry event' do
+        let(:event) { sentry_event }
+
+        it_behaves_like 'processes the exception'
       end
     end
   end

spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 require 'rspec-parameterized'
 
-RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
+RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor, :sentry do
   after do
     if described_class.instance_variable_defined?(:@permitted_arguments_for_worker)
       described_class.remove_instance_variable(:@permitted_arguments_for_worker)
@@ -95,7 +95,9 @@ RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
   end
 
   describe '.call' do
-    let(:required_options) do
+    let(:exception) { StandardError.new('Test exception') }
+
+    let(:raven_required_options) do
       {
         configuration: Raven.configuration,
         context: Raven.context,
@@ -103,9 +105,25 @@ RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
       }
     end
 
-    let(:event) { Raven::Event.new(required_options.merge(wrapped_value)) }
+    let(:raven_event) do
+      Raven::Event.new(raven_required_options.merge(wrapped_value))
+    end
+
+    let(:sentry_event) do
+      Sentry.get_current_client.event_from_exception(exception)
+    end
+
     let(:result_hash) { described_class.call(event).to_hash }
 
+    before do
+      Sentry.get_current_scope.update_from_options(**wrapped_value)
+      Sentry.get_current_scope.apply_to_event(sentry_event)
+    end
+
+    after do
+      Sentry.get_current_scope.clear
+    end
+
     context 'when there is Sidekiq data' do
       let(:wrapped_value) { { extra: { sidekiq: value } } }
 
@@ -140,42 +158,90 @@ RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
       end
 
       context 'when processing via the default error handler' do
-        include_examples 'Sidekiq arguments', args_in_job_hash: true
+        context 'with Raven events' do
+          let(:event) { raven_event}
+
+          include_examples 'Sidekiq arguments', args_in_job_hash: true
+        end
+
+        context 'with Sentry events' do
+          let(:event) { sentry_event}
+
+          include_examples 'Sidekiq arguments', args_in_job_hash: true
+        end
       end
 
       context 'when processing via Gitlab::ErrorTracking' do
-        include_examples 'Sidekiq arguments', args_in_job_hash: false
-      end
+        context 'with Raven events' do
+          let(:event) { raven_event}
 
-      context 'when a jobstr field is present' do
-        let(:value) do
-          {
-            job: { 'args' => [1] },
-            jobstr: { 'args' => [1] }.to_json
-          }
+          include_examples 'Sidekiq arguments', args_in_job_hash: false
         end
 
-        it 'removes the jobstr' do
-          expect(result_hash.dig(:extra, :sidekiq)).to eq(value.except(:jobstr))
+        context 'with Sentry events' do
+          let(:event) { sentry_event}
+
+          include_examples 'Sidekiq arguments', args_in_job_hash: false
         end
       end
 
-      context 'when no jobstr value is present' do
-        let(:value) { { job: { 'args' => [1] } } }
+      shared_examples 'handles jobstr fields' do
+        context 'when a jobstr field is present' do
+          let(:value) do
+            {
+              job: { 'args' => [1] },
+              jobstr: { 'args' => [1] }.to_json
+            }
+          end
+
+          it 'removes the jobstr' do
+            expect(result_hash.dig(:extra, :sidekiq)).to eq(value.except(:jobstr))
+          end
+        end
+
+        context 'when no jobstr value is present' do
+          let(:value) { { job: { 'args' => [1] } } }
 
-        it 'does nothing' do
-          expect(result_hash.dig(:extra, :sidekiq)).to eq(value)
+          it 'does nothing' do
+            expect(result_hash.dig(:extra, :sidekiq)).to eq(value)
+          end
         end
       end
+
+      context 'with Raven events' do
+        let(:event) { raven_event}
+
+        it_behaves_like 'handles jobstr fields'
+      end
+
+      context 'with Sentry events' do
+        let(:event) { sentry_event}
+
+        it_behaves_like 'handles jobstr fields'
+      end
     end
 
     context 'when there is no Sidekiq data' do
       let(:value) { { tags: { foo: 'bar', baz: 'quux' } } }
       let(:wrapped_value) { value }
 
-      it 'does nothing' do
-        expect(result_hash).to include(value)
-        expect(result_hash.dig(:extra, :sidekiq)).to be_nil
+      shared_examples 'does nothing' do
+        it 'does nothing' do
+          expect(result_hash).to include(value)
+          expect(result_hash.dig(:extra, :sidekiq)).to be_nil
+        end
+      end
+
+      context 'with Raven events' do
+        let(:event) { raven_event}
+
+        it_behaves_like 'does nothing'
+      end
+
+      context 'with Sentry events' do
+        let(:event) { sentry_event}
+
+        it_behaves_like 'does nothing'
       end
     end
 
@@ -183,8 +249,22 @@ RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
       let(:value) { { other: 'foo' } }
       let(:wrapped_value) { { extra: { sidekiq: value } } }
 
-      it 'does nothing' do
-        expect(result_hash.dig(:extra, :sidekiq)).to eq(value)
+      shared_examples 'does nothing' do
+        it 'does nothing' do
+          expect(result_hash.dig(:extra, :sidekiq)).to eq(value)
+        end
+      end
+
+      context 'with Raven events' do
+        let(:event) { raven_event}
+
+        it_behaves_like 'does nothing'
+      end
+
+      context 'with Sentry events' do
+        let(:event) { sentry_event}
+
+        it_behaves_like 'does nothing'
       end
     end
   end

spec/support/helpers/stub_configuration.rb

@@ -90,10 +90,18 @@ module StubConfiguration
     allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
   end
 
-  def stub_sentry_settings
-    allow(Gitlab.config.sentry).to receive(:enabled).and_return(true)
-    allow(Gitlab.config.sentry).to receive(:dsn).and_return('dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/42')
-    allow(Gitlab.config.sentry).to receive(:clientside_dsn).and_return('dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/43')
+  def stub_sentry_settings(enabled: true)
+    allow(Gitlab.config.sentry).to receive(:enabled) { enabled }
+    allow(Gitlab::CurrentSettings).to receive(:sentry_enabled?) { enabled }
+
+    dsn = 'dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/42'
+    allow(Gitlab.config.sentry).to receive(:dsn) { dsn }
+    allow(Gitlab::CurrentSettings).to receive(:sentry_dsn) { dsn }
+
+    clientside_dsn = 'dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/43'
+    allow(Gitlab.config.sentry).to receive(:clientside_dsn) { clientside_dsn }
+    allow(Gitlab::CurrentSettings)
+      .to receive(:sentry_clientside_dsn) { clientside_dsn }
   end
 
   def stub_kerberos_setting(messages)

spec/support/sentry.rb

+# frozen_string_literal: true
+
+RSpec.configure do |config|
+  config.around(:example, :sentry) do |example|
+    dsn = Sentry.get_current_client.configuration.dsn
+    Sentry.get_current_client.configuration.dsn = 'dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/42'
+    begin
+      example.run
+    ensure
+      Sentry.get_current_client.configuration.dsn = dsn.to_s.presence
+    end
+  end
+end