Improve ActiveRecord connection resolution performance

[callumj]

Looping through connection handlers and connections can be a performance hit due to duping involved on the underlying thread safe call from connection_pool_list to ThreadSafe::Cache#values. Underneath #values dupes the connection list per each call which can add up.

This backports in an improvement that was introduced in Rails 6 that passes through the connection in the notification payload.

StackProf:

NewRelic::Agent::Instrumentation::ActiveRecordSubscriber#active_record_config (/usr/local/bundle/gems/newrelic_rpm-4.6.0.338/lib/new_relic/agent/instrumentation/active_record_subscriber.rb:78)
  samples:     9 self (0.1%)  /    167 total (1.4%)
  callers:
     167  (  100.0%)  NewRelic::Agent::Instrumentation::ActiveRecordSubscriber#start
       7  (    4.2%)  NewRelic::Agent::Instrumentation::ActiveRecordSubscriber#active_record_config
  callees (158 total):
     147  (   93.0%)  ActiveRecord::ConnectionAdapters::ConnectionHandler#connection_pool_list
      10  (    6.3%)  ActiveRecord::Base.connection_handler
       7  (    4.4%)  NewRelic::Agent::Instrumentation::ActiveRecordSubscriber#active_record_config
       1  (    0.6%)  AutoReplica::ConnectionHandler#method_missing
  code:
                                  |    78  |         def active_record_config(payload)
                                  |    79  |           return unless payload[:connection_id]
                                  |    80  |
                                  |    81  |           connection = nil
                                  |    82  |           connection_id = payload[:connection_id]
                                  |    83  |
  162    (1.3%)                   |    84  |           ::ActiveRecord::Base.connection_handler.connection_pool_list.each do |handler|
    3    (0.0%)                   |    85  |             connection = handler.connections.detect do |conn|
    3    (0.0%) /     3   (0.0%)  |    86  |               conn.object_id == connection_id
                                  |    87  |             end
                                  |    88  |
    1    (0.0%) /     1   (0.0%)  |    89  |             break if connection
                                  |    90  |           end
                                  |    91  |
    5    (0.0%) /     5   (0.0%)  |    92  |           connection.instance_variable_get(:@config) if connection
                                  |    93  |         end

Happy to take pointers on where best to add tests.

[mwear]

@callumj: Thanks for this work. We have internal issue around improving this same code path, but in a different way. On MRI ObjectSpace._id2ref can be used to obtain the connection given its object id. This would be less invasive than monkey patching older versions of Rails. One thing that has kept us from pursuing this option, is that this approach will not work on JRuby. We could, however, provide a different implementation on JRuby.

I haven't tested this code, but this is what I think it'd look like:

active_record_subscriber.rb

...
def active_record_config(payload)
  connection = nil
  
  if payload[:connection]
    connection = payload[:connection]
  elsif payload[:connection_id]
    connection = active_record_connection_from_id payload[:connection_id]
  end

  connection.instance_variable_get(:@config) if connection
end


if RUBY_ENGINE == 'jruby'
  def active_record_connection_from_id connection_id
    ::ActiveRecord::Base.connection_handler.connection_pool_list.each do |handler|
      connection = handler.connections.detect do |conn|
        conn.object_id == connection_id
      end

      return connection if connection
    end
  end
else
  require 'objspace'
  
  def active_record_connection_from_id connection_id
    ObjectSpace._id2ref connection_id
  end
end
...

We do something similar in TransactionTimeAggregator.

What do you think about this approach?

[callumj]

@mwear That sounds like a great approach since rpm already uses id2ref. I avoided it originally because of the JRuby incompatibility and some hints from the community that it may not be preferred: https://bugs.ruby-lang.org/issues/15408. But I agree it is dangerous to patch over some deep Rails infra so it’s just a choice between the lesser evils. I’d go with id2ref and eventually it should be phased out as people upgrade to Rails 6.

Originally this was inspired by the Datadog work around this: DataDog/dd-trace-rb#649

[mwear]

@callumj Thanks for bringing that Ruby issue to my attention. While I was aware the use of ObjectSpace on JRuby was not recommended, I was not aware that it was being considered for deprecation from MRI. Given that information, I don't think it makes sense to use _id_2_ref if there's an alternative.

We can probably accept the backport approach so long as there is a way to opt-out if a monkey patch causes issues for a user. Would you consider adding an option called backport_fast_active_record_connection_lookup to default source with a default value of true and only install the patch if it's enabled? You can leave the original implementation in place as a fallback if it's not.

[callumj]

Good call @mwear, I should be able to get this to you by end of week April 8.

[callumj]

@mwear Added setting control

[mwear]

This looks great @callumj. Thanks for your work and for improving the Ruby Agent! We're going to pull this into our development branch and it should become part of our next release. We'll let you know when it goes out!

[callumj]

Thank you @mwear for your review and guidance on getting this merged in.

[justinfoote]

This has been included in version 6.3.0 of the New Relic ruby agent!