ActiveRecord::RecordNotUnique should be handled similarly to PG::UniqueViolation

[patbenatar]

Rails wraps PG::UniqueViolation in a ActiveRecord::RecordNotUnique. Should this code https://github.com/appsignal/appsignal-ruby/blob/main/lib/appsignal/transaction.rb#L538 handle both?

[tombruijn]

Hi @patbenatar, we can definitely take a look. Do you have an example error message for ActiveRecord::RecordNotUnique. A very small example app would be even better :)

[patbenatar]

PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "example_constraint"
DETAIL: Key (email)=([email protected]) already exists.

We've monkey-patched like so in our app:

module Appsignal
  class Transaction
    def cleaned_error_message(error)
      case error.class.to_s
      when "PG::UniqueViolation", "ActiveRecord::RecordNotUnique"
        error.message.to_s.gsub(/\)=\(.*\)/, ")=(?)")
      else
        error.message.to_s
      end
    end
  end
end

Perhaps rather than adding a list of error classes here, we could make this method extensible via configuration? We use https://github.com/ankane/logstop elsewhere in our application and it'd be great to be able to configure Appsignal to use that too, something like:

config.clean_error_message = ->(msg) { ... }

[tombruijn]

Looks like you sent a PG::UniqueViolation example, the error message is the same for ActiveRecord::RecordNotUnique? Or is one wrapped in the other?

we could make this method extensible via configuration?

We considered that in the past, but decided not to add it to this first version. We want to take a good look at how best to fix this for all integrations, not just Ruby.

[patbenatar]

It's wrapped. That's the error message for the AR exception.

Sounds good re: configuration. Simply adding the AR exception class to the list as I've done in my monkey patch would be great for now.

Thanks!

[tombruijn]

I've submitted the fix in PR #834.

[tombruijn]

@patbenatar The fix for this is released in Ruby gem 3.0.25.

[patbenatar]

@tombruijn thank you!