Refactor Sentry handlingimprove-exception-handling

17 files changed, 150 insertions, 97 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 140a625d333..fd39dfcdc8d 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -18,7 +18,7 @@ class ApplicationController < ActionController::Base
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
   before_action :ldap_security_check
-  before_action :sentry_context
+  around_action :sentry_context
   before_action :default_headers
   before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
   before_action :configure_permitted_parameters, if: :devise_controller?
@@ -152,7 +152,7 @@ class ApplicationController < ActionController::Base
   end
 
   def log_exception(exception)
-    Gitlab::Sentry.track_acceptable_exception(exception)
+    Gitlab::Sentry.report_exception(exception)
 
     backtrace_cleaner = request.env["action_dispatch.backtrace_cleaner"]
     application_trace = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, exception).application_trace
diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb
index 4e11772b252..29c437503d6 100644
--- a/app/helpers/icons_helper.rb
+++ b/app/helpers/icons_helper.rb
@@ -42,11 +42,9 @@ module IconsHelper
   end
 
   def sprite_icon(icon_name, size: nil, css_class: nil)
-    if Gitlab::Sentry.should_raise_for_dev?
-      unless known_sprites.include?(icon_name)
-        exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg")
-        raise exception
-      end
+    unless known_sprites.include?(icon_name)
+      exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg")
+      Gitlab::Sentry.handle_exception(exception)
     end
 
     css_classes = []
diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb
index 73c1402eae5..0cecbd9373c 100644
--- a/app/helpers/users_helper.rb
+++ b/app/helpers/users_helper.rb
@@ -57,7 +57,7 @@ module UsersHelper
 
     unless user.association(:status).loaded?
       exception = RuntimeError.new("Status was not preloaded")
-      Gitlab::Sentry.track_exception(exception, extra: { user: user.inspect })
+      Gitlab::Sentry.handle_exception(exception, extra: { user: user.inspect })
     end
 
     return unless user.status
diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb
index 05cd4265133..5a0beb1d34a 100644
--- a/app/models/concerns/group_descendant.rb
+++ b/app/models/concerns/group_descendant.rb
@@ -52,7 +52,7 @@ module GroupDescendant
       }
       issue_url = 'https://gitlab.com/gitlab-org/gitlab-ce/issues/40785'
 
-      Gitlab::Sentry.track_exception(exception, issue_url: issue_url, extra: extras)
+      Gitlab::Sentry.handle_exception(exception, issue_url: issue_url, extra: extras)
     end
 
     if parent.nil? && hierarchy_top.present?
diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb
index 498996f4f80..d1dd46a8d13 100644
--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -38,7 +38,7 @@ module Storage
         write_projects_repository_config
       rescue => e
         # Raise if development/test environment, else just notify Sentry
-        Gitlab::Sentry.track_exception(e, extra: { full_path_was: full_path_was, full_path: full_path, action: 'move_dir' })
+        Gitlab::Sentry.handle_exception(e, extra: { full_path_was: full_path_was, full_path: full_path, action: 'move_dir' })
       end
 
       true # false would cancel later callbacks but not rollback
diff --git a/app/models/upload.rb b/app/models/upload.rb
index 20860f14b83..a3cd8ff5221 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -70,10 +70,8 @@ class Upload < ActiveRecord::Base
 
     # Help sysadmins find missing upload files
     if persisted? && !exist
-      if Gitlab::Sentry.enabled?
-        Raven.capture_message("Upload file does not exist", extra: self.attributes)
-      end
-
+      exception = RuntimeError.new("Uploaded file does not exist")
+      Gitlab::Sentry.report_exception(exception, extra: self.attributes)
       Gitlab::Metrics.counter(:upload_file_does_not_exist_total, 'The number of times an upload record could not find its file').increment
     end
 
diff --git a/app/services/clusters/applications/base_helm_service.rb b/app/services/clusters/applications/base_helm_service.rb
index e86ca8cf1d0..88b3842c265 100644
--- a/app/services/clusters/applications/base_helm_service.rb
+++ b/app/services/clusters/applications/base_helm_service.rb
@@ -23,7 +23,7 @@ module Clusters
         }
 
         logger.error(meta)
-        Gitlab::Sentry.track_acceptable_exception(error, extra: meta)
+        Gitlab::Sentry.report_exception(error, extra: meta)
       end
 
       def logger
diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb
index deb94d7dbce..140f7b93c57 100644
--- a/config/initializers/forbid_sidekiq_in_transactions.rb
+++ b/config/initializers/forbid_sidekiq_in_transactions.rb
@@ -28,7 +28,7 @@ module Sidekiq
                 MSG
               rescue Sidekiq::Worker::EnqueueFromTransactionError => e
                 ::Rails.logger.error(e.message) if ::Rails.env.production?
-                Gitlab::Sentry.track_exception(e)
+                Gitlab::Sentry.handle_exception(e)
               end
             end
 
diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb
index 2a6c5148f71..81a8491be32 100644
--- a/config/initializers/sentry.rb
+++ b/config/initializers/sentry.rb
@@ -2,26 +2,25 @@
 
 require 'gitlab/current_settings'
 
-def configure_sentry
-  # allow it to fail: it may do so when create_from_defaults is executed before migrations are actually done
-  begin
-    sentry_enabled = Gitlab::CurrentSettings.current_application_settings.sentry_enabled
-  rescue
-    sentry_enabled = false
-  end
+# allow it to fail: it may do so when create_from_defaults is executed before migrations are actually done
+begin
+  sentry_enabled = Gitlab::CurrentSettings.current_application_settings.sentry_enabled
+rescue
+  sentry_enabled = false
+end
 
-  if sentry_enabled
-    Raven.configure do |config|
-      config.dsn = Gitlab::CurrentSettings.current_application_settings.sentry_dsn
-      config.release = Gitlab.revision
+program_context = 
+  if Sidekiq.server?
+    'sidekiq'
+  else
+    'rails'
+  end
 
-      # Sanitize fields based on those sanitized from Rails.
-      config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)
-      # Sanitize authentication headers
-      config.sanitize_http_headers = %w[Authorization Private-Token]
-      config.tags = { program: Gitlab::Sentry.program_context }
-    end
+sentry_dsn =
+  if sentry_enabled && Rails.env.production?
+    Gitlab::CurrentSettings.current_application_settings.sentry_dsn
   end
-end
 
-configure_sentry if Rails.env.production? || Rails.env.development?
+Gitlab::Sentry.configure!(
+  sentry_dsn: sentry_dsn,
+  program: program_context)
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 6c1a730935a..f0f8d2b3ce1 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -372,8 +372,9 @@ module API
     def handle_api_exception(exception)
       if report_exception?(exception)
         define_params_for_grape_middleware
-        Gitlab::Sentry.context(current_user)
-        Gitlab::Sentry.track_acceptable_exception(exception, extra: params)
+        Gitlab::Sentry.in_context(current_user) do
+          Gitlab::Sentry.report_exception(exception, extra: params)
+        end
       end
 
       # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb
index d2484217ab9..21360d28d82 100644
--- a/lib/gitlab/diff/highlight.rb
+++ b/lib/gitlab/diff/highlight.rb
@@ -35,7 +35,7 @@ module Gitlab
             # match the blob, which is a bug. But we shouldn't fail to render
             # completely in that case, even though we want to report the error.
             rescue RangeError => e
-              Gitlab::Sentry.track_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/45441')
+              Gitlab::Sentry.handle_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/45441')
             end
           end
 
diff --git a/lib/gitlab/highlight.rb b/lib/gitlab/highlight.rb
index a4e60bbd828..a4c2fe2b3f2 100644
--- a/lib/gitlab/highlight.rb
+++ b/lib/gitlab/highlight.rb
@@ -61,7 +61,7 @@ module Gitlab
       tokens = lexer.lex(text, continue: continue)
       Timeout.timeout(timeout_time) { @formatter.format(tokens, tag: tag).html_safe }
     rescue Timeout::Error => e
-      Gitlab::Sentry.track_exception(e)
+      Gitlab::Sentry.handle_exception(e)
       highlight_plain(text)
     rescue
       highlight_plain(text)
diff --git a/lib/gitlab/sentry.rb b/lib/gitlab/sentry.rb
index 46d01964eac..8fbfe219ae1 100644
--- a/lib/gitlab/sentry.rb
+++ b/lib/gitlab/sentry.rb
@@ -1,67 +1,113 @@
 # frozen_string_literal: true
 
 module Gitlab
-  module Sentry
-    def self.enabled?
-      (Rails.env.production? || Rails.env.development?) &&
-        Gitlab::CurrentSettings.sentry_enabled?
-    end
+  class Sentry
+    class << self
+      attr_reader :sentry_enabled, :program
+      attr_accessor :user_context
+
+      def configure!(sentry_dsn:, program:)
+        @sentry_enabled = sentry_dsn.present?
+        @program = program
+
+        Raven.configure do |config|
+          config.dsn = dsn
+          config.release = Gitlab.revision
+    
+          # Sanitize fields based on those sanitized from Rails.
+          config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)
+          # Sanitize authentication headers
+          config.sanitize_http_headers = %w[Authorization Private-Token]
+          config.tags = { program: program }
+        end if sentry_enabled
+      end
 
-    def self.context(current_user = nil)
-      return unless enabled?
+      def in_context(current_user = nil)
+        last_context = self.user_context
 
-      Raven.tags_context(locale: I18n.locale)
+        begin
+          self.user_context = {
+            id: current_user.id,
+            email: current_user.email,
+            username: current_user.username
+          } if current_user
 
-      if current_user
-        Raven.user_context(
-          id: current_user.id,
-          email: current_user.email,
-          username: current_user.username
-        )
+          yield
+        ensure
+          self.user_context = last_context
+        end
       end
-    end
 
-    # This can be used for investigating exceptions that can be recovered from in
-    # code. The exception will still be raised in development and test
-    # environments.
-    #
-    # That way we can track down these exceptions with as much information as we
-    # need to resolve them.
-    #
-    # Provide an issue URL for follow up.
-    def self.track_exception(exception, issue_url: nil, extra: {})
-      track_acceptable_exception(exception, issue_url: issue_url, extra: extra)
-
-      raise exception if should_raise_for_dev?
-    end
+      # This can be used for investigating exceptions that can be recovered from in
+      # code. The exception will still be raised in development and test
+      # environments.
+      #
+      # That way we can track down these exceptions with as much information as we
+      # need to resolve them.
+      #
+      # Provide an issue URL for follow up.
+      def handle_exception(exception, issue_url: nil, extra: {})
+        report_exception(exception, issue_url: issue_url, extra: extra)
 
-    # This should be used when you do not want to raise an exception in
-    # development and test. If you need development and test to behave
-    # just the same as production you can use this instead of
-    # track_exception.
-    def self.track_acceptable_exception(exception, issue_url: nil, extra: {})
-      if enabled?
+        raise exception if should_raise_for_dev?
+      end
+
+      # This should be used when you do not want to raise an exception in
+      # development and test. If you need development and test to behave
+      # just the same as production you can use this instead of
+      # handle_exception.
+      def report_exception(exception, issue_url: nil, extra: {})
         extra[:issue_url] = issue_url if issue_url
-        context # Make sure we've set everything we know in the context
 
-        tags = {
-          Gitlab::CorrelationId::LOG_KEY.to_sym => Gitlab::CorrelationId.current_id
-        }
+        if sentry_enabled
+          tags = {
+            Gitlab::CorrelationId::LOG_KEY.to_sym => Gitlab::CorrelationId.current_id
+          }
 
-        Raven.capture_exception(exception, tags: tags, extra: extra)
+          Raven.capture_exception(exception, tags: tags, extra: extra)
+        else
+          # otherwise, send it to log file
+          details = extra.merge(user_context.to_h).merge(
+            exception_details(exception))
+
+          logger.error(details)
+        end
       end
-    end
 
-    def self.program_context
-      if Sidekiq.server?
-        'sidekiq'
-      else
-        'rails'
+      private
+
+      def user_context= (value)
+        @user_context = value
+
+        if sentry_enabled
+          Raven.tags_context(locale: I18n.locale)
+          Raven.user_context(*value)
+        end
+      end
+
+      def should_raise_for_dev?
+        Rails.env.development? || Rails.env.test?
+      end
+
+      def logger
+        Gitlab::Sentry::Logger.build
       end
-    end
 
-    def self.should_raise_for_dev?
-      Rails.env.development? || Rails.env.test?
+      def exception_details(exception)
+        { class: exception.class.to_s,
+          message: exception.message,
+          backtrace: filter_backtrace(exception.backtrace)
+        }
+      end
+
+      # filter backtrace to only include GitLab directories
+      def filter_backtrace(backtrace, limit: 5)
+        root_path = Rails.root.to_s + '/'
+
+        backtrace.select do |line|
+          line.start_with?(root_path)
+        end.first(limit)
+      end
     end
   end
 end
diff --git a/lib/gitlab/sentry/logger.rb b/lib/gitlab/sentry/logger.rb
new file mode 100644
index 00000000000..74dce0fa341
--- /dev/null
+++ b/lib/gitlab/sentry/logger.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Gitlab
+  class Sentry
+    class Logger < ::Gitlab::JsonLogger
+      def self.file_name_noext
+        'exceptions_json'
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/diff/highlight_spec.rb b/spec/lib/gitlab/diff/highlight_spec.rb
index 5d0a603d11d..176aeaec0e5 100644
--- a/spec/lib/gitlab/diff/highlight_spec.rb
+++ b/spec/lib/gitlab/diff/highlight_spec.rb
@@ -103,7 +103,7 @@ describe Gitlab::Diff::Highlight do
         end
 
         it 'keeps the original rich line' do
-          allow(Gitlab::Sentry).to receive(:track_exception)
+          allow(Gitlab::Sentry).to receive(:handle_exception)
 
           code = %q{+      raise RuntimeError, "System commands must be given as an array of strings"}
 
@@ -112,7 +112,7 @@ describe Gitlab::Diff::Highlight do
         end
 
         it 'reports to Sentry if configured' do
-          expect(Gitlab::Sentry).to receive(:track_exception).and_call_original
+          expect(Gitlab::Sentry).to receive(:handle_exception).and_call_original
 
           expect { subject }. to raise_exception(RangeError)
         end
diff --git a/spec/lib/gitlab/sentry_spec.rb b/spec/lib/gitlab/sentry_spec.rb
index 1128eaf8560..faae9f1577c 100644
--- a/spec/lib/gitlab/sentry_spec.rb
+++ b/spec/lib/gitlab/sentry_spec.rb
@@ -11,7 +11,7 @@ describe Gitlab::Sentry do
     end
   end
 
-  describe '.track_exception' do
+  describe '.handle_exception' do
     let(:exception) { RuntimeError.new('boom') }
 
     before do
@@ -20,7 +20,7 @@ describe Gitlab::Sentry do
 
     it 'raises the exception if it should' do
       expect(described_class).to receive(:should_raise_for_dev?).and_return(true)
-      expect { described_class.track_exception(exception) }
+      expect { described_class.handle_exception(exception) }
         .to raise_error(RuntimeError)
     end
 
@@ -45,7 +45,7 @@ describe Gitlab::Sentry do
                             tags: a_hash_including(expected_tags),
                             extra: a_hash_including(expected_extras))
 
-        described_class.track_exception(
+        described_class.handle_exception(
           exception,
           issue_url: 'http://gitlab.com/gitlab-org/gitlab-ce/issues/1',
           extra: { some_other_info: 'info' }
@@ -55,12 +55,12 @@ describe Gitlab::Sentry do
       it 'sets the context' do
         expect(described_class).to receive(:context)
 
-        described_class.track_exception(exception)
+        described_class.handle_exception(exception)
       end
     end
   end
 
-  context '.track_acceptable_exception' do
+  context '.report_exception' do
     let(:exception) { RuntimeError.new('boom') }
 
     before do
@@ -83,7 +83,7 @@ describe Gitlab::Sentry do
                           tags: a_hash_including(expected_tags),
                           extra: a_hash_including(expected_extras))
 
-      described_class.track_acceptable_exception(
+      described_class.report_exception(
         exception,
         issue_url: 'http://gitlab.com/gitlab-org/gitlab-ce/issues/1',
         extra: { some_other_info: 'info' }
diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb
index 28352d8c961..b59ca24bbf4 100644
--- a/spec/models/concerns/group_descendant_spec.rb
+++ b/spec/models/concerns/group_descendant_spec.rb
@@ -80,7 +80,7 @@ describe GroupDescendant, :nested_groups do
       end
 
       it 'tracks the exception when a parent was not preloaded' do
-        expect(Gitlab::Sentry).to receive(:track_exception).and_call_original
+        expect(Gitlab::Sentry).to receive(:handle_exception).and_call_original
 
         expect { GroupDescendant.build_hierarchy([subsub_group]) }.to raise_error(ArgumentError)
       end
@@ -89,7 +89,7 @@ describe GroupDescendant, :nested_groups do
         expected_hierarchy = { parent => { subgroup => subsub_group } }
 
         # this does not raise in production, so stubbing it here.
-        allow(Gitlab::Sentry).to receive(:track_exception)
+        allow(Gitlab::Sentry).to receive(:handle_exception)
 
         expect(GroupDescendant.build_hierarchy([subsub_group])).to eq(expected_hierarchy)
       end