Improve payload cleaning performance

.rubocop.yml

@@ -24,12 +24,27 @@ Lint/RaiseException:
 Lint/StructNewOverride:
   Enabled: true
 
+Lint/DeprecatedOpenSSLConstant:
+  Enabled: true
+
+Lint/MixedRegexpCaptureTypes:
+  Enabled: true
+
+Style/RedundantFetchBlock:
+  Enabled: true
+
 Style/ExponentialNotation:
   Enabled: false
 
 Style/HashEachMethods:
   Enabled: true
 
+Style/RedundantRegexpCharacterClass:
+  Enabled: true
+
+Style/RedundantRegexpEscape:
+  Enabled: true
+
 # These require newer version of Ruby than our minimum supported version, so
 # have to be disabled
 Style/HashTransformKeys: # Requires Ruby 2.5

CHANGELOG.md

@@ -11,7 +11,11 @@ Changelog
 * The Breadcrumb name limit of 30 characters has been removed
   | [#600](https://github.com/bugsnag/bugsnag-ruby/pull/600)
 
+* Improve performance of payload cleaning
+  | [#601](https://github.com/bugsnag/bugsnag-ruby/pull/601)
+
 ### Deprecated
+
 * The `ignore_classes` configuration option has been deprecated in favour of `discard_classes`. `ignore_classes` will be removed in the next major release
 
 ## 6.13.1 (11 May 2020)

dockerfiles/Dockerfile.ruby-maze-runner

@@ -20,7 +20,7 @@ COPY spec ./spec
 RUN gem build bugsnag.gemspec
 
 # The maze-runner node tests
-FROM 855461928731.dkr.ecr.us-west-1.amazonaws.com/maze-runner:v2-cli as ruby-maze-runner
+FROM 855461928731.dkr.ecr.us-west-1.amazonaws.com/maze-runner:latest-cli as ruby-maze-runner
 WORKDIR /app/
 COPY features ./features
 COPY --from=ruby-package-builder /app/bugsnag-*.gem bugsnag.gem

lib/bugsnag.rb

@@ -33,11 +33,13 @@
 require "bugsnag/breadcrumbs/breadcrumb"
 require "bugsnag/breadcrumbs/breadcrumbs"
 
+# rubocop:todo Metrics/ModuleLength
 module Bugsnag
   LOCK = Mutex.new
   INTEGRATIONS = [:resque, :sidekiq, :mailman, :delayed_job, :shoryuken, :que, :mongo]
 
   NIL_EXCEPTION_DESCRIPTION = "'nil' was notified as an exception"
+  SCOPES_TO_FILTER = ['events.metaData', 'events.breadcrumbs.metaData'].freeze
 
   class << self
     ##
@@ -63,14 +65,15 @@ def notify(exception, auto_notify=false, &block)
         auto_notify = false
       end
 
-      return unless deliver_notification?(exception, auto_notify)
+      return unless should_deliver_notification?(exception, auto_notify)
 
       exception = NIL_EXCEPTION_DESCRIPTION if exception.nil?
 
       report = Report.new(exception, configuration, auto_notify)
 
       # If this is an auto_notify we yield the block before the any middleware is run
       yield(report) if block_given? && auto_notify
+
       if report.ignore?
         configuration.debug("Not notifying #{report.exceptions.last[:errorClass]} due to ignore being signified in auto_notify block")
         return
@@ -97,6 +100,7 @@ def notify(exception, auto_notify=false, &block)
         # If this is not an auto_notify then the block was provided by the user. This should be the last
         # block that is run as it is the users "most specific" block.
         yield(report) if block_given? && !auto_notify
+
         if report.ignore?
           configuration.debug("Not notifying #{report.exceptions.last[:errorClass]} due to ignore being signified in user provided block")
           return
@@ -111,13 +115,7 @@ def notify(exception, auto_notify=false, &block)
           report.severity_reason = initial_reason
         end
 
-        # Deliver
-        configuration.info("Notifying #{configuration.notify_endpoint} of #{report.exceptions.last[:errorClass]}")
-        options = {:headers => report.headers}
-        payload = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(report.as_json))
-        Bugsnag::Delivery[configuration.delivery_method].deliver(configuration.notify_endpoint, payload, configuration, options)
-        report_summary = report.summary
-        leave_breadcrumb(report_summary[:error_class], report_summary, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto)
+        deliver_notification(report)
       end
     end
 
@@ -212,27 +210,27 @@ def leave_breadcrumb(name, meta_data={}, type=Bugsnag::Breadcrumbs::MANUAL_BREAD
       validator.validate(breadcrumb)
 
       # Skip if it's already invalid
-      unless breadcrumb.ignore?
-        # Run callbacks
-        configuration.before_breadcrumb_callbacks.each do |c|
-          c.arity > 0 ? c.call(breadcrumb) : c.call
-          break if breadcrumb.ignore?
-        end
+      return if breadcrumb.ignore?
+
+      # Run callbacks
+      configuration.before_breadcrumb_callbacks.each do |c|
+        c.arity > 0 ? c.call(breadcrumb) : c.call
+        break if breadcrumb.ignore?
+      end
 
-        # Return early if ignored
-        return if breadcrumb.ignore?
+      # Return early if ignored
+      return if breadcrumb.ignore?
 
-        # Validate again in case of callback alteration
-        validator.validate(breadcrumb)
+      # Validate again in case of callback alteration
+      validator.validate(breadcrumb)
 
-        # Add to breadcrumbs buffer if still valid
-        configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore?
-      end
+      # Add to breadcrumbs buffer if still valid
+      configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore?
     end
 
     private
 
-    def deliver_notification?(exception, auto_notify)
+    def should_deliver_notification?(exception, auto_notify)
       reason = abort_reason(exception, auto_notify)
       configuration.debug(reason) unless reason.nil?
       reason.nil?
@@ -250,6 +248,32 @@ def abort_reason(exception, auto_notify)
       end
     end
 
+    ##
+    # Deliver the notification to Bugsnag
+    #
+    # @param report [Report]
+    # @return void
+    def deliver_notification(report)
+      configuration.info("Notifying #{configuration.notify_endpoint} of #{report.exceptions.last[:errorClass]}")
+
+      payload = report_to_json(report)
+      options = {:headers => report.headers}
+
+      Bugsnag::Delivery[configuration.delivery_method].deliver(
+        configuration.notify_endpoint,
+        payload,
+        configuration,
+        options
+      )
+
+      leave_breadcrumb(
+        report.summary[:error_class],
+        report.summary,
+        Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE,
+        :auto
+      )
+    end
+
     # Check if the API key is valid and warn (once) if it is not
     def check_key_valid
       @key_warning = false unless defined?(@key_warning)
@@ -274,7 +298,28 @@ def check_endpoint_setup
         raise ArgumentError, "The session endpoint cannot be modified without the notify endpoint"
       end
     end
+
+    ##
+    # Convert the Report object to JSON
+    #
+    # We ensure the report is safe to send by removing recursion, fixing
+    # encoding errors and redacting metadata according to "meta_data_filters"
+    #
+    # @param report [Report]
+    # @return string
+    def report_to_json(report)
+      cleaner = Cleaner.new(
+        configuration.meta_data_filters,
+        SCOPES_TO_FILTER
+      )
+
+      cleaned = cleaner.clean_object(report.as_json)
+      trimmed = Bugsnag::Helpers.trim_if_needed(cleaned)
+
+      ::JSON.dump(trimmed)
+    end
   end
 end
+# rubocop:enable Metrics/ModuleLength
 
 Bugsnag.load_integrations unless ENV["BUGSNAG_DISABLE_AUTOCONFIGURE"]

lib/bugsnag/cleaner.rb

@@ -7,9 +7,13 @@ class Cleaner
     OBJECT = '[OBJECT]'.freeze
     RAISED = '[RAISED]'.freeze
 
-    def initialize(filters)
+    ##
+    # @param filters [Set<String, Regexp>]
+    # @param scopes_to_filter [Array<String>]
+    def initialize(filters, scopes_to_filter)
       @filters = Array(filters)
       @deep_filters = @filters.any? {|f| f.kind_of?(Regexp) && f.to_s.include?("\\.".freeze) }
+      @scopes_to_filter = scopes_to_filter
     end
 
     def clean_object(obj)
@@ -28,12 +32,14 @@ def traverse_object(obj, seen, scope)
       value = case obj
       when Hash
         clean_hash = {}
-        obj.each do |k,v|
+        obj.each do |k, v|
           begin
-            if filters_match_deeply?(k, scope)
+            current_scope = [scope, k].compact.join('.')
+
+            if filters_match_deeply?(k, current_scope)
               clean_hash[k] = FILTERED
             else
-              clean_hash[k] = traverse_object(v, seen, [scope, k].compact.join('.'))
+              clean_hash[k] = traverse_object(v, seen, current_scope)
             end
           # If we get an error here, we assume the key needs to be filtered
           # to avoid leaking things we shouldn't. We also remove the key itself
@@ -88,7 +94,7 @@ def clean_string(str)
     end
 
     def self.clean_object_encoding(obj)
-      new(nil).clean_object(obj)
+      new(Set.new, []).clean_object(obj)
     end
 
     def clean_url(url)
@@ -112,6 +118,9 @@ def clean_url(url)
 
     private
 
+    ##
+    # @param key [String, #to_s]
+    # @return [Boolean]
     def filters_match?(key)
       str = key.to_s
 
@@ -125,15 +134,30 @@ def filters_match?(key)
       end
     end
 
+    ##
     # If someone has a Rails filter like /^stuff\.secret/, it won't match "request.params.stuff.secret",
     # so we try it both with and without the "request.params." bit.
+    #
+    # @param key [String, #to_s]
+    # @param scope [String]
+    # @return [Boolean]
     def filters_match_deeply?(key, scope)
+      return false unless scope_should_be_filtered?(scope)
+
       return true if filters_match?(key)
       return false unless @deep_filters
 
-      long = [scope, key].compact.join('.')
-      short = long.sub(/^request\.params\./, '')
-      filters_match?(long) || filters_match?(short)
+      short = scope.sub(/^request\.params\./, '')
+      filters_match?(scope) || filters_match?(short)
+    end
+
+    ##
+    # Should the given scope be filtered according to our 'scopes_to_filter'?
+    #
+    # @param scope [String]
+    # @return [Boolean]
+    def scope_should_be_filtered?(scope)
+      @scopes_to_filter.any? {|scope_to_filter| scope.start_with?(scope_to_filter) }
     end
   end
 end

lib/bugsnag/configuration.rb

@@ -91,7 +91,7 @@ class Configuration
     DEFAULT_MAX_BREADCRUMBS = 25
 
     # Path to vendored code. Used to mark file paths as out of project.
-    DEFAULT_VENDOR_PATH = %r{^(vendor\/|\.bundle\/)}
+    DEFAULT_VENDOR_PATH = %r{^(vendor/|\.bundle/)}
 
     alias :track_sessions :auto_capture_sessions
     alias :track_sessions= :auto_capture_sessions=

lib/bugsnag/helpers.rb

@@ -17,12 +17,10 @@ module Helpers
     def self.trim_if_needed(value)
       value = "" if value.nil?
 
-      # Sanitize object
-      sanitized_value = Bugsnag::Cleaner.clean_object_encoding(value)
-      return sanitized_value unless payload_too_long?(sanitized_value)
+      return value unless payload_too_long?(value)
 
       # Trim metadata
-      reduced_value = trim_metadata(sanitized_value)
+      reduced_value = trim_metadata(value)
       return reduced_value unless payload_too_long?(reduced_value)
 
       # Trim code from stacktrace

lib/bugsnag/middleware/rack_request.rb

@@ -28,7 +28,7 @@ def call(report)
         url = "#{request.scheme}://#{request.host}"
         url << ":#{request.port}" unless [80, 443].include?(request.port)
 
-        cleaner = Bugsnag::Cleaner.new(report.configuration.meta_data_filters)
+        cleaner = Bugsnag::Cleaner.new(report.configuration.meta_data_filters, [])
 
         # If app is passed a bad URL, this code will crash attempting to clean it
         begin

lib/bugsnag/report.rb

@@ -97,33 +97,23 @@ def as_json
           releaseStage: release_stage,
           type: app_type
         },
+        breadcrumbs: breadcrumbs.map(&:to_h),
         context: context,
         device: {
           hostname: hostname,
           runtimeVersions: runtime_versions
         },
         exceptions: exceptions,
         groupingHash: grouping_hash,
+        metaData: meta_data,
         session: session,
         severity: severity,
         severityReason: severity_reason,
         unhandled: @unhandled,
         user: user
       }
 
-      # cleanup character encodings
-      payload_event = Bugsnag::Cleaner.clean_object_encoding(payload_event)
-
-      # filter out sensitive values in (and cleanup encodings) metaData
-      filter_cleaner = Bugsnag::Cleaner.new(configuration.meta_data_filters)
-      payload_event[:metaData] = filter_cleaner.clean_object(meta_data)
-      payload_event[:breadcrumbs] = breadcrumbs.map do |breadcrumb|
-        breadcrumb_hash = breadcrumb.to_h
-        breadcrumb_hash[:metaData] = filter_cleaner.clean_object(breadcrumb_hash[:metaData])
-        breadcrumb_hash
-      end
-
-      payload_event.reject! {|k,v| v.nil? }
+      payload_event.reject! {|k, v| v.nil? }
 
       # return the payload hash
       {

lib/bugsnag/stacktrace.rb

@@ -9,6 +9,8 @@ class Stacktrace
 
     ##
     # Process a backtrace and the configuration into a parsed stacktrace.
+    #
+    # rubocop:todo Metrics/CyclomaticComplexity
     def initialize(backtrace, configuration)
       @configuration = configuration
 
@@ -64,6 +66,7 @@ def initialize(backtrace, configuration)
         end
       end.compact
     end
+    # rubocop:enable Metrics/CyclomaticComplexity
 
     ##
     # Returns the processed backtrace