Better Error Reporting

[skukx]

I'm not sure how the community feels about this but I would be willing to work on this myself if there is enough interest. Currently many of the Solidus errors are logged like so:

Rails.logger.error(<String>)

# Or

Rails.logger.error(e.message)

This is fine in a lot of cases, however in certain cases this is not optimal. The reason I say it isn't optimal is because stack trace is eliminated making it harder to debug. When using reporting services such as Rollbar it is nice to have those stack traces.

Non-Optimal Examples

Better Example

Summary

When ever we rescue an error we should call Rails.logger.error(error) or Rails.logger.warn(error). Here are some examples where this practice could be implemented.

solidus/frontend/app/controllers/spree/store_controller.rb

Lines 25 to 28 in 4857815

	rescue Spree::OrderMutex::LockFailed
	flash[:error] = t('spree.order_mutex_error')
	redirect_to spree.cart_path
	end

solidus/backend/app/controllers/spree/admin/payments_controller.rb

Lines 49 to 52 in 859143f

	rescue Spree::Core::GatewayError => e
	flash[:error] = e.message.to_s
	redirect_to new_admin_order_payment_path(@order)
	end

solidus/backend/app/controllers/spree/admin/payments_controller.rb

Lines 49 to 52 in 859143f

	rescue Spree::Core::GatewayError => e
	flash[:error] = e.message.to_s
	redirect_to new_admin_order_payment_path(@order)
	end

Again this is nothing major but more of a quality of life change.

[dhonig]

@skukx Really glad you brought this up. Really good suggestions on the changes. It would be great to modernize how errors are reported. I'm not sure anyone has given this some good thinking in ages.

[tvdeyen]

@skukx thanks. Absolutely. Error handling needs some overhaul.

But instead of change the way we log, we should think about adding a global error handler, because like you said we want to be able to report to error tracking services or log, or do both. Having an configurable error handler (that defaults to Rails.logger.error) would be a huge benefit.

[mtomov]

https://youtu.be/9R4wlyWBP1k

There's a cool guy with an opinion on the subject 😀

Sample implementation:

class ErrorHandler
  class << self
    def report(error, context = {}, options = {})
      Honeybadger.notify(error, options.merge(context: context))

      severity = options[:severity] || :error
      message = error.respond_to?(:message) ? error.message : error
      Rails.logger.send(severity, message)
    end
  end
end

[skukx]

@mtomov Great Video! I like your implementation as well. I've started a PR #2816 on this. I made some small changes to your implementation. Since this would be a new feature, I find it best to start simple and as needs grow expanding on it later.

I also left out

message = error.respond_to?(:message) ? error.message : error

And please correct me if I'm wrong but I believe the logger automatically extracts the message if the object passed in is an error object.

[skukx]

Here is a list of current Spree errors being raise for reference:

List of current spree custom errors

Spree::Core::GatewayError
Spree::Exchange::UnableToCreateShipments
Spree::LineItem::CurrencyMismatch
Spree::OrderCapturingFailures
Spree::OrderMutex::LockFailed
Spree::Order::CannotRebuildShipments
Spree::Order::InsufficientStock
Spree::PromotionCodeBatch::CantProcessStartedBatch
Spree::Reimbursement::IncompleteReimbursementError
Spree::StockLocation::InvalidMovementError
Spree::Stock::ShipmentRequired
Spree::Stock::OrderRequired
Spree::Wallet::Unauthorized

List of generic raise errors

These are errors that don't raise any specific exception. For example:

raise 'Some error'

These errors may be fine as is or we may want to create a custom error for
some so they are not classified RuntimeError directly. For example:

raise "Cannot set 'default' on a credit card without a user"
# Versus
raise Spree::CreditCardError, "Cannot set 'default' on a credit card without a user"

raise "Cannot set 'default' on a credit card without a user"
raise "The order association has been removed from InventoryUnit. The order is now determined from the shipment."
raise 'perform should be implemented in a sub-class of PromotionAction'
raise "Attempted to call code on a Spree::Promotion. Promotions are now tied to multiple code records"
raise "Attempted to call code= on a Spree::Promotion. Promotions are now tied to multiple code records"
raise "unexpected match policy: #{preferred_match_policy.inspect}"
raise "Implement me"
raise "Adjustment is already created"
raise "Adjustable does not match line item"
raise 'No master variant found to infer price'
raise "Unexpected originator type #{originator.class}"
raise "Your database needs at least one completed order to render this preview"