-
Notifications
You must be signed in to change notification settings - Fork 5
Description
Hi -
The following happens in a multi-process scenario, such as puma with multiple workers.
I learned today that while queries_total and query_duration are being tracked correctly, the connection_pool_xxx ones are not.
This is because the former are tracked via a notification subscription that ActiveRecord sends out.
see https://github.com/yabeda-rb/yabeda-activerecord/blob/master/lib/yabeda/active_record.rb#L53
The latter are only tracked when a request is made to the metric collection endpoint. Unfortunately in this situation that process is the master puma process and isn't actually doing anything so reports zeros for most of the stats in question -- worse, it doesn't have any insight into the sub process stats.
see https://github.com/yabeda-rb/yabeda-activerecord/blob/master/lib/yabeda/active_record.rb#L73
I have a fix for this which is to spin up a thread in each puma sub process that sends an notification, then sleeps for awhile, over and over again. Then tweak the code on line 73 above to subscribe to this notification instead of using collect do.
Right now I've got it so that it works by setting an ENV var. Which works, but maybe isn't the best, but at the moment there isn't any config stuff for this gem. I see in #6 that maybe we'll have configuration shortly? If we could merge that I could build on top of it...?
Let me know you're thoughts on how to proceed and I'll push up a PR.
The relevant part of the PR is this:
if ENV.key?("YABEDA_ACTIVERECORD_CONNECTION_POOL_EVENT_NOTIFICATION")
ActiveSupport::Notifications.subscribe ENV["YABEDA_ACTIVERECORD_CONNECTION_POOL_EVENT_NOTIFICATION"] do |*args|
set_connection_pool_metrics.call # this is defined as a block farther up
end
else
collect do
set_connection_pool_metrics.call
end
end
Setting up the notification instrumentation I felt was better left outside of this as depending on the specific implementation it will have to be done in different spots.