Include the database configuration in activerecord notifications.

[thejonanshow]

In some cases it is useful to have the database configuration in an activerecord
notification to initiate an independent connection to the database.

[rafaelfranca]

Thank you for the contribution.

You already have the connection_id and can get the connection configuration using it on ActiveRecord::Base.connection_handler.connection_pools. We are planning to add an better API to get it. See #11301.

I don't think we should always add the database configuration on the notification.

[thejonanshow]

Hello Rafael!

I think a lot of the value of an evented notification system is that the events include the information necessary to take action without the need to call back into the system.

It seems that others would find it useful to include the full connection details in the notification as well ('We send query timings to statsd by subscribing to AR notifications...'), is there a downside here that I'm not seeing?

[rafaelfranca]

Fine. I'll leave the decision to who could have use for it.

cc @jeremy

[mjc]

It looks like the NewRelic agent uses this info as well... It currently needs to do ObjectSpace._id2ref to get it, which degrades performance on rubinius rather terribly.

https://github.com/newrelic/rpm/blob/master/lib/new_relic/agent/instrumentation/active_record_subscriber.rb#L91

[rafaelfranca]

I talked with @jeremy and he agrees we don't need to include the specs in all the notification. Since we already provide the connection id we only need to expose an API to get the specs for a given connection id.

[thejonanshow]

@mjc I'm actually on the ruby agent team at New Relic and that's exactly the reason we made this pull request. For the time being we will try and use the connection_handler but it seems like the sort of data that should be included in the notifications.

[mjc]

@1337807 another interim option might be to make a weakhash? weakrefs are fine in rbx...

[thejonanshow]

How do you mean?

[mjc]

@1337807 ah nevermind, it probably wouldn't be better than using connection_handler. I was thinking you could make a weakhash of id -> ref and use that instead of _id2ref, similar to the way headius/weakling does for jruby.

[rafaelfranca]

@1337807 will an API to get the specs from a given connection id work to you? If not we can think about merging this one

[thejonanshow]

@rafaelfranca It could work if the API were available soon and reasonably fast, but it still seems a better design to include the information needed to act on an event in the notification itself. I prefer this solution.

[thejonanshow]

@rafaelfranca Any more thoughts on merging this? We'd love to be able to improve our Rails support with our next version.

[rafaelfranca]

I still prefer to not always include the configuration information in the event.

This new API will be only available on 4.1 and even this pull request if applied will only be available on 4.1. 4-0-stable is a bug fix branch on now.

I'll discuss with @jeremy about this one again.

[jaggederest]

👍 avoiding _id2ref speeds things up quite a bit

@rafaelfranca has any work been done on this API you discuss? I'd be happy to work on it, but it'd need to be pretty fast - hash of ids to configs could work though.

[rafaelfranca]

@jaggederest not yet.

[thejonanshow]

@rafaelfranca So you think it likely this will be merged by the release of 4.1? It will certainly make our Rubinius customers happy.

[rafaelfranca]

I don't think the current implementation will be merged, but we plan to include the API to get the configuration giving a connection ID on 4.1

[tenderlove]

1. Using object_id for the connection_id is wrong. object_id can be reused, so this value could be a lie.

2. You don't need to use _id2ref to get the object, you should be able to get a list of database connections and compare the object ids. I know this will be a linear search, but I'll bet any particular app doesn't have that many database connections.

At first I thought the key based solution would work. However, there is an annoying issue. What if the connection is dropped and deleted before the event listener has a chance to act? Are we going to guarantee that notifications are always blocking and "in-band"? It would mean that we don't support notification listeners that run under a different thread than the main app.

/cc @carllerche

[tenderlove]

Just a thought, we support urls strings for making connections. It seems OK to just pass the connection URL in the notification. That should give you enough information to connect to the same database, and we only have to add one key and one string value.

[sgrif]

@thejonanshow Are you still interested in working on this? I agree that the connection URL should be enough information.

[thejonanshow]

@sgrif Sure, I'll take a crack at it. May not be until after the holidays but I'll do what I can.

[kamipo]

Closing this for now, since now we have a connection itself in notifications #34602.