diff --git a/README.md b/README.md index 8d35c7d0..6285527d 100644 --- a/README.md +++ b/README.md @@ -74,9 +74,9 @@ The generator creates and runs a migration to add the necessary table to your database. It also mounts Maintenance Tasks in your `config/routes.rb`. By default the web UI can be accessed in the new `/maintenance_tasks` path. -In case you use an exception reporting service (e.g. Bugsnag) you might want to -define an error handler. See [Customizing the error -handler](#customizing-the-error-handler) for more information. +This gem using the Rails Error Reporter to report errors. If you are using a bug +tracking service you may want to subscribe to the reporter. See [Reporting Errors](#reporting-errors) +for more information. ### Active Job Dependency @@ -988,44 +988,32 @@ If you are stuck in `pausing` and wish to preserve your tasks's position There are a few configurable options for the gem. Custom configurations should be placed in a `maintenance_tasks.rb` initializer. -#### Customizing the error handler +#### Reporting errors Exceptions raised while a Task is performing are rescued and information about the error is persisted and visible in the UI. -If you want to integrate with an exception monitoring service (e.g. Bugsnag), -you can define an error handler: +Errors are also sent to the `Rails.error.reporter` we can be configured by your +application. See the [Error Reporting in Rails Applications](https://guides.rubyonrails.org/error_reporting.html) guide for more details. -```ruby -# config/initializers/maintenance_tasks.rb - -MaintenanceTasks.error_handler = ->(error, task_context, _errored_element) do - Bugsnag.notify(error) do |notification| - notification.add_metadata(:task, task_context) - end -end -``` - -The error handler should be a lambda that accepts three arguments: +Reports to the error reporter will contain the following data: * `error`: The exception that was raised. -* `task_context`: A hash with additional information about the Task and the +* `context`: A hash with additional information about the Task and the error: * `task_name`: The name of the Task that errored * `started_at`: The time the Task started * `ended_at`: The time the Task errored - - Note that `task_context` may be empty if the Task produced an error before any - context could be gathered (for example, if deserializing the job to process - your Task failed). -* `errored_element`: The element, if any, that was being processed when the Task - raised an exception. If you would like to pass this object to your exception - monitoring service, make sure you **sanitize the object** to avoid leaking - sensitive data and **convert it to a format** that is compatible with your bug - tracker. For example, Bugsnag only sends the id and class name of Active - Record objects in order to protect sensitive data. CSV rows, on the other - hand, are converted to strings and passed raw to Bugsnag, so make sure to - filter any personal data from these objects before adding them to a report. + * `errored_element`: The element, if any, that was being processed when the Task + raised an exception. If you would like to pass this object to your exception + monitoring service, make sure you **sanitize the object** to avoid leaking + sensitive data and **convert it to a format** that is compatible with your bug + tracker. +* `source`: This will be `maintenance-tasks` + +Note that `context` may be empty if the Task produced an error before any +context could be gathered (for example, if deserializing the job to process +your Task failed). #### Customizing the maintenance tasks module diff --git a/app/jobs/concerns/maintenance_tasks/task_job_concern.rb b/app/jobs/concerns/maintenance_tasks/task_job_concern.rb index ffe65a56..69f67f9a 100644 --- a/app/jobs/concerns/maintenance_tasks/task_job_concern.rb +++ b/app/jobs/concerns/maintenance_tasks/task_job_concern.rb @@ -183,9 +183,14 @@ def on_error(error) ended_at: @run.ended_at, } end - errored_element = @errored_element if defined?(@errored_element) + task_context[:errored_element] = @errored_element if defined?(@errored_element) ensure - MaintenanceTasks.error_handler.call(error, task_context, errored_element) + if MaintenanceTasks.instance_variable_get(:@error_handler) + errored_element = task_context.delete(:errored_element) + MaintenanceTasks.error_handler.call(error, task_context, errored_element) + else + Rails.error.report(error, context: task_context, source: "maintenance-tasks") + end end end end diff --git a/lib/maintenance_tasks.rb b/lib/maintenance_tasks.rb index 009158bf..8e180aa4 100644 --- a/lib/maintenance_tasks.rb +++ b/lib/maintenance_tasks.rb @@ -61,16 +61,6 @@ module MaintenanceTasks # use when cleaning a Run's backtrace. mattr_accessor :backtrace_cleaner - # @!attribute error_handler - # @scope class - # - # The callback to perform when an error occurs in the Task. See the - # {file:README#label-Customizing+the+error+handler} for details. - # - # @return [Proc] the callback to perform when an error occurs in the Task. - mattr_accessor :error_handler, default: - ->(_error, _task_context, _errored_element) {} - # @!attribute parent_controller # @scope class # @@ -94,4 +84,29 @@ module MaintenanceTasks # # @return [ActiveSupport::Duration] the threshold in seconds after which a task is considered stuck. mattr_accessor :stuck_task_duration, default: 5.minutes + + class << self + DEPRECATION_MESSAGE = "MaintenanceTasks.error_handler is deprecated and will be removed in the next release. " \ + "Instead, reports will be sent to the Rails error reporter. Do not set a handler and subscribe" \ + "to the error reporter instead." + + # no-doc + def error_handler + deprecator.warn(DEPRECATION_MESSAGE) + + @error_handler + end + + # no-doc + def error_handler=(proc) + deprecator.warn(DEPRECATION_MESSAGE) + + @error_handler = proc + end + + # no-doc + def deprecator + @deprecator ||= ActiveSupport::Deprecation.new + end + end end diff --git a/test/jobs/maintenance_tasks/task_job_test.rb b/test/jobs/maintenance_tasks/task_job_test.rb index 55195a64..cddf9132 100644 --- a/test/jobs/maintenance_tasks/task_job_test.rb +++ b/test/jobs/maintenance_tasks/task_job_test.rb @@ -344,64 +344,70 @@ class << self end end - test ".perform_now calls the error handler when there was an Error" do - error_handler_before = MaintenanceTasks.error_handler - handled_error = nil - handled_task_context = nil - handled_errored_element = nil - - MaintenanceTasks.error_handler = ->(error, task_context, errored_el) do - handled_error = error - handled_task_context = task_context - handled_errored_element = errored_el - end + test ".perform_now calls the error handler if one is set" do + MaintenanceTasks.deprecator.with(behavior: :silence) do + error_handler_before = MaintenanceTasks.error_handler + handled_error = nil + handled_task_context = nil + handled_errored_element = nil + + MaintenanceTasks.error_handler = ->(error, task_context, errored_el) do + handled_error = error + handled_task_context = task_context + handled_errored_element = errored_el + end - run = Run.create!(task_name: "Maintenance::ErrorTask") + run = Run.create!(task_name: "Maintenance::ErrorTask") - TaskJob.perform_now(run) + TaskJob.perform_now(run) - assert_equal(ArgumentError, handled_error.class) - assert_equal("Maintenance::ErrorTask", handled_task_context[:task_name]) - assert_equal(3, handled_errored_element) - ensure - MaintenanceTasks.error_handler = error_handler_before + assert_equal(ArgumentError, handled_error.class) + assert_equal("Maintenance::ErrorTask", handled_task_context[:task_name]) + assert_equal(3, handled_errored_element) + ensure + MaintenanceTasks.error_handler = error_handler_before + end end test ".perform_now still persists the error properly if the error handler raises" do - error_handler_before = MaintenanceTasks.error_handler - MaintenanceTasks.error_handler = ->(error, _task_context, _errored_el) do - raise error + MaintenanceTasks.deprecator.with(behavior: :silence) do + error_handler_before = MaintenanceTasks.error_handler + MaintenanceTasks.error_handler = ->(error, _task_context, _errored_el) do + raise error + end + run = Run.create!(task_name: "Maintenance::ErrorTask") + + assert_raises { TaskJob.perform_now(run) } + run.reload + + assert_predicate(run, :errored?) + assert_equal(2, run.tick_count) + ensure + MaintenanceTasks.error_handler = error_handler_before end + end + + test ".perform_now reports errors raised by the task" do run = Run.create!(task_name: "Maintenance::ErrorTask") - assert_raises { TaskJob.perform_now(run) } + assert_error_reported do + TaskJob.perform_now(run) + end run.reload assert_predicate(run, :errored?) assert_equal(2, run.tick_count) - ensure - MaintenanceTasks.error_handler = error_handler_before end - test ".perform_now handles case where run is not set and calls error handler" do - error_handler_before = MaintenanceTasks.error_handler - handled_error = nil - handled_task_context = nil - MaintenanceTasks.error_handler = ->(error, task_context, _errored_el) do - handled_error = error - handled_task_context = task_context - end - + test ".perform_now handles case where run is not set and reports error" do + TaskError = StandardError.new RaisingTaskJob = Class.new(TaskJob) do - before_perform(prepend: true) { raise "Uh oh!" } + before_perform(prepend: true) { raise TaskError } end - RaisingTaskJob.perform_now(@run) - - assert_equal("Uh oh!", handled_error.message) - assert_empty(handled_task_context) - ensure - MaintenanceTasks.error_handler = error_handler_before + assert_error_reported(TaskError) do + RaisingTaskJob.perform_now(@run) + end end test ".perform_now throttles when running Task that uses throttle_on" do