Supply additional task info as argument when calling error handler

[adrianna-chang-shopify]

Closes: #309

This PR changes the call to the defined error handler to pass along an additional task_context argument. This arg is a hash containing some additional information about the Task and the corresponding run.

An example of how users can leverage this info has been added to the README: if they integrate with Bugsnag, the simplest thing to do would just be to add a tab containing the details of the Task's context when notifying.

We might want to think a bit more about what information it makes sense to expose. Assuming that most users are using Sidekiq for their queuing backend, a lot of the information pertaining to the job is exposed through the job_context (as shown in the original issue) - job_id, cursor, job_class, etc... I thought it might make sense to expose started_at and ended_at (although enqueued_at is available from the job context). Alternatively, we could expose as much information about the Task as possible (include job_id, cursor, time_running, etc...) under the assumption that not everyone will use Sidekiq and have access to all this additional information to augment their error notifier.

cc @timgladwell

[timgladwell]

Thanks @adrianna-chang-shopify!

Would it also be possible to include some kind of detail about the specific entity being processed when the error occurred? I'm thinking that would make it easier to track down and identify the cause of the error.

[adrianna-chang-shopify]

Hey @timgladwell , that should be doable! I'll make the adjustment to this PR

[adrianna-chang-shopify]

@etiennebarrie I've made the following changes:

• Modified 6f246d0 to change the order of the arguments to error_handler.call, and to add the arity check
• Added 8f72aed to introduce a deprecation warning for the old error handler signature. I think it's worth having people move over to this new expected API to eventually be able to remove that extra arity check in TaskJob.
• Added 15b4444 to accomplish what @timgladwell requested. I opted to set it as an ivar on the TaskJob instance so we can pass it as part of the task_context, but I've been debating whether this is something we'd actually like to expose in the UI as well (and consequently persist to the @run.) I think it could bring a lot of value in terms of helping users figure out why something happened with their Task, but does involve a schema change... WDYT?

[etiennebarrie]

Regarding the processed element which caused the error:

• we have the cursor_position, which we could save when we have an error and it would have an up-to-date value of the cursor, pointing to the element (meaning if we rebuild the enumerator, and take the first element, it should be the one that caused the error), so I don't think we need to persist the element, but we may want to properly save the cursor_position upon error.
• the PII question worries me a bit, and I'm thinking as a result it should probably be a third parameter to the error handler so that application owners are more explicitly handling it. But then the processed element type will depend on each task, so it may seem natural to call back the task so that it knows what to do with it, but that adds to the Task API. Maybe we could let app developers deal with it though, and not prescribe anything, e.g.:

MaintenanceTasks.error_handler = ->(error, context, element) do
  # they could go full in, calling back the Task
  context[:element] = Task.named(context[:task_name]).sanitize(element)
  # or just generic
  context[:element] = "#{element.class} ##{element.id}" if element.is_a?(ActiveRecord::Base)
  context[:element] = row.to_s.hash if element.is_a?(CSV::Row)

  # Bugsnag.notify ...
end

Ultimately, the cursor gives developers the way to dive deeper without taking any risk about sending PII to the bug tracker. The problem is that we've hidden the cursor from the API so well that they may not know what to do with it.

[etiennebarrie]

I know we try to be minimalist for this project, so going with 2 parameters to the handler makes sense, but this is a situation where a block is the perfect tool since it doesn't care about the arity, so my preferred API for this would be:

MaintenanceTasks.error_handler { |error| Bugsnag.notify(error) }
MaintenanceTasks.error_handler { |error, context|
  Bugsnag.notify(error) { _1.add_tab(:task, context) } }
MaintenanceTasks.error_handler { |error, context, element|
  Bugsnag.notify(error) { _1.add_tab(:task, context.merge(element: element)) } }

That way we give the pieces to our users and they can handler them the way they want. And the documentation could give as an example the second case. That way people copy/pasting would not get into any risk of sending PII.

[adrianna-chang-shopify]

Fair enough, I'm okay to go with a block! 👍 I thought maybe it would be less confusing to enforce an error handler with a specific signature to ensure consistency and to make users aware of all the possible info that comes in when an error triggers a callback, but I think documenting the different use cases in the README is sufficient.

I'll get this updated!

[etiennebarrie]

Let's squash the commits, but everything looks good!