From de7b4eee004dad881525acb5694924a69eef9e9c Mon Sep 17 00:00:00 2001 From: fallwith Date: Wed, 6 Nov 2024 23:53:01 -0800 Subject: [PATCH 1/9] db explain plan refactor - DRY up the similar explain plan logic that exists in both the AR and AR subscriber instrumentation - support AR v7.2+ by making use of `with_connection` when available --- lib/new_relic/agent/database.rb | 36 +++++++++++++++++++ .../agent/instrumentation/active_record.rb | 9 +---- .../active_record_subscriber.rb | 13 +------ .../active_record_pg/active_record_test.rb | 26 ++++++-------- 4 files changed, 49 insertions(+), 35 deletions(-) diff --git a/lib/new_relic/agent/database.rb b/lib/new_relic/agent/database.rb index 63b25c8f08..e7d9db9f40 100644 --- a/lib/new_relic/agent/database.rb +++ b/lib/new_relic/agent/database.rb @@ -90,6 +90,42 @@ def get_connection(config, &connector) ConnectionManager.instance.get_connection(config, &connector) end + def explain_this(statement, use_execute = false) + if supports_with_connection? + explain_this_using_with_connection(statement) + else + explain_this_using_adapter_connection(statement, use_execute) + end + rescue => e + NewRelic::Agent.logger.error("Couldn't fetch the explain plan for statement: #{e}") + end + + def explain_this_using_with_connection(statement) + ::ActiveRecord::Base.with_connection do |conn| + conn.exec_query("EXPLAIN #{statement.sql}", "Explain #{statement.name}", statement.binds) + end + end + + def explain_this_using_adapter_connection(statement, use_execute) + connection = get_connection(statement.config) do + ::ActiveRecord::Base.send(:"#{statement.config[:adapter]}_connection", statement.config) + end + + if use_execute + connection.execute("EXPLAIN #{statement.sql}") + else + connection.exec_query("EXPLAIN #{statement.sql}", "Explain #{statement.name}", statement.binds) + end + end + + # ActiveRecord v7.2.0 introduced with_connection + def supports_with_connection? + return @supports_with_connection if defined?(@supports_with_connection) + + @supports_with_connection = defined?(::ActiveRecord::VERSION::STRING) && + Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new('7.2.0') + end + def close_connections ConnectionManager.instance.close_connections end diff --git a/lib/new_relic/agent/instrumentation/active_record.rb b/lib/new_relic/agent/instrumentation/active_record.rb index 9948ee314f..14961c63b6 100644 --- a/lib/new_relic/agent/instrumentation/active_record.rb +++ b/lib/new_relic/agent/instrumentation/active_record.rb @@ -9,14 +9,7 @@ module Agent module Instrumentation module ActiveRecord EXPLAINER = lambda do |statement| - connection = NewRelic::Agent::Database.get_connection(statement.config) do - ::ActiveRecord::Base.send("#{statement.config[:adapter]}_connection", - statement.config) - end - # the following line needs else branch coverage - if connection && connection.respond_to?(:execute) # rubocop:disable Style/SafeNavigation - return connection.execute("EXPLAIN #{statement.sql}") - end + NewRelic::Agent::Database.explain_this(statement, true) end def self.insert_instrumentation diff --git a/lib/new_relic/agent/instrumentation/active_record_subscriber.rb b/lib/new_relic/agent/instrumentation/active_record_subscriber.rb index 92c11e3e9f..d455770c7d 100644 --- a/lib/new_relic/agent/instrumentation/active_record_subscriber.rb +++ b/lib/new_relic/agent/instrumentation/active_record_subscriber.rb @@ -70,18 +70,7 @@ def finish(name, id, payload) # THREAD_LOCAL_ACCESS end def get_explain_plan(statement) - connection = NewRelic::Agent::Database.get_connection(statement.config) do - ::ActiveRecord::Base.send("#{statement.config[:adapter]}_connection", - statement.config) - end - # the following line needs else branch coverage - if connection && connection.respond_to?(:exec_query) # rubocop:disable Style/SafeNavigation - return connection.exec_query("EXPLAIN #{statement.sql}", - "Explain #{statement.name}", - statement.binds) - end - rescue => e - NewRelic::Agent.logger.debug("Couldn't fetch the explain plan for #{statement} due to #{e}") + NewRelic::Agent::Database.explain_this(statement) end def active_record_config(payload) diff --git a/test/multiverse/suites/active_record_pg/active_record_test.rb b/test/multiverse/suites/active_record_pg/active_record_test.rb index 470a267c06..4e838972d9 100644 --- a/test/multiverse/suites/active_record_pg/active_record_test.rb +++ b/test/multiverse/suites/active_record_pg/active_record_test.rb @@ -375,14 +375,15 @@ def test_metrics_for_direct_sql_other end def test_metrics_for_direct_sql_show - if supports_show_tables? - in_web_transaction do - conn = Order.connection + skip "Adapter does not support 'show tables'" unless supports_show_tables? + + in_web_transaction do + Order.with_connection do |conn| conn.execute('show tables') end - - assert_generic_rollup_metrics('show') end + + assert_generic_rollup_metrics('show') end def test_still_records_metrics_in_error_cases @@ -454,11 +455,10 @@ def test_gathers_explain_plans sample.prepare_to_send! explanations = sql_node.params[:explain_plan] - if supports_explain_plans? - refute_nil explanations, "No explains in node: #{sql_node}" - assert_equal(2, explanations.size, - "No explains in node: #{sql_node}") - end + + refute_nil explanations, "No explains in node: #{sql_node}" + assert_equal(2, explanations.size, + "No explains in node: #{sql_node}") end end @@ -593,11 +593,7 @@ def adapter end def supports_show_tables? - [:mysql, :postgres].include?(adapter) - end - - def supports_explain_plans? - [:mysql, :postgres].include?(adapter) + [:mysql, :mysql2, :trilogy].include?(adapter) end def current_product From a41780aa55f6f494a6071b3e22ce1459b9ad5bf1 Mon Sep 17 00:00:00 2001 From: fallwith Date: Sat, 9 Nov 2024 15:05:01 -0800 Subject: [PATCH 2/9] aws-sdk-lambda: require_relative have the chain and prepend module files relatively include the main instrumentation file --- lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb | 2 ++ lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb index c71a7b8e5e..221ff74542 100644 --- a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb +++ b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb @@ -2,6 +2,8 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true +require_relative 'instrumentation' + module NewRelic::Agent::Instrumentation module AwsSdkLambda::Chain def self.instrument! diff --git a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb index 20910221b6..db4de5b4ad 100644 --- a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb +++ b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb @@ -2,6 +2,8 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true +require_relative 'instrumentation' + module NewRelic::Agent::Instrumentation module AwsSdkLambda::Prepend include NewRelic::Agent::Instrumentation::AwsSdkLambda From 67b1f6c8f981cab34b07b80e0ea25bc022696aa6 Mon Sep 17 00:00:00 2001 From: fallwith Date: Tue, 12 Nov 2024 00:03:33 -0800 Subject: [PATCH 3/9] unit test updates for explain logic refactor test refactors to match explain plan DRY refactor --- test/new_relic/agent/sql_sampler_test.rb | 14 ++++++++------ .../new_relic/agent/transaction/trace_node_test.rb | 7 +++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/test/new_relic/agent/sql_sampler_test.rb b/test/new_relic/agent/sql_sampler_test.rb index 0d97949234..6596b91696 100644 --- a/test/new_relic/agent/sql_sampler_test.rb +++ b/test/new_relic/agent/sql_sampler_test.rb @@ -167,15 +167,17 @@ def test_harvest_should_aggregate_similar_queries end def test_harvest_should_collect_explain_plans - @connection.expects(:execute).with('EXPLAIN select * from test') \ - .returns(dummy_mysql_explain_result({'header0' => 'foo0', 'header1' => 'foo1', 'header2' => 'foo2'})) - @connection.expects(:execute).with('EXPLAIN select * from test2') \ - .returns(dummy_mysql_explain_result({'header0' => 'bar0', 'header1' => 'bar1', 'header2' => 'bar2'})) - data = NewRelic::Agent::TransactionSqlData.new data.set_transaction_info('/c/a', 'guid') data.set_transaction_name('WebTransaction/Controller/c/a') - explainer = NewRelic::Agent::Instrumentation::ActiveRecord::EXPLAINER + explainer = proc do |statement| + hash = if statement.sql.match?('test2') + {'header0' => 'bar0', 'header1' => 'bar1', 'header2' => 'bar2'} + else + {'header0' => 'foo0', 'header1' => 'foo1', 'header2' => 'foo2'} + end + dummy_mysql_explain_result(hash) + end config = {:adapter => 'mysql'} queries = [ NewRelic::Agent::SlowSql.new(NewRelic::Agent::Database::Statement.new('select * from test', config, explainer), diff --git a/test/new_relic/agent/transaction/trace_node_test.rb b/test/new_relic/agent/transaction/trace_node_test.rb index 3fa8c8b2ee..d3dfa60b04 100644 --- a/test/new_relic/agent/transaction/trace_node_test.rb +++ b/test/new_relic/agent/transaction/trace_node_test.rb @@ -37,7 +37,7 @@ def test_to_s s.to_s end - def test_to_array + def ttest_explain_sql_raising_an_errorest_to_array parent = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/parent', 1) parent.params[:test] = 'value' child = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/child', 2) @@ -290,12 +290,11 @@ def test_each_node_with_nest_tracking def test_explain_sql_raising_an_error s = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/metric', Process.clock_gettime(Process::CLOCK_REALTIME)) - config = {:adapter => 'mysql'} statement = NewRelic::Agent::Database::Statement.new('SELECT') - statement.config = config + statement.config = {:adapter => 'mysql'} statement.explainer = NewRelic::Agent::Instrumentation::ActiveRecord::EXPLAINER s.params = {:sql => statement} - NewRelic::Agent::Database.expects(:get_connection).with(config).raises(RuntimeError.new('whee')) + NewRelic::Agent::Database.expects(:explain_this).raises(RuntimeError.new('whee')) s.explain_sql end From 8633e5b03858fdc7ad88eaac7a121e88c2b20c6c Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Tue, 12 Nov 2024 16:54:07 -0800 Subject: [PATCH 4/9] Fix typos, out of context code --- lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb | 2 -- lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb | 2 -- test/new_relic/agent/transaction/trace_node_test.rb | 2 +- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb index 221ff74542..c71a7b8e5e 100644 --- a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb +++ b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/chain.rb @@ -2,8 +2,6 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true -require_relative 'instrumentation' - module NewRelic::Agent::Instrumentation module AwsSdkLambda::Chain def self.instrument! diff --git a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb index db4de5b4ad..20910221b6 100644 --- a/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb +++ b/lib/new_relic/agent/instrumentation/aws_sdk_lambda/prepend.rb @@ -2,8 +2,6 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true -require_relative 'instrumentation' - module NewRelic::Agent::Instrumentation module AwsSdkLambda::Prepend include NewRelic::Agent::Instrumentation::AwsSdkLambda diff --git a/test/new_relic/agent/transaction/trace_node_test.rb b/test/new_relic/agent/transaction/trace_node_test.rb index d3dfa60b04..708fab956a 100644 --- a/test/new_relic/agent/transaction/trace_node_test.rb +++ b/test/new_relic/agent/transaction/trace_node_test.rb @@ -37,7 +37,7 @@ def test_to_s s.to_s end - def ttest_explain_sql_raising_an_errorest_to_array + def test_explain_sql_raising_an_errorest_to_array parent = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/parent', 1) parent.params[:test] = 'value' child = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/child', 2) From 365f79822d2456321561a5f7cfbccfae0d1ec3af Mon Sep 17 00:00:00 2001 From: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com> Date: Thu, 14 Nov 2024 11:09:38 -0800 Subject: [PATCH 5/9] Update test/multiverse/suites/active_record_pg/active_record_test.rb --- test/multiverse/suites/active_record_pg/active_record_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/active_record_pg/active_record_test.rb b/test/multiverse/suites/active_record_pg/active_record_test.rb index 4e838972d9..f54e93d5fa 100644 --- a/test/multiverse/suites/active_record_pg/active_record_test.rb +++ b/test/multiverse/suites/active_record_pg/active_record_test.rb @@ -593,7 +593,7 @@ def adapter end def supports_show_tables? - [:mysql, :mysql2, :trilogy].include?(adapter) + [:mysql, :mysql2, :trilogy, :postgresql].include?(adapter) end def current_product From 8af26a8c9d48a8440e070d2a3549a3472d4536b3 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 14 Nov 2024 11:44:54 -0800 Subject: [PATCH 6/9] Undo postgresql addition to supports_show_tables? --- test/multiverse/suites/active_record_pg/active_record_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/active_record_pg/active_record_test.rb b/test/multiverse/suites/active_record_pg/active_record_test.rb index f54e93d5fa..4e838972d9 100644 --- a/test/multiverse/suites/active_record_pg/active_record_test.rb +++ b/test/multiverse/suites/active_record_pg/active_record_test.rb @@ -593,7 +593,7 @@ def adapter end def supports_show_tables? - [:mysql, :mysql2, :trilogy, :postgresql].include?(adapter) + [:mysql, :mysql2, :trilogy].include?(adapter) end def current_product From 63d4d75570b95a1530b8017a24130d81fc51a564 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 14 Nov 2024 15:28:59 -0800 Subject: [PATCH 7/9] Update test name --- test/new_relic/agent/transaction/trace_node_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/new_relic/agent/transaction/trace_node_test.rb b/test/new_relic/agent/transaction/trace_node_test.rb index 708fab956a..f097e48c4d 100644 --- a/test/new_relic/agent/transaction/trace_node_test.rb +++ b/test/new_relic/agent/transaction/trace_node_test.rb @@ -37,7 +37,7 @@ def test_to_s s.to_s end - def test_explain_sql_raising_an_errorest_to_array + def test_explain_sql_raising_an_error parent = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/parent', 1) parent.params[:test] = 'value' child = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/child', 2) From ef8825d05907d2713ec2e463010cd6b425209f98 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 14 Nov 2024 15:44:56 -0800 Subject: [PATCH 8/9] Explain the explain bug --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b590254305..9719ae5504 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,16 @@ ## dev -Version introduces instrumentation for the aws-sdk-lambda gem. +Version introduces instrumentation for the aws-sdk-lambda gem and fixes a bug with explain plans on Rails 7.2+. - **Feature: Instrumentation for aws-sdk-lambda** If the aws-sdk-lambda gem is present and used to invoke remote AWS Lambda functions, timing and error details for the invocations will be reported to New Relic. [PR#2926](https://github.com/newrelic/newrelic-ruby-agent/pull/2926) +- **Bugfix: Record explain plan traces on Rails 7.2+** + + Rails 7.2 removed adapter-specific connection methods (ex. `ActiveRecord::Base.postgresql_connection`) and replaced them with `ActiveRecord::Base.with_connection`. Our explain plan feature relies on making a connection to the database to create an explain plan trace. Due to a bug in our tests, we missed this regression. Now, the agent uses the new method to fetch explain plans on Rails 7.2+. Thank you, [@gsar](https://github.com/gsar) and [@gstark](https://github.com/gstark) for bringing this to our attention! [Issue#2922](https://github.com/newrelic/newrelic-ruby-agent/issues/2922) [PR#2940](https://github.com/newrelic/newrelic-ruby-agent/pull/2940) + ## v9.15.0 Version 9.15.0 updates View Component instrumentation to use a default metric name when one is unavailable, adds a configuration option to associate the AWS account ID with the DynamoDB calls from the AWS SDK, resolves a bug in rdkafka instrumentation when using the karafka-rdkafka gem, resolves a bug in the ruby-kafka instrumentation, fixes a bug with Grape instrumentation, and addresses a bug preventing the agent from running in serverless mode in an AWS Lambda layer. From 8e5027acf7993b60781ba135ef454f07e23e6d82 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 14 Nov 2024 17:03:15 -0800 Subject: [PATCH 9/9] Update test name to remove duplicate --- test/new_relic/agent/transaction/trace_node_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/new_relic/agent/transaction/trace_node_test.rb b/test/new_relic/agent/transaction/trace_node_test.rb index f097e48c4d..e4df6cd85a 100644 --- a/test/new_relic/agent/transaction/trace_node_test.rb +++ b/test/new_relic/agent/transaction/trace_node_test.rb @@ -37,7 +37,7 @@ def test_to_s s.to_s end - def test_explain_sql_raising_an_error + def test_to_array parent = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/parent', 1) parent.params[:test] = 'value' child = NewRelic::Agent::Transaction::TraceNode.new('Custom/test/child', 2)