Merge branch...

Merge branch 'qmnguyen0711/846-sentry-merge-sentry-s-contexts-and-users-into-applicationcontext' into 'master'

Merge Sentry's contexts and users into Application Context

See merge request gitlab-org/gitlab!53691

app/controllers/application_controller.rb

@@ -29,7 +29,6 @@ class ApplicationController < ActionController::Base
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration, if: :html_request?
   before_action :ldap_security_check
-  around_action :sentry_context
   before_action :default_headers
   before_action :default_cache_headers
   before_action :add_gon_variables, if: :html_request?
@@ -171,7 +170,12 @@ class ApplicationController < ActionController::Base
   end
 
   def log_exception(exception)
-    Gitlab::ErrorTracking.track_exception(exception)
+    # At this point, the controller already exits set_current_context around
+    # block. To maintain the context while handling error exception, we need to
+    # set the context again
+    set_current_context do
+      Gitlab::ErrorTracking.track_exception(exception)
+    end
 
     backtrace_cleaner = request.env["action_dispatch.backtrace_cleaner"]
     application_trace = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, exception).application_trace
@@ -528,10 +532,6 @@ class ApplicationController < ActionController::Base
       .execute
   end
 
-  def sentry_context(&block)
-    Gitlab::ErrorTracking.with_context(current_user, &block)
-  end
-
   def allow_gitaly_ref_name_caching
     ::Gitlab::GitalyClient.allow_ref_name_caching do
       yield

app/services/merge_requests/base_service.rb

@@ -181,7 +181,7 @@ module MergeRequests
       }
 
       if exception
-        Gitlab::ErrorTracking.with_context(current_user) do
+        Gitlab::ApplicationContext.with_context(user: current_user) do
           Gitlab::ErrorTracking.track_exception(exception, data)
         end
 

changelogs/unreleased/qmnguyen0711-846-sentry-merge-sentry-s-contexts-and-users-into-applicatio.yml

+---
+title: Merge Sentry's contexts into Gitlab::ApplicationContext
+merge_request: 53691
+author:
+type: changed

ee/lib/gitlab/database/load_balancing/service_discovery.rb

@@ -59,7 +59,7 @@ module Gitlab
                 rescue => error
                   # Any exceptions that might occur should be reported to
                   # Sentry, instead of silently terminating this thread.
-                  Raven.capture_exception(error)
+                  Gitlab::ErrorTracking.track_exception(error)
 
                   Gitlab::AppLogger.error(
                     "Service discovery encountered an error: #{error.message}"

ee/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb

@@ -66,8 +66,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do
         .to receive(:refresh_if_necessary)
         .and_raise(error)
 
-      expect(Raven)
-        .to receive(:capture_exception)
+      expect(Gitlab::ErrorTracking)
+        .to receive(:track_exception)
         .with(error)
 
       expect(service)

lib/api/helpers.rb

@@ -467,7 +467,7 @@ module API
     def handle_api_exception(exception)
       if report_exception?(exception)
         define_params_for_grape_middleware
-        Gitlab::ErrorTracking.with_context(current_user) do
+        Gitlab::ApplicationContext.with_context(user: current_user) do
           Gitlab::ErrorTracking.track_exception(exception)
         end
       end

lib/gitlab/application_context.rb

@@ -27,8 +27,12 @@ module Gitlab
       Labkit::Context.push(application_context.to_lazy_hash)
     end
 
+    def self.current
+      Labkit::Context.current.to_h
+    end
+
     def self.current_context_include?(attribute_name)
-      Labkit::Context.current.to_h.include?(Labkit::Context.log_key(attribute_name))
+      current.include?(Labkit::Context.log_key(attribute_name))
     end
 
     def initialize(**args)

lib/gitlab/error_tracking.rb

@@ -27,33 +27,16 @@ module Gitlab
           config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)
           config.processors << ::Gitlab::ErrorTracking::Processor::SidekiqProcessor
           config.processors << ::Gitlab::ErrorTracking::Processor::GrpcErrorProcessor
+          config.processors << ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor
 
           # Sanitize authentication headers
           config.sanitize_http_headers = %w[Authorization Private-Token]
-          config.tags = extra_tags_from_env.merge(program: Gitlab.process_name)
           config.before_send = method(:before_send)
 
           yield config if block_given?
         end
       end
 
-      def with_context(current_user = nil)
-        last_user_context = Raven.context.user
-
-        user_context = {
-          id: current_user&.id,
-          email: current_user&.email,
-          username: current_user&.username
-        }.compact
-
-        Raven.tags_context(default_tags)
-        Raven.user_context(user_context)
-
-        yield
-      ensure
-        Raven.user_context(last_user_context)
-      end
-
       # This should be used when you want to passthrough exception handling:
       # rescue and raise to be catched in upper layers of the application.
       #
@@ -118,37 +101,20 @@ module Gitlab
       end
 
       def process_exception(exception, sentry: false, logging: true, extra:)
-        exception.try(:sentry_extra_data)&.tap do |data|
-          extra = extra.merge(data) if data.is_a?(Hash)
-        end
-
-        extra = sanitize_request_parameters(extra)
+        context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(exception, extra)
 
         if sentry && Raven.configuration.server
-          Raven.capture_exception(exception, tags: default_tags, extra: extra)
+          Raven.capture_exception(exception, **context_payload)
         end
 
         if logging
-          # TODO: this logic could migrate into `Gitlab::ExceptionLogFormatter`
-          # and we could also flatten deep nested hashes if required for search
-          # (e.g. if `extra` includes hash of hashes).
-          # In the current implementation, we don't flatten multi-level folded hashes.
-          log_hash = {}
-          Raven.context.tags.each { |name, value| log_hash["tags.#{name}"] = value }
-          Raven.context.user.each { |name, value| log_hash["user.#{name}"] = value }
-          Raven.context.extra.merge(extra).each { |name, value| log_hash["extra.#{name}"] = value }
-
-          Gitlab::ExceptionLogFormatter.format!(exception, log_hash)
+          formatter = Gitlab::ErrorTracking::LogFormatter.new
+          log_hash = formatter.generate_log(exception, context_payload)
 
           Gitlab::ErrorTracking::Logger.error(log_hash)
         end
       end
 
-      def sanitize_request_parameters(parameters)
-        filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters)
-        filter.filter(parameters)
-      end
-
       def sentry_dsn
         return unless Rails.env.production? || Rails.env.development?
         return unless Gitlab.config.sentry.enabled
@@ -160,22 +126,6 @@ module Gitlab
         Rails.env.development? || Rails.env.test?
       end
 
-      def default_tags
-        {
-          Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id,
-          locale: I18n.locale
-        }
-      end
-
-      # Static tags that are set on application start
-      def extra_tags_from_env
-        Gitlab::Json.parse(ENV.fetch('GITLAB_SENTRY_EXTRA_TAGS', '{}')).to_hash
-      rescue => e
-        Gitlab::AppLogger.debug("GITLAB_SENTRY_EXTRA_TAGS could not be parsed as JSON: #{e.class.name}: #{e.message}")
-
-        {}
-      end
-
       # Group common, mostly non-actionable exceptions by type and message,
       # rather than cause
       def custom_fingerprinting(event, ex)

lib/gitlab/error_tracking/context_payload_generator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ErrorTracking
+    class ContextPayloadGenerator
+      def self.generate(exception, extra = {})
+        new.generate(exception, extra)
+      end
+
+      def generate(exception, extra = {})
+        {
+          extra: extra_payload(exception, extra),
+          tags: tags_payload,
+          user: user_payload
+        }
+      end
+
+      private
+
+      def extra_payload(exception, extra)
+        inline_extra = exception.try(:sentry_extra_data)
+        if inline_extra.present? && inline_extra.is_a?(Hash)
+          extra = extra.merge(inline_extra)
+        end
+
+        sanitize_request_parameters(extra)
+      end
+
+      def sanitize_request_parameters(parameters)
+        filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters)
+        filter.filter(parameters)
+      end
+
+      def tags_payload
+        extra_tags_from_env.merge!(
+          program: Gitlab.process_name,
+          locale: I18n.locale,
+          feature_category: current_context['meta.feature_category'],
+          Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id
+        )
+      end
+
+      def user_payload
+        {
+          username: current_context['meta.user']
+        }
+      end
+
+      # Static tags that are set on application start
+      def extra_tags_from_env
+        Gitlab::Json.parse(ENV.fetch('GITLAB_SENTRY_EXTRA_TAGS', '{}')).to_hash
+      rescue => e
+        Gitlab::AppLogger.debug("GITLAB_SENTRY_EXTRA_TAGS could not be parsed as JSON: #{e.class.name}: #{e.message}")
+
+        {}
+      end
+
+      def current_context
+        # In case Gitlab::ErrorTracking is used when the app starts
+        return {} unless defined?(::Gitlab::ApplicationContext)
+
+        ::Gitlab::ApplicationContext.current.to_h
+      end
+    end
+  end
+end

lib/gitlab/error_tracking/log_formatter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ErrorTracking
+    class LogFormatter
+      # Note: all the accesses to Raven's contexts here are to keep the
+      # backward-compatibility to Sentry's built-in integrations. In future,
+      # they can be removed.
+      def generate_log(exception, context_payload)
+        payload = {}
+
+        Gitlab::ExceptionLogFormatter.format!(exception, payload)
+        append_user_to_log!(payload, context_payload)
+        append_tags_to_log!(payload, context_payload)
+        append_extra_to_log!(payload, context_payload)
+
+        payload
+      end
+
+      private
+
+      def append_user_to_log!(payload, context_payload)
+        user_context = Raven.context.user.merge(context_payload[:user])
+        user_context.each do |key, value|
+          payload["user.#{key}"] = value
+        end
+      end
+
+      def append_tags_to_log!(payload, context_payload)
+        tags_context = Raven.context.tags.merge(context_payload[:tags])
+        tags_context.each do |key, value|
+          payload["tags.#{key}"] = value
+        end
+      end
+
+      def append_extra_to_log!(payload, context_payload)
+        extra = Raven.context.extra.merge(context_payload[:extra])
+        extra = extra.except(:server)
+
+        # The extra value for sidekiq is a hash whose keys are strings.
+        if extra[:sidekiq].is_a?(Hash) && extra[:sidekiq].key?('args')
+          sidekiq_extra = extra.delete(:sidekiq)
+          sidekiq_extra['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor.loggable_arguments(
+            sidekiq_extra['args'], sidekiq_extra['class']
+          )
+          payload["extra.sidekiq"] = sidekiq_extra
+        end
+
+        extra.each do |key, value|
+          payload["extra.#{key}"] = value
+        end
+      end
+    end
+  end
+end

lib/gitlab/error_tracking/processor/context_payload_processor.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ErrorTracking
+    module Processor
+      class ContextPayloadProcessor < ::Raven::Processor
+        # This processor is added to inject application context into Sentry
+        # events generated by Sentry built-in integrations. When the
+        # integrations are re-implemented and use Gitlab::ErrorTracking, this
+        # processor should be removed.
+        def process(payload)
+          context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(nil, {})
+          payload.deep_merge!(context_payload)
+        end
+      end
+    end
+  end
+end

lib/gitlab/exception_log_formatter.rb

@@ -12,16 +12,6 @@ module Gitlab
         'exception.message' => exception.message
       )
 
-      payload.delete('extra.server')
-
-      payload['extra.sidekiq'].tap do |value|
-        if value.is_a?(Hash) && value.key?('args')
-          value = value.dup
-          payload['extra.sidekiq']['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor
-                                               .loggable_arguments(value['args'], value['class'])
-        end
-      end
-
       if exception.backtrace
         payload['exception.backtrace'] = Rails.backtrace_cleaner.clean(exception.backtrace)
       end

spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+RSpec.describe Gitlab::ErrorTracking::ContextPayloadGenerator do
+  subject(:generator) { described_class.new }
+
+  let(:extra) do
+    {
+      some_other_info: 'info',
+      issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1'
+    }
+  end
+
+  let(:exception) { StandardError.new("Dummy exception") }
+
+  before do
+    allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid')
+    allow(I18n).to receive(:locale).and_return('en')
+  end
+
+  context 'user metadata' do
+    let(:user) { create(:user) }
+
+    it 'appends user metadata to the payload' do
+      payload = {}
+
+      Gitlab::ApplicationContext.with_context(user: user) do
+        payload = generator.generate(exception, extra)
+      end
+
+      expect(payload[:user]).to eql(
+        username: user.username
+      )
+    end
+  end
+
+  context 'tags metadata' do
+    context 'when the GITLAB_SENTRY_EXTRA_TAGS env is not set' do
+      before do
+        stub_env('GITLAB_SENTRY_EXTRA_TAGS', nil)
+      end
+
+      it 'does not log into AppLogger' do
+        expect(Gitlab::AppLogger).not_to receive(:debug)
+
+        generator.generate(exception, extra)
+      end
+
+      it 'does not send any extra tags' do
+        payload = {}
+
+        Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
+          payload = generator.generate(exception, extra)
+        end
+
+        expect(payload[:tags]).to eql(
+          correlation_id: 'cid',
+          locale: 'en',
+          program: 'test',
+          feature_category: 'feature_a'
+        )
+      end
+    end
+
+    context 'when the GITLAB_SENTRY_EXTRA_TAGS env is a JSON hash' do
+      it 'includes those tags in all events' do
+        stub_env('GITLAB_SENTRY_EXTRA_TAGS', { foo: 'bar', baz: 'quux' }.to_json)
+        payload = {}
+
+        Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
+          payload = generator.generate(exception, extra)
+        end
+
+        expect(payload[:tags]).to eql(
+          correlation_id: 'cid',
+          locale: 'en',
+          program: 'test',
+          feature_category: 'feature_a',
+          'foo' => 'bar',
+          'baz' => 'quux'
+        )
+      end
+
+      it 'does not log into AppLogger' do
+        expect(Gitlab::AppLogger).not_to receive(:debug)
+
+        generator.generate(exception, extra)
+      end
+    end
+
+    context 'when the GITLAB_SENTRY_EXTRA_TAGS env is not a JSON hash' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:env_var, :error) do
+        { foo: 'bar', baz: 'quux' }.inspect | 'JSON::ParserError'
+        [].to_json | 'NoMethodError'
+        [%w[foo bar]].to_json | 'NoMethodError'
+        %w[foo bar].to_json | 'NoMethodError'
+        '"string"' | 'NoMethodError'
+      end
+
+      with_them do
+        before do
+          stub_env('GITLAB_SENTRY_EXTRA_TAGS', env_var)
+        end
+
+        it 'logs into AppLogger' do
+          expect(Gitlab::AppLogger).to receive(:debug).with(a_string_matching(error))
+
+          generator.generate({})
+        end
+
+        it 'does not include any extra tags' do
+          payload = {}
+
+          Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
+            payload = generator.generate(exception, extra)
+          end
+
+          expect(payload[:tags]).to eql(
+            correlation_id: 'cid',
+            locale: 'en',
+            program: 'test',
+            feature_category: 'feature_a'
+          )
+        end
+      end
+    end
+  end
+
+  context 'extra metadata' do
+    it 'appends extra metadata to the payload' do
+      payload = generator.generate(exception, extra)
+
+      expect(payload[:extra]).to eql(
+        some_other_info: 'info',
+        issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1'
+      )
+    end
+
+    it 'appends exception embedded extra metadata to the payload' do
+      allow(exception).to receive(:sentry_extra_data).and_return(
+        some_other_info: 'another_info',
+        mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1'
+      )
+
+      payload = generator.generate(exception, extra)
+
+      expect(payload[:extra]).to eql(
+        some_other_info: 'another_info',
+        issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1',
+        mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1'
+      )
+    end
+
+    it 'filters sensitive extra info' do
+      extra[:my_token] = '456'
+      allow(exception).to receive(:sentry_extra_data).and_return(
+        mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1',
+        another_token: '1234'
+      )
+
+      payload = generator.generate(exception, extra)
+
+      expect(payload[:extra]).to eql(
+        some_other_info: 'info',
+        issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1',
+        mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1',
+        my_token: '[FILTERED]',
+        another_token: '[FILTERED]'
+      )
+    end
+  end
+end

spec/lib/gitlab/error_tracking/log_formatter_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::ErrorTracking::LogFormatter do
+  let(:exception) { StandardError.new('boom') }
+  let(:context_payload) do
+    {
+      server: 'local-hostname-of-the-server',
+      user: {
+        ip_address: '127.0.0.1',
+        username: 'root'
+      },
+      tags: {
+        locale: 'en',
+        feature_category: 'category_a'
+      },
+      extra: {
+        some_other_info: 'other_info',
+        sidekiq: {
+          'class' => 'HelloWorker',
+          'args' => ['senstive string', 1, 2],
+          'another_field' => 'field'
+        }
+      }
+    }
+  end
+
+  before do
+    Raven.context.user[:user_flag] = 'flag'
+    Raven.context.tags[:shard] = 'catchall'
+    Raven.context.extra[:some_info] = 'info'
+
+    allow(exception).to receive(:backtrace).and_return(
+      [
+        'lib/gitlab/file_a.rb:1',
+        'lib/gitlab/file_b.rb:2'
+      ]
+    )
+  end
+
+  after do
+    ::Raven::Context.clear!
+  end
+
+  it 'appends error-related log fields and filters sensitive Sidekiq arguments' do
+    payload = described_class.new.generate_log(exception, context_payload)
+
+    expect(payload).to eql(
+      'exception.class' => 'StandardError',
+      'exception.message' => 'boom',
+      'exception.backtrace' => [
+        'lib/gitlab/file_a.rb:1',
+        'lib/gitlab/file_b.rb:2'
+      ],
+      'user.ip_address' => '127.0.0.1',
+      'user.username' => 'root',
+      'user.user_flag' => 'flag',
+      'tags.locale' => 'en',
+      'tags.feature_category' => 'category_a',
+      'tags.shard' => 'catchall',
+      'extra.some_other_info' => 'other_info',
+      'extra.some_info' => 'info',
+      "extra.sidekiq" => {
+        "another_field" => "field",
+        "args" => ["[FILTERED]", "1", "2"],
+        "class" => "HelloWorker"
+      }
+    )
+  end
+end

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

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Gitlab::ErrorTracking::Processor::ContextPayloadProcessor do
+  subject(:processor) { described_class.new }
+
+  before do
+    allow_next_instance_of(Gitlab::ErrorTracking::ContextPayloadGenerator) do |generator|
+      allow(generator).to receive(:generate).and_return(
+        user: { username: 'root' },
+        tags: { locale: 'en', program: 'test', feature_category: 'feature_a', correlation_id: 'cid' },
+        extra: { some_info: 'info' }
+      )
+    end
+  end
+
+  it 'merges the context payload into event payload' do
+    payload = {
+      user: { ip_address: '127.0.0.1' },
+      tags: { priority: 'high' },
+      extra: { sidekiq: { class: 'SomeWorker', args: ['[FILTERED]', 1, 2] } }
+    }
+
+    processor.process(payload)
+
+    expect(payload).to eql(
+      user: {
+        ip_address: '127.0.0.1',
+        username: 'root'
+      },
+      tags: {
+        priority: 'high',
+        locale: 'en',
+        program: 'test',
+        feature_category: 'feature_a',
+        correlation_id: 'cid'
+      },
+      extra: {
+        some_info: 'info',
+        sidekiq: { class: 'SomeWorker', args: ['[FILTERED]', 1, 2] }
+      }
+    )
+  end
+end

spec/models/upload_spec.rb

@@ -221,7 +221,7 @@ RSpec.describe Upload do
         it 'does not send a message to Sentry' do
           upload = described_class.new(path: "#{__FILE__}-nope", store: ObjectStorage::Store::LOCAL)
 
-          expect(Raven).not_to receive(:capture_message)
+          expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
 
           upload.exist?
         end

spec/requests/api/helpers_spec.rb

@@ -314,14 +314,13 @@ RSpec.describe API::Helpers do
 
       expect(Gitlab::ErrorTracking).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn)
       Gitlab::ErrorTracking.configure
-      Raven.client.configuration.encoding = 'json'
     end
 
     it 'does not report a MethodNotAllowed exception to Sentry' do
       exception = Grape::Exceptions::MethodNotAllowed.new({ 'X-GitLab-Test' => '1' })
       allow(exception).to receive(:backtrace).and_return(caller)
 
-      expect(Raven).not_to receive(:capture_exception).with(exception)
+      expect(Gitlab::ErrorTracking).not_to receive(:track_exception).with(exception)
 
       handle_api_exception(exception)
     end
@@ -330,8 +329,7 @@ RSpec.describe API::Helpers do
       exception = RuntimeError.new('test error')
       allow(exception).to receive(:backtrace).and_return(caller)
 
-      expect(Raven).to receive(:capture_exception).with(exception, tags:
-        a_hash_including(correlation_id: 'new-correlation-id'), extra: {})
+      expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception)
 
       Labkit::Correlation::CorrelationId.use_id('new-correlation-id') do
         handle_api_exception(exception)
@@ -357,20 +355,6 @@ RSpec.describe API::Helpers do
         expect(json_response['message']).to start_with("\nRuntimeError (Runtime Error!):")
       end
     end
-
-    context 'extra information' do
-      # Sentry events are an array of the form [auth_header, data, options]
-      let(:event_data) { Raven.client.transport.events.first[1] }
-
-      it 'sends the params, excluding confidential values' do
-        expect(ProjectsFinder).to receive(:new).and_raise('Runtime Error!')
-
-        get api('/projects', user), params: { password: 'dont_send_this', other_param: 'send_this' }
-
-        expect(event_data).to include('other_param=send_this')
-        expect(event_data).to include('password=********')
-      end
-    end
   end
 
   describe '.authenticate_non_get!' do