From 59716dbd5bec95ed69de1000fd3699de42dab03b Mon Sep 17 00:00:00 2001 From: jules Ivanic Date: Mon, 3 Dec 2018 10:39:08 +0100 Subject: [PATCH 1/3] Fix performance issues with JRuby --- .../contrib/active_record/events/sql.rb | 2 +- lib/ddtrace/contrib/active_record/utils.rb | 43 +++++++++---------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/lib/ddtrace/contrib/active_record/events/sql.rb b/lib/ddtrace/contrib/active_record/events/sql.rb index e4f8da76ca5..e92a0e77b5f 100644 --- a/lib/ddtrace/contrib/active_record/events/sql.rb +++ b/lib/ddtrace/contrib/active_record/events/sql.rb @@ -24,7 +24,7 @@ def span_name end def process(span, event, _id, payload) - config = Utils.connection_config(payload[:connection_id]) + config = Utils.connection_config(payload[:connection], payload[:connection_id]) settings = Datadog.configuration[:active_record, config] adapter_name = Datadog::Utils::Database.normalize_vendor(config[:adapter]) service_name = !settings.nil? ? settings.service_name : configuration[:service_name] diff --git a/lib/ddtrace/contrib/active_record/utils.rb b/lib/ddtrace/contrib/active_record/utils.rb index 3eb96ceff70..95f03fc6fc5 100644 --- a/lib/ddtrace/contrib/active_record/utils.rb +++ b/lib/ddtrace/contrib/active_record/utils.rb @@ -3,6 +3,8 @@ module Contrib module ActiveRecord # Common utilities for Rails module Utils + EMPTY_CONFIG = {}.freeze + def self.adapter_name Datadog::Utils::Database.normalize_vendor(connection_config[:adapter]) end @@ -19,28 +21,23 @@ def self.adapter_port connection_config[:port] end - def self.connection_config(object_id = nil) - object_id.nil? ? default_connection_config : connection_config_by_id(object_id) - end - - # Attempt to retrieve the connection from an object ID. - def self.connection_by_id(object_id) - return nil if object_id.nil? - ObjectSpace._id2ref(object_id) - rescue StandardError - nil - end - - # Attempt to retrieve the connection config from an object ID. - # Typical of ActiveSupport::Notifications `sql.active_record` - def self.connection_config_by_id(object_id) - connection = connection_by_id(object_id) - return {} if connection.nil? - - if connection.instance_variable_defined?(:@config) - connection.instance_variable_get(:@config) + # In newer versions of Rails, the `payload` contains both the `connection` and its `object_id` named `connection_id`. + # + # So, here, if rails is recent we'll have a direct access to the connection. Else, we'll find it thanks to the passed `connection_id`. + # + # See this PR for more details: https://github.com/rails/rails/pull/34602 + # + def self.connection_config(connection = nil, connection_id = nil) + if connection.nil? && connection_id.nil? + default_connection_config else - {} + conn = connection || ::ActiveRecord::Base + .connection_handler + .connection_pool_list + .flat_map { |p| p.connections } + .select { |c| c.object_id == connection_id } + + conn.respond_to?(:config) ? conn.config : EMPTY_CONFIG end end @@ -53,9 +50,9 @@ def self.default_connection_config end connection_pool = ::ActiveRecord::Base.connection_handler.retrieve_connection_pool(current_connection_name) - connection_pool.nil? ? {} : (@default_connection_config = connection_pool.spec.config) + connection_pool.nil? ? EMPTY_CONFIG : (@default_connection_config = connection_pool.spec.config) rescue StandardError - {} + EMPTY_CONFIG end end end From a661e7ca84c5d7c81ddc9f90874f95653bce805e Mon Sep 17 00:00:00 2001 From: jules Ivanic Date: Mon, 3 Dec 2018 10:50:47 +0100 Subject: [PATCH 2/3] Typo --- lib/ddtrace/contrib/active_record/utils.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ddtrace/contrib/active_record/utils.rb b/lib/ddtrace/contrib/active_record/utils.rb index 95f03fc6fc5..3b08c2866a6 100644 --- a/lib/ddtrace/contrib/active_record/utils.rb +++ b/lib/ddtrace/contrib/active_record/utils.rb @@ -35,7 +35,7 @@ def self.connection_config(connection = nil, connection_id = nil) .connection_handler .connection_pool_list .flat_map { |p| p.connections } - .select { |c| c.object_id == connection_id } + .find { |c| c.object_id == connection_id } conn.respond_to?(:config) ? conn.config : EMPTY_CONFIG end From e2613ee11565ebed99e60dc5899f735a75339e57 Mon Sep 17 00:00:00 2001 From: jules Ivanic Date: Mon, 3 Dec 2018 11:18:02 +0100 Subject: [PATCH 3/3] Rubocop --- lib/ddtrace/contrib/active_record/utils.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/ddtrace/contrib/active_record/utils.rb b/lib/ddtrace/contrib/active_record/utils.rb index 3b08c2866a6..60ed2f99578 100644 --- a/lib/ddtrace/contrib/active_record/utils.rb +++ b/lib/ddtrace/contrib/active_record/utils.rb @@ -21,9 +21,10 @@ def self.adapter_port connection_config[:port] end - # In newer versions of Rails, the `payload` contains both the `connection` and its `object_id` named `connection_id`. + # In newer Rails versions, the `payload` contains both the `connection` and its `object_id` named `connection_id`. # - # So, here, if rails is recent we'll have a direct access to the connection. Else, we'll find it thanks to the passed `connection_id`. + # So, if rails is recent we'll have a direct access to the connection. + # Else, we'll find it thanks to the passed `connection_id`. # # See this PR for more details: https://github.com/rails/rails/pull/34602 # @@ -31,11 +32,12 @@ def self.connection_config(connection = nil, connection_id = nil) if connection.nil? && connection_id.nil? default_connection_config else - conn = connection || ::ActiveRecord::Base - .connection_handler - .connection_pool_list - .flat_map { |p| p.connections } - .find { |c| c.object_id == connection_id } + conn = + connection || ::ActiveRecord::Base + .connection_handler + .connection_pool_list + .flat_map(&:connections) + .find { |c| c.object_id == connection_id } conn.respond_to?(:config) ? conn.config : EMPTY_CONFIG end