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)

TESTING.md

@@ -42,7 +42,7 @@ aws configure --profile=opensource
 Subsequently you'll need to run the following commmand to authenticate with the registry:
 
 ```
-$(aws ecr get-login --profile=opensource --no-include-email)
+aws ecr get-login-password --profile=opensource | docker login --username AWS --password-stdin 855461928731.dkr.ecr.us-west-1.amazonaws.com
 ```
 
 __Your session will periodically expire__, so you'll need to run this command to re-authenticate when that happens.
@@ -64,7 +64,7 @@ Configure the tests to be run in the following way:
 When running the end-to-end tests, you'll want to restrict the feature files run to the specific test features for the platform.  This is done using the Cucumber CLI syntax at the end of the `docker-compose run ruby-maze-runner` command, i.e:
 
 ```
-RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner features/rails_features --tags "@rails6"
+RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run --use-aliases ruby-maze-runner features/rails_features --tags "@rails6"
 ```
 
 - Plain ruby tests should target `features/plain_features`
@@ -75,7 +75,7 @@ RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner featur
 In order to target specific features the exact `.feature` file can be specified, i.e:
 
 ```
-RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner features/rails_features/app_version.feature --tags "@rails6"
+RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run --use-aliases ruby-maze-runner features/rails_features/app_version.feature --tags "@rails6"
 ```
 
 In order to avoid running flakey or unfinished tests, the tag `"not @wip"` can be added to the tags option. This is recommended for all CI runs. If a tag is already specified, this should be added using the `and` keyword, e.g. `--tags "@rails6 and not @wip"`

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

features/fixtures/rails3/app/config/initializers/bugsnag.rb

@@ -11,10 +11,11 @@
   config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE"
   config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false"
   config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true"
+  config.meta_data_filters << 'filtered_parameter'
 
   if ENV["SQL_ONLY_BREADCRUMBS"] == "true"
     config.before_breadcrumb_callbacks << Proc.new do |breadcrumb|
       breadcrumb.ignore! unless breadcrumb.meta_data[:event_name] == "sql.active_record" && breadcrumb.meta_data[:name] == "User Load"
     end
   end
-end
+end

features/fixtures/rails4/app/config/initializers/bugsnag.rb

@@ -11,6 +11,7 @@
   config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE"
   config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false"
   config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true"
+  config.meta_data_filters << 'filtered_parameter'
 
   if ENV["SQL_ONLY_BREADCRUMBS"] == "true"
     config.before_breadcrumb_callbacks << Proc.new do |breadcrumb|

features/fixtures/rails5/app/config/initializers/bugsnag.rb

@@ -11,6 +11,7 @@
   config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE"
   config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false"
   config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true"
+  config.meta_data_filters << 'filtered_parameter'
 
   if ENV["SQL_ONLY_BREADCRUMBS"] == "true"
     config.before_breadcrumb_callbacks << Proc.new do |breadcrumb|

features/fixtures/rails6/app/config/initializers/bugsnag.rb

@@ -11,6 +11,7 @@
   config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE"
   config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false"
   config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true"
+  config.meta_data_filters << 'filtered_parameter'
 
   if ENV["SQL_ONLY_BREADCRUMBS"] == "true"
     config.before_breadcrumb_callbacks << Proc.new do |breadcrumb|

features/rails_features/meta_data_filters.feature

@@ -3,12 +3,14 @@ Feature: Metadata filters
 @rails3 @rails4 @rails5 @rails6
 Scenario: Meta_data_filters should include Rails.configuration.filter_parameters
   Given I start the rails service
-  When I navigate to the route "/metadata_filters/filter" on the rails app
+  When I navigate to the route "/metadata_filters/filter?filtered_parameter=foo&other_parameter=bar" on the rails app
   And I wait to receive a request
   Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier"
   And the event "unhandled" is false
   And the exception "errorClass" equals "RuntimeError"
   And the exception "message" starts with "handled string"
   And the event "app.type" equals "rails"
-  And the event "metaData.request.url" ends with "/metadata_filters/filter"
+  And the event "metaData.request.url" ends with "/metadata_filters/filter?filtered_parameter=[FILTERED]&other_parameter=bar"
   And the event "metaData.my_specific_filter" equals "[FILTERED]"
+  And the event "metaData.request.params.filtered_parameter" equals "[FILTERED]"
+  And the event "metaData.request.params.other_parameter" equals "bar"

lib/bugsnag.rb

@@ -33,6 +33,7 @@
 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]
@@ -63,14 +64,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 +99,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 +114,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 +209,40 @@ 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?
 
-        # Return early if ignored
-        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?
 
-        # 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?
+      # Add to breadcrumbs buffer if still valid
+      configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore?
+    end
+
+    ##
+    # Returns the client's Cleaner object, or creates one if not yet created.
+    #
+    # @api private
+    #
+    # @return [Cleaner]
+    def cleaner
+      @cleaner = nil unless defined?(@cleaner)
+      @cleaner || LOCK.synchronize do
+        @cleaner ||= Bugsnag::Cleaner.new(configuration)
       end
     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 +260,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 +310,23 @@ 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)
+      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"]