send active support notifications on run events

[elfassy]

closes #747

Why

Callbacks on the task are not meant to be used for instrumentation across all tasks. Task callbacks also don't have access to the Run object, preventing this logic from accessing the metadata or other information stored on the run. This makes it difficult to set notifications for status changes, for instance to send slack messages when a job is completed

What

Send active support notifications on the Run status changes

      maintenance_tasks.enqueued,    # The task has been enqueued by the user.
      maintenance_tasks.succeeded,   # The task finished without error.
      maintenance_tasks.cancelled,   # The user explicitly halted the task's execution.
      maintenance_tasks.paused,      # The task was paused in the middle of the run by the user.
      maintenance_tasks.errored,     # The task code produced an unhandled exception.

Example

ActiveSupport::Notifications.subscribe("maintenance_tasks.succeeded") do |*args|
       payload = args.last
       run = payload[:run]
       run.task_name
       run.arguments
       run.metadata
end

ActiveSupport::Notifications.subscribe("maintenance_tasks.errored") do |*args|
       payload = args.last
       run = payload[:run]
       run.task_name
       run.arguments
       run.error_message
       run.error_class
end

# or

class MaintenanceTasksInstrumenter < ActiveSupport::Subscriber
 attach_to :maintenance_tasks
 
 def enqueued(event)
   run = event.payload[:run]
   SlackNotifier.broadcast(SLACK_CHANNEL, "Job #{run.task_name} was started by #{run.metadata[:user_email]}} with arguments #{run.arguments.to_s.truncate(255)}")
 end
 
 def errored(event)
   run = event.payload[:run]
   SlackNotifier.broadcast(SLACK_CHANNEL, "Job #{run.task_name} halted with an error")
 end
 
 def succeeded(event)
   run = event.payload[:run]
   # do something anytime a maintenance task succeeds
   SlackNotifier.broadcast(SLACK_CHANNEL, "Job #{run.task_name} completed successfully")
 end
 
 ...
end

[fridaynosaur]

Nice changes, re-read everything and it looks awesome.

[ryanseys]

Why not these events?

[elfassy]

original discussion here #961 (comment). TLDR: these states are easily confused with other states, and there's no use case that we know of for them b499351. This commit can easily be reversed if need be

these states are easily confused with other states. For example
- running vs enqueued
- pausing vs paused
- cancelling vs cancelled
- interrupted vs paused

If someone has a valid use case for these states, feel free to revert this commit and add them back

[ryanseys]

LGTM, couple questions

[nvasilevski]

I understand the idea to ensure that issuing a notification will not result in the whole flow being halted but I think this rescue overly defensive. Realistically we don't expect anything to go wrong in this method but rescue will swallow all the problems including even NoMethodError

[etiennebarrie]

We need it because if any of the subscriber blocks raise, the instrument calls will raise, which could stop the job/task. We can't restrict the exceptions we want to rescue because instrument will only raise ActiveSupport::Notifications::InstrumentationSubscriberError if there's more than one exception. So if there's only one subscriber and it raises, or if there are multiple but only one raises, the error is re-raised as-is.

[nvasilevski]

We need it because if any of the subscriber blocks raise

Doesn't it mean that subscribers should be the one to be defensive?
Otherwise it sounds like everything, including Rails itself should accompany every instrument call with a rescue block due to subscriber potentially raising.

Perhaps I'm missing something?

[elfassy]

To Etienne's point, unrelated tests start failing if I remove this rescue

Error:
MaintenanceTasks::TaskJobTest#test_.perform_now_pauses_run_when_race_occurs_between_interrupting_and_pausing_run:
ActiveRecord::RecordInvalid: Validation failed: Status Cannot transition run from status errored to pausing
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/validations.rb:84:in `raise_validation_error'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/validations.rb:55:in `save!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:313:in `block in save!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:365:in `block in with_transaction_returning_status'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/connection_adapters/abstract/database_statements.rb:342:in `transaction'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:361:in `with_transaction_returning_status'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:313:in `save!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/suppressor.rb:56:in `save!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/persistence.rb:906:in `block in update!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:365:in `block in with_transaction_returning_status'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/connection_adapters/abstract/database_statements.rb:342:in `transaction'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/transactions.rb:361:in `with_transaction_returning_status'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/persistence.rb:904:in `update!'
    /Users/michaelelfassy/.gem/ruby/3.3.0/gems/activerecord-7.1.3/lib/active_record/enum.rb:313:in `block in define_enum_methods'
    test/jobs/maintenance_tasks/task_job_test.rb:516:in `block (2 levels) in <class:TaskJobTest>'

[nvasilevski]

Does it mean that rescue was rescuing ActiveRecord::RecordInvalid: Validation failed: Status Cannot transition run from status errored to pausing ?

Doesn't it prove my point that we will start rescuing errors that have nothing to do with issuing a notification and may indicate a real issue with the flow?

[elfassy]

@nvasilevski you're correct, this exception is due to an invalid mock and was getting swallowed. I'm not sure if we want to raise only in tests or if we want to remove the rescue all together

[nvasilevski]

I'm still in favor of removing it because I believe it's subscriber's responsibility to catch any potential issues and the gem should not swallow it if means breaking tasks's run state. But I could have misunderstood @etiennebarrie point so let me chat with him!

[etiennebarrie]

Yeah removing the rescue clause is probably what makes the most sense. If we were to keep it, we should find a way to re-raise the exception from the job after the data is correctly persisted. Maybe we could add the rescue to the readme example to show the best practice that prevents the instrumentation block from raising.

[adrianna-chang-shopify]

Thanks for working on this! ❤️ One small request for additional docs, and let's wait to see if anyone else on the team has feedback, but otherwise 👍

[nvasilevski]

Let's squash the commits before merging please