Remove redundant data in the context processor

Issue https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/846

lib/gitlab/error_tracking.rb

@@ -27,7 +27,7 @@ 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::ApplicationContextProcessor
+          config.processors << ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor
 
           # Sanitize authentication headers
           config.sanitize_http_headers = %w[Authorization Private-Token]
@@ -101,15 +101,15 @@ module Gitlab
       end
 
       def process_exception(exception, sentry: false, logging: true, extra:)
-        extra = build_exception_extra(exception, extra)
+        context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(exception, extra)
 
         if sentry && Raven.configuration.server
-          Raven.capture_exception(exception, extra: extra)
+          Raven.capture_exception(exception, **context_payload)
         end
 
         if logging
           log_hash = {}
-          Gitlab::ErrorTracking::LogFormatter.format!(exception, extra, log_hash)
+          Gitlab::ErrorTracking::LogFormatter.format!(log_hash, exception, context_payload)
           Gitlab::ErrorTracking::Logger.error(log_hash)
         end
       end
@@ -141,21 +141,6 @@ module Gitlab
           inject_context_for_exception(event, ex.cause) if ex.cause.present?
         end
       end
-
-      def build_exception_extra(exception, extra)
-        inline_extra = exception.try(:sentry_extra_data)
-        if inline_extra.present? && inline_extra.is_a?(Hash)
-          extra = extra.merge(inline_extra)
-        end
-
-        extra = sanitize_request_parameters(extra)
-        extra
-      end
-
-      def sanitize_request_parameters(parameters)
-        filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters)
-        filter.filter(parameters)
-      end
     end
   end
 end

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 = {})
+        payload = {}
+
+        append_extra!(payload, exception, extra)
+        append_tags!(payload)
+        append_user!(payload)
+
+        payload
+      end
+
+      private
+
+      def append_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
+
+        payload[:extra] = sanitize_request_parameters(extra)
+      end
+
+      def sanitize_request_parameters(parameters)
+        filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters)
+        filter.filter(parameters)
+      end
+
+      def append_tags!(payload)
+        payload[:tags] = {}
+        payload[:tags]
+          .merge!(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 append_user!(payload)
+        payload[:user] = {
+          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
+        ::Gitlab::ApplicationContext.current.to_h
+      end
+    end
+  end
+end

lib/gitlab/error_tracking/log_formatter.rb

@@ -3,40 +3,36 @@
 module Gitlab
   module ErrorTracking
     class LogFormatter
-      def self.format!(exception, extra, payload)
+      def self.format!(payload, exception, context_payload)
         Gitlab::ExceptionLogFormatter.format!(exception, payload)
-        append_context_to_log!(payload)
-        append_extra_to_log!(extra, payload)
+        append_user_to_log!(payload, context_payload)
+        append_tags_to_log!(payload, context_payload)
+        append_extra_to_log!(payload, context_payload)
       end
 
-      def self.append_context_to_log!(payload)
-        Raven.context.tags.each do |key, value|
-          payload["tags.#{key}"] = value
-        end
-
-        Raven.context.user.each do |key, value|
+      def self.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
 
-        current_context = ::Gitlab::ApplicationContext.current
-        payload.merge!(
-          'user.username' => current_context['meta.user'],
-          'tags.program' => Gitlab.process_name,
-          'tags.locale' => I18n.locale,
-          'tags.feature_category' => current_context['meta.feature_category']
-        )
-
-        payload
+      def self.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 self.append_extra_to_log!(extra, payload)
-        extra = Raven.context.extra.merge(extra).except(:server)
+      def self.append_extra_to_log!(payload, context_payload)
+        extra = Raven.context.extra.merge(context_payload[:extra])
+        extra = extra.except(:server)
 
         sidekiq_extra = extra[:sidekiq]
         if sidekiq_extra.is_a?(Hash) && sidekiq_extra.key?('args')
           sidekiq_extra = sidekiq_extra.dup
           sidekiq_extra['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor.loggable_arguments(
-            value['args'], value['class']
+            sidekiq_extra['args'], sidekiq_extra['class']
           )
         end
 

lib/gitlab/error_tracking/processor/application_context_processor.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module ErrorTracking
-    module Processor
-      class ApplicationContextProcessor < ::Raven::Processor
-        def process(event)
-          append_tags!(event)
-          append_user!(event)
-
-          event
-        end
-
-        private
-
-        def append_tags!(event)
-          event[:tags] ||= {}
-          event[:tags]
-            .merge!(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 append_user!(event)
-          event[:user] ||= {}
-          event[:user].merge!(
-            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
-          ::Gitlab::ApplicationContext.current.to_h
-        end
-      end
-    end
-  end
-end

lib/gitlab/error_tracking/processor/context_payload_processor.rb

+# frozen_string_literal: true
+
+require 'set'
+
+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, {})
+          context_payload.merge!(payload)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/error_tracking/processor/application_context_processor_spec.rb → spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb

@@ -3,8 +3,17 @@
 require 'spec_helper'
 require 'rspec-parameterized'
 
-RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
-  subject(:processor) { described_class.new }
+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')
@@ -14,37 +23,16 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
   context 'user metadata' do
     let(:user) { create(:user) }
 
-    context 'when event user metadata was not set' do
-      it 'appends username to the event metadata' do
-        event = {}
-
-        Gitlab::ApplicationContext.with_context(user: user) do
-          processor.process(event)
-        end
+    it 'appends user metadata to the payload' do
+      payload = {}
 
-        expect(event[:user]).to eql(
-          username: user.username
-        )
+      Gitlab::ApplicationContext.with_context(user: user) do
+        payload = generator.generate(exception, extra)
       end
-    end
 
-    context 'when event user metadata was already set' do
-      it 'appends username to the event metadata' do
-        event = {
-          user: {
-            ip_address: '127.0.0.1'
-          }
-        }
-
-        Gitlab::ApplicationContext.with_context(user: user) do
-          processor.process(event)
-        end
-
-        expect(event[:user]).to eql(
-          ip_address: '127.0.0.1',
-          username: user.username
-        )
-      end
+      expect(payload[:user]).to eql(
+        username: user.username
+      )
     end
   end
 
@@ -56,16 +44,18 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
 
       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
-        event = {}
+        payload = {}
 
         Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
-          processor.process(event)
+          payload = generator.generate(exception, extra)
         end
 
-        expect(event[:tags]).to eql(
+        expect(payload[:tags]).to eql(
           correlation_id: 'cid',
           locale: 'en',
           program: 'test',
@@ -77,14 +67,13 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
     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)
-
-        event = {}
+        payload = {}
 
         Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
-          processor.process(event)
+          payload = generator.generate(exception, extra)
         end
 
-        expect(event[:tags]).to eql(
+        expect(payload[:tags]).to eql(
           correlation_id: 'cid',
           locale: 'en',
           program: 'test',
@@ -96,6 +85,8 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
 
       it 'does not log into AppLogger' do
         expect(Gitlab::AppLogger).not_to receive(:debug)
+
+        generator.generate(exception, extra)
       end
     end
 
@@ -118,17 +109,17 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
         it 'logs into AppLogger' do
           expect(Gitlab::AppLogger).to receive(:debug).with(a_string_matching(error))
 
-          processor.process({})
+          generator.generate({})
         end
 
         it 'does not include any extra tags' do
-          event = {}
+          payload = {}
 
           Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do
-            processor.process(event)
+            payload = generator.generate(exception, extra)
           end
 
-          expect(event[:tags]).to eql(
+          expect(payload[:tags]).to eql(
             correlation_id: 'cid',
             locale: 'en',
             program: 'test',
@@ -138,4 +129,48 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ApplicationContextProcessor do
       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 embeded 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 senstive 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_spec.rb

@@ -8,23 +8,44 @@ RSpec.describe Gitlab::ErrorTracking do
   let(:exception) { RuntimeError.new('boom') }
   let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' }
 
-  let(:expected_payload_includes) do
-    [
-      { 'exception.class' => 'RuntimeError' },
-      { 'exception.message' => 'boom' },
-      { 'tags.correlation_id' => 'cid' },
-      { 'extra.some_other_info' => 'info' },
-      { 'extra.issue_url' => 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' }
-    ]
+  let(:sentry_payload) do
+    {
+      tags: {
+        program: 'test',
+        locale: 'en',
+        feature_category: nil,
+        correlation_id: 'cid'
+      },
+      user: {
+        username: nil
+      },
+      extra: {
+        some_other_info: 'info',
+        issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1'
+      }
+    }
   end
 
-  let(:sentry_event) { Gitlab::Json.parse(Raven.client.transport.events.last[1]) }
+  let(:logger_payload) do
+    {
+      'exception.class' => 'RuntimeError',
+      'exception.message' => 'boom',
+      'tags.program' => 'test',
+      'tags.locale' => 'en',
+      'tags.feature_category' => nil,
+      'tags.correlation_id' => 'cid',
+      'user.username' => nil,
+      'extra.some_other_info' => 'info',
+      'extra.issue_url' => 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1'
+    }
+  end
 
   before do
     stub_sentry_settings
 
     allow(described_class).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn)
     allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid')
+    allow(I18n).to receive(:locale).and_return('en')
 
     described_class.configure do |config|
       config.encoding = 'json'
@@ -38,10 +59,15 @@ RSpec.describe Gitlab::ErrorTracking do
       end
 
       it 'raises the exception' do
-        expect(Raven).to receive(:capture_exception)
-
-        expect { described_class.track_and_raise_for_dev_exception(exception) }
-          .to raise_error(RuntimeError)
+        expect(Raven).to receive(:capture_exception).with(exception, sentry_payload)
+
+        expect do
+          described_class.track_and_raise_for_dev_exception(
+            exception,
+            issue_url: issue_url,
+            some_other_info: 'info'
+          )
+        end.to raise_error(RuntimeError, /boom/)
       end
     end
 
@@ -51,19 +77,7 @@ RSpec.describe Gitlab::ErrorTracking do
       end
 
       it 'logs the exception with all attributes passed' do
-        expected_extras = {
-          some_other_info: 'info',
-          issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1'
-        }
-
-        expected_tags = {
-          correlation_id: 'cid'
-        }
-
-        expect(Raven).to receive(:capture_exception)
-                           .with(exception,
-                            tags: a_hash_including(expected_tags),
-                            extra: a_hash_including(expected_extras))
+        expect(Raven).to receive(:capture_exception).with(exception, sentry_payload)
 
         described_class.track_and_raise_for_dev_exception(
           exception,
@@ -73,8 +87,7 @@ RSpec.describe Gitlab::ErrorTracking do
       end
 
       it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do
-        expect(Gitlab::ErrorTracking::Logger).to receive(:error)
-          .with(a_hash_including(*expected_payload_includes))
+        expect(Gitlab::ErrorTracking::Logger).to receive(:error).with(logger_payload)
 
         described_class.track_and_raise_for_dev_exception(
           exception,
@@ -87,15 +100,19 @@ RSpec.describe Gitlab::ErrorTracking do
 
   describe '.track_and_raise_exception' do
     it 'always raises the exception' do
-      expect(Raven).to receive(:capture_exception)
+      expect(Raven).to receive(:capture_exception).with(exception, sentry_payload)
 
-      expect { described_class.track_and_raise_exception(exception) }
-        .to raise_error(RuntimeError)
+      expect do
+        described_class.track_and_raise_for_dev_exception(
+          exception,
+          issue_url: issue_url,
+          some_other_info: 'info'
+        )
+      end.to raise_error(RuntimeError, /boom/)
     end
 
     it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do
-      expect(Gitlab::ErrorTracking::Logger).to receive(:error)
-        .with(a_hash_including(*expected_payload_includes))
+      expect(Gitlab::ErrorTracking::Logger).to receive(:error).with(logger_payload)
 
       expect do
         described_class.track_and_raise_exception(
@@ -120,17 +137,16 @@ RSpec.describe Gitlab::ErrorTracking do
     it 'calls Raven.capture_exception' do
       track_exception
 
-      expect(Raven).to have_received(:capture_exception)
-                   .with(exception,
-                         tags: a_hash_including(correlation_id: 'cid'),
-                         extra: a_hash_including(some_other_info: 'info', issue_url: issue_url))
+      expect(Raven).to have_received(:capture_exception).with(
+        exception,
+        sentry_payload
+      )
     end
 
     it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do
       track_exception
 
-      expect(Gitlab::ErrorTracking::Logger).to have_received(:error)
-                                           .with(a_hash_including(*expected_payload_includes))
+      expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with(logger_payload)
     end
 
     context 'with filterable parameters' do
@@ -139,8 +155,9 @@ RSpec.describe Gitlab::ErrorTracking do
       it 'filters parameters' do
         track_exception
 
-        expect(Gitlab::ErrorTracking::Logger).to have_received(:error)
-                                             .with(hash_including({ 'extra.test' => 1, 'extra.my_token' => '[FILTERED]' }))
+        expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with(
+          hash_including({ 'extra.test' => 1, 'extra.my_token' => '[FILTERED]' })
+        )
       end
     end
 
@@ -151,8 +168,9 @@ RSpec.describe Gitlab::ErrorTracking do
       it 'includes the extra data from the exception in the tracking information' do
         track_exception
 
-        expect(Raven).to have_received(:capture_exception)
-                     .with(exception, a_hash_including(extra: a_hash_including(extra_info)))
+        expect(Raven).to have_received(:capture_exception).with(
+          exception, a_hash_including(extra: a_hash_including(extra_info))
+        )
       end
     end
 
@@ -163,8 +181,9 @@ RSpec.describe Gitlab::ErrorTracking do
       it 'just includes the other extra info' do
         track_exception
 
-        expect(Raven).to have_received(:capture_exception)
-                     .with(exception, a_hash_including(extra: a_hash_including(extra)))
+        expect(Raven).to have_received(:capture_exception).with(
+          exception, a_hash_including(extra: a_hash_including(extra))
+        )
       end
     end
 
@@ -176,7 +195,13 @@ RSpec.describe Gitlab::ErrorTracking do
           track_exception
 
           expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with(
-            hash_including({ 'extra.sidekiq' => { 'class' => 'PostReceive', 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] } }))
+            hash_including(
+              'extra.sidekiq' => {
+                'class' => 'PostReceive',
+                'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value']
+              }
+            )
+          )
         end
       end
 
@@ -186,9 +211,17 @@ RSpec.describe Gitlab::ErrorTracking do
         it 'filters sensitive arguments before sending' do
           track_exception
 
+          sentry_event = Gitlab::Json.parse(Raven.client.transport.events.last[1])
+
           expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq(['[FILTERED]', 1, 2])
           expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with(
-            hash_including('extra.sidekiq' => { 'class' => 'UnknownWorker', 'args' => ['[FILTERED]', '1', '2'] }))
+            hash_including(
+              'extra.sidekiq' => {
+                'class' => 'UnknownWorker',
+                'args' => ['[FILTERED]', '1', '2']
+              }
+            )
+          )
         end
       end
     end