From 937d5c8c87f15000529ca0728141f3c7dd358c0e Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 20 Dec 2018 16:31:48 -0500 Subject: [PATCH 1/3] Fix performance issues with JRuby (@guizmaii) --- .../contrib/active_record/events/sql.rb | 2 +- lib/ddtrace/contrib/active_record/utils.rb | 37 +++++++++++++++---- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/lib/ddtrace/contrib/active_record/events/sql.rb b/lib/ddtrace/contrib/active_record/events/sql.rb index 7129f6f04f4..d6525a83c82 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]) + 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 = if settings.service_name != Datadog::Utils::Database::VENDOR_DEFAULT diff --git a/lib/ddtrace/contrib/active_record/utils.rb b/lib/ddtrace/contrib/active_record/utils.rb index 64a6a07e166..30fe90b3eb8 100644 --- a/lib/ddtrace/contrib/active_record/utils.rb +++ b/lib/ddtrace/contrib/active_record/utils.rb @@ -21,14 +21,37 @@ def self.adapter_port connection_config[:port] end - def self.connection_config(connection = nil) - connection.nil? ? default_connection_config : connection_config_from_connection(connection) - end + # In newer Rails versions, the `payload` contains both the `connection` and its `object_id` named `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 + # + def self.connection_config(connection = nil, connection_id = nil) + return default_connection_config if connection.nil? && connection_id.nil? + + conn = if !connection.nil? + connection + # Rails 3.0 - 3.2 + elsif Gem.loaded_specs['activerecord'].version < Gem::Version.new('4.0') + ::ActiveRecord::Base + .connection_handler + .connection_pools + .values + .flat_map(&:connections) + .find { |c| c.object_id == connection_id } + # Rails 4.2+ + else + ::ActiveRecord::Base + .connection_handler + .connection_pool_list + .flat_map(&:connections) + .find { |c| c.object_id == connection_id } + end - # Typical of ActiveSupport::Notifications `sql.active_record` - def self.connection_config_from_connection(connection) - if connection.instance_variable_defined?(:@config) - connection.instance_variable_get(:@config) + if conn.instance_variable_defined?(:@config) + conn.instance_variable_get(:@config) else EMPTY_CONFIG end From e55763c8b77522e70659170ba031c90592c059bf Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 20 Dec 2018 16:32:10 -0500 Subject: [PATCH 2/3] Fixed: ActiveRecord test generating unwanted span. --- spec/ddtrace/contrib/sinatra/activerecord_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/ddtrace/contrib/sinatra/activerecord_spec.rb b/spec/ddtrace/contrib/sinatra/activerecord_spec.rb index 0ad21d92ad0..5997ceaba49 100644 --- a/spec/ddtrace/contrib/sinatra/activerecord_spec.rb +++ b/spec/ddtrace/contrib/sinatra/activerecord_spec.rb @@ -70,7 +70,9 @@ def migrate_db ActiveRecord::Base.establish_connection( adapter: 'sqlite3', database: ':memory:' - ) + ).tap do |conn| + conn.connection.execute('SELECT 42') + end end let(:sinatra_span) { spans.first } From 601fd80c2d180d7a51771b8db694b91f504b622d Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 20 Dec 2018 16:32:18 -0500 Subject: [PATCH 3/3] Removed: ActiveRecord AbstractAdapter patch. --- lib/ddtrace/contrib/active_record/patcher.rb | 2 - .../active_record/patches/abstract_adapter.rb | 72 ------------------- 2 files changed, 74 deletions(-) delete mode 100644 lib/ddtrace/contrib/active_record/patches/abstract_adapter.rb diff --git a/lib/ddtrace/contrib/active_record/patcher.rb b/lib/ddtrace/contrib/active_record/patcher.rb index 74d5f132e59..ff172372fbc 100644 --- a/lib/ddtrace/contrib/active_record/patcher.rb +++ b/lib/ddtrace/contrib/active_record/patcher.rb @@ -1,5 +1,4 @@ require 'ddtrace/contrib/patcher' -require 'ddtrace/contrib/active_record/patches/abstract_adapter' require 'ddtrace/contrib/active_record/events' module Datadog @@ -18,7 +17,6 @@ def patched? def patch do_once(:active_record) do begin - ::ActiveRecord::ConnectionAdapters::AbstractAdapter.send(:include, Patches::AbstractAdapter) Events.subscribe! rescue StandardError => e Datadog::Tracer.log.error("Unable to apply Active Record integration: #{e}") diff --git a/lib/ddtrace/contrib/active_record/patches/abstract_adapter.rb b/lib/ddtrace/contrib/active_record/patches/abstract_adapter.rb deleted file mode 100644 index 3ddefd1cac3..00000000000 --- a/lib/ddtrace/contrib/active_record/patches/abstract_adapter.rb +++ /dev/null @@ -1,72 +0,0 @@ -require 'set' -require 'ddtrace/augmentation/shim' - -module Datadog - module Contrib - module ActiveRecord - # Defines basic behaviors for an ActiveRecord event. - module Patches - # Adds patch to AbstractAdapter to make it pass more information through - # ActiveSupport notifications, for better instrumentation. - module AbstractAdapter - def self.included(base) - if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.0.0') - base.class_eval do - alias_method :log_without_datadog, :log - remove_method :log - include InstanceMethods - end - else - base.send(:prepend, InstanceMethods) - end - end - - # Compatibility shim for Rubies not supporting `.prepend` - module InstanceMethodsCompatibility - def log(*args, &block) - log_without_datadog(*args, &block) - end - end - - # InstanceMethods - implementing instrumentation - module InstanceMethods - include InstanceMethodsCompatibility unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0.0') - - EVENT_ACTIVERECORD_SQL = 'sql.active_record'.freeze - - # Override #log since sometimes connections are initialized prior - # to when the patch is applied; this will allow existing connections - # to receive the Shim as well. - def log(*args, &block) - insert_shim! unless shim_inserted? - super - end - - private - - def shim_inserted? - instance_variable_defined?(:@instrumenter) \ - && Datadog::Shim.shim?(@instrumenter) - end - - def insert_shim! - @instrumenter = Datadog::Shim.new(@instrumenter) do |shim| - connection = self - - shim.override_method!(:instrument) do |*args, &block| - # Inject connection into arguments - if args[0] == EVENT_ACTIVERECORD_SQL && args[1].is_a?(Hash) - args[1][:connection] ||= connection - end - - # Call original - shim_target.instrument(*args, &block) - end - end - end - end - end - end - end - end -end