From 9ce63601892dc2b6a35a1fb9b0bf91280cf6497f Mon Sep 17 00:00:00 2001 From: hramadan Date: Thu, 1 Dec 2022 14:06:11 -0800 Subject: [PATCH 01/53] concurrent_ruby scaffold --- .../agent/configuration/default_source.rb | 8 +++++ .../agent/instrumentation/concurrent_ruby.rb | 29 +++++++++++++++++++ .../instrumentation/concurrent_ruby/chain.rb | 22 ++++++++++++++ .../concurrent_ruby/instrumentation.rb | 13 +++++++++ .../concurrent_ruby/prepend.rb | 13 +++++++++ newrelic.yml | 4 +++ .../multiverse/suites/concurrent_ruby/Envfile | 9 ++++++ .../concurrent_ruby_instrumentation_test.rb | 15 ++++++++++ .../concurrent_ruby/config/newrelic.yml | 19 ++++++++++++ 9 files changed, 132 insertions(+) create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb create mode 100644 test/multiverse/suites/concurrent_ruby/Envfile create mode 100644 test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb create mode 100644 test/multiverse/suites/concurrent_ruby/config/newrelic.yml diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 9039679fa8..76196dea83 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1556,6 +1556,14 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => 'Controls auto-instrumentation of bunny at start up. May be one of [auto|prepend|chain|disabled].' }, + :'instrumentation.concurrent_ruby' => { + :default => 'auto', + :public => true, + :type => String, + :dynamic_name => true, + :allowed_from_server => false, + :description => 'Controls auto-instrumentation of the concurrent_ruby library at start up. May be one of [auto|prepend|chain|disabled].' + }, :'instrumentation.curb' => { :default => instrumentation_value_of(:disable_curb), :documentation_default => 'auto', diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb new file mode 100644 index 0000000000..1e7f8eb1f9 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -0,0 +1,29 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +require_relative 'concurrent_ruby/instrumentation' +require_relative 'concurrent_ruby/chain' +require_relative 'concurrent_ruby/prepend' + +DependencyDetection.defer do + named :'concurrent_ruby' + + depends_on do + # The class that needs to be defined to prepend/chain onto. This can be used + # to determine whether the library is installed. + defined?(::Concurrent) + # Add any additional requirements to verify whether this instrumentation + # should be installed + end + + executes do + ::NewRelic::Agent.logger.info('Installing concurrent_ruby instrumentation') + + if use_prepend? + prepend_instrument ::Concurrent, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend + else + chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb new file mode 100644 index 0000000000..787f8285c9 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -0,0 +1,22 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby + def self.instrument! + ::Concurrent.class_eval do + include NewRelic::Agent::Instrumentation::ConcurrentRuby + + alias_method(:method_to_instrument_without_new_relic, :method_to_instrument) + alias_method(:method_to_instrument, :method_to_instrument_with_new_relic) + + def method_to_instrument(*args) + method_to_instrument_with_new_relic(*args) do + method_to_instrument_without_new_relic(*args) + end + end + end + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb new file mode 100644 index 0000000000..70073688ac --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -0,0 +1,13 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby + + def method_to_instrument_with_new_relic(*args) + # add instrumentation content here + yield + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb new file mode 100644 index 0000000000..e063a53f10 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -0,0 +1,13 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby::Prepend + include NewRelic::Agent::Instrumentation::ConcurrentRuby + + def method_to_instrument(*args) + method_to_instrument_with_new_relic(*args) { super } + end + end +end diff --git a/newrelic.yml b/newrelic.yml index f264d4e9fb..6277f80f48 100644 --- a/newrelic.yml +++ b/newrelic.yml @@ -314,6 +314,10 @@ common: &default_settings # May be one of [auto|prepend|chain|disabled]. # instrumentation.bunny: auto + # Controls auto-instrumentation of concurrent_ruby at start up. + # May be one of [auto|prepend|chain|disabled] + # instrumentation.concurrent_ruby: auto + # Controls auto-instrumentation of Curb at start up. # May be one of [auto|prepend|chain|disabled]. # instrumentation.curb: auto diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile new file mode 100644 index 0000000000..40d0658f94 --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -0,0 +1,9 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +instrumentation_methods :chain, :prepend + +gemfile <<-RB + gem 'concurrent-ruby' +RB diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb new file mode 100644 index 0000000000..74e555a985 --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -0,0 +1,15 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +class ConcurrentRubyInstrumentationTest < Minitest::Test + def setup + @stats_engine = NewRelic::Agent.instance.stats_engine + end + + def teardown + NewRelic::Agent.instance.stats_engine.clear_stats + end + + # Add tests here +end diff --git a/test/multiverse/suites/concurrent_ruby/config/newrelic.yml b/test/multiverse/suites/concurrent_ruby/config/newrelic.yml new file mode 100644 index 0000000000..270c4d408d --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/config/newrelic.yml @@ -0,0 +1,19 @@ +--- +development: + error_collector: + enabled: true + apdex_t: 0.5 + monitor_mode: true + license_key: bootstrap_newrelic_admin_license_key_000 + instrumentation: + concurrent_ruby: <%= $instrumentation_method %> + app_name: test + log_level: debug + host: 127.0.0.1 + api_host: 127.0.0.1 + transaction_trace: + record_sql: obfuscated + enabled: true + stack_trace_threshold: 0.5 + transaction_threshold: 1.0 + capture_params: false From cbd49b0dfa304f59fc6a2224a11b1cbf8fa5ce5e Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 7 Dec 2022 17:13:20 -0800 Subject: [PATCH 02/53] WIP Instrument Concurrent::Promises#future --- .../agent/instrumentation/concurrent_ruby.rb | 8 ++--- .../instrumentation/concurrent_ruby/chain.rb | 13 +++---- .../concurrent_ruby/instrumentation.rb | 18 ++++++++-- .../concurrent_ruby/prepend.rb | 4 +-- .../concurrent_ruby_instrumentation_test.rb | 36 ++++++++++++++++--- 5 files changed, 57 insertions(+), 22 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index 1e7f8eb1f9..a64fdda2c3 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -10,18 +10,14 @@ named :'concurrent_ruby' depends_on do - # The class that needs to be defined to prepend/chain onto. This can be used - # to determine whether the library is installed. defined?(::Concurrent) - # Add any additional requirements to verify whether this instrumentation - # should be installed end executes do - ::NewRelic::Agent.logger.info('Installing concurrent_ruby instrumentation') + ::NewRelic::Agent.logger.info('Installing concurrent-ruby instrumentation') if use_prepend? - prepend_instrument ::Concurrent, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend + prepend_instrument ::Concurrent::Promises::FactoryMethods, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend else chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 787f8285c9..aa65f6b3a0 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -5,15 +5,16 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby def self.instrument! - ::Concurrent.class_eval do + # this is a module, can I still use class_eval? + ::Concurrent::Promises::FactoryMethods.class_eval do include NewRelic::Agent::Instrumentation::ConcurrentRuby - alias_method(:method_to_instrument_without_new_relic, :method_to_instrument) - alias_method(:method_to_instrument, :method_to_instrument_with_new_relic) + alias_method(:future_without_new_relic, :future) + alias_method(:future, :future_with_new_relic) - def method_to_instrument(*args) - method_to_instrument_with_new_relic(*args) do - method_to_instrument_without_new_relic(*args) + def future(*args, &task) + future_with_new_relic(*args) do + future_without_new_relic(*args, &task) end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 70073688ac..2099c3c216 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -4,10 +4,22 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby + PROMISES_FUTURE_NAME = 'Concurrent::Promises#future' - def method_to_instrument_with_new_relic(*args) - # add instrumentation content here - yield + def future_with_new_relic(*args) + segment = NewRelic::Agent::Tracer.start_segment(name: segment_name(*args)) + begin + NewRelic::Agent::Tracer.capture_segment_error(segment) { yield } + ensure + ::NewRelic::Agent::Transaction::Segment.finish(segment) + end + end + + private + + def segment_name(*args) + keyword_args = args[0] + keyword_args && keyword_args.key?(:nr_name) ? keyword_args[:nr_name] : PROMISES_FUTURE_NAME end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index e063a53f10..0dba9a3ec6 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -6,8 +6,8 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby::Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby - def method_to_instrument(*args) - method_to_instrument_with_new_relic(*args) { super } + def future(*args, &task) + future_with_new_relic(*args) { super } end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 74e555a985..878ace50aa 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -3,13 +3,39 @@ # frozen_string_literal: true class ConcurrentRubyInstrumentationTest < Minitest::Test - def setup - @stats_engine = NewRelic::Agent.instance.stats_engine + def test_promises_future_creates_segment_with_default_name + txn = in_transaction do + Concurrent::Promises.future { 'hi' } + end + segment = txn.segments[1] + + assert_equal segment.name, NewRelic::Agent::Instrumentation::ConcurrentRuby::PROMISES_FUTURE_NAME end - def teardown - NewRelic::Agent.instance.stats_engine.clear_stats + def test_promises_future_creates_segment_with_custom_name + name = 'my little pony' + txn = in_transaction do + Concurrent::Promises.future(nr_name: name) { 'hi' } + end + segment = txn.segments[1] + + assert_equal segment.name, name end - # Add tests here + # hmm -- i think I'm not understanding how errors work in this context + # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 + def test_promises_future_captures_segment_error + skip + txn = nil + begin + txn = in_transaction('concurrent') do + Concurrent::Promises.future { raise 'boom!' } + end + rescue StandardError => e + # NOOP -- allowing span and transaction to notice error + end + + assert_segment_noticed_error txn, /concurrent$/, StandardError, /boom/i + assert_transaction_noticed_error txn, StandardError + end end From c40a516356bfb42a9e89f40a95efff2ceca2d168 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 7 Dec 2022 17:20:41 -0800 Subject: [PATCH 03/53] Rubocop --- .../concurrent_ruby/concurrent_ruby_instrumentation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 878ace50aa..ec0aa17dd6 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -23,7 +23,7 @@ def test_promises_future_creates_segment_with_custom_name end # hmm -- i think I'm not understanding how errors work in this context - # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 + # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 def test_promises_future_captures_segment_error skip txn = nil From 63e3a50522418224f5b796e7891fa58b8b33f233 Mon Sep 17 00:00:00 2001 From: hramadan Date: Thu, 1 Dec 2022 14:06:11 -0800 Subject: [PATCH 04/53] concurrent_ruby scaffold --- .../agent/configuration/default_source.rb | 8 +++++ .../agent/instrumentation/concurrent_ruby.rb | 29 +++++++++++++++++++ .../instrumentation/concurrent_ruby/chain.rb | 22 ++++++++++++++ .../concurrent_ruby/instrumentation.rb | 13 +++++++++ .../concurrent_ruby/prepend.rb | 13 +++++++++ newrelic.yml | 4 +++ .../multiverse/suites/concurrent_ruby/Envfile | 9 ++++++ .../concurrent_ruby_instrumentation_test.rb | 15 ++++++++++ .../concurrent_ruby/config/newrelic.yml | 19 ++++++++++++ 9 files changed, 132 insertions(+) create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb create mode 100644 lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb create mode 100644 test/multiverse/suites/concurrent_ruby/Envfile create mode 100644 test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb create mode 100644 test/multiverse/suites/concurrent_ruby/config/newrelic.yml diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 8819e9ce71..387d5522ef 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1569,6 +1569,14 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => 'Controls auto-instrumentation of bunny at start up. May be one of [auto|prepend|chain|disabled].' }, + :'instrumentation.concurrent_ruby' => { + :default => 'auto', + :public => true, + :type => String, + :dynamic_name => true, + :allowed_from_server => false, + :description => 'Controls auto-instrumentation of the concurrent_ruby library at start up. May be one of [auto|prepend|chain|disabled].' + }, :'instrumentation.curb' => { :default => instrumentation_value_of(:disable_curb), :documentation_default => 'auto', diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb new file mode 100644 index 0000000000..1e7f8eb1f9 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -0,0 +1,29 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +require_relative 'concurrent_ruby/instrumentation' +require_relative 'concurrent_ruby/chain' +require_relative 'concurrent_ruby/prepend' + +DependencyDetection.defer do + named :'concurrent_ruby' + + depends_on do + # The class that needs to be defined to prepend/chain onto. This can be used + # to determine whether the library is installed. + defined?(::Concurrent) + # Add any additional requirements to verify whether this instrumentation + # should be installed + end + + executes do + ::NewRelic::Agent.logger.info('Installing concurrent_ruby instrumentation') + + if use_prepend? + prepend_instrument ::Concurrent, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend + else + chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb new file mode 100644 index 0000000000..787f8285c9 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -0,0 +1,22 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby + def self.instrument! + ::Concurrent.class_eval do + include NewRelic::Agent::Instrumentation::ConcurrentRuby + + alias_method(:method_to_instrument_without_new_relic, :method_to_instrument) + alias_method(:method_to_instrument, :method_to_instrument_with_new_relic) + + def method_to_instrument(*args) + method_to_instrument_with_new_relic(*args) do + method_to_instrument_without_new_relic(*args) + end + end + end + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb new file mode 100644 index 0000000000..70073688ac --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -0,0 +1,13 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby + + def method_to_instrument_with_new_relic(*args) + # add instrumentation content here + yield + end + end +end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb new file mode 100644 index 0000000000..e063a53f10 --- /dev/null +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -0,0 +1,13 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +module NewRelic::Agent::Instrumentation + module ConcurrentRuby::Prepend + include NewRelic::Agent::Instrumentation::ConcurrentRuby + + def method_to_instrument(*args) + method_to_instrument_with_new_relic(*args) { super } + end + end +end diff --git a/newrelic.yml b/newrelic.yml index 8a41897536..1c516e112a 100644 --- a/newrelic.yml +++ b/newrelic.yml @@ -324,6 +324,10 @@ common: &default_settings # May be one of [auto|prepend|chain|disabled]. # instrumentation.bunny: auto + # Controls auto-instrumentation of concurrent_ruby at start up. + # May be one of [auto|prepend|chain|disabled] + # instrumentation.concurrent_ruby: auto + # Controls auto-instrumentation of Curb at start up. # May be one of [auto|prepend|chain|disabled]. # instrumentation.curb: auto diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile new file mode 100644 index 0000000000..40d0658f94 --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -0,0 +1,9 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +instrumentation_methods :chain, :prepend + +gemfile <<-RB + gem 'concurrent-ruby' +RB diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb new file mode 100644 index 0000000000..74e555a985 --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -0,0 +1,15 @@ +# This file is distributed under New Relic's license terms. +# See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. +# frozen_string_literal: true + +class ConcurrentRubyInstrumentationTest < Minitest::Test + def setup + @stats_engine = NewRelic::Agent.instance.stats_engine + end + + def teardown + NewRelic::Agent.instance.stats_engine.clear_stats + end + + # Add tests here +end diff --git a/test/multiverse/suites/concurrent_ruby/config/newrelic.yml b/test/multiverse/suites/concurrent_ruby/config/newrelic.yml new file mode 100644 index 0000000000..270c4d408d --- /dev/null +++ b/test/multiverse/suites/concurrent_ruby/config/newrelic.yml @@ -0,0 +1,19 @@ +--- +development: + error_collector: + enabled: true + apdex_t: 0.5 + monitor_mode: true + license_key: bootstrap_newrelic_admin_license_key_000 + instrumentation: + concurrent_ruby: <%= $instrumentation_method %> + app_name: test + log_level: debug + host: 127.0.0.1 + api_host: 127.0.0.1 + transaction_trace: + record_sql: obfuscated + enabled: true + stack_trace_threshold: 0.5 + transaction_threshold: 1.0 + capture_params: false From 293d423351cde439aa6273787da597b6bc237c7a Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 7 Dec 2022 17:13:20 -0800 Subject: [PATCH 05/53] WIP Instrument Concurrent::Promises#future --- .../agent/instrumentation/concurrent_ruby.rb | 8 ++--- .../instrumentation/concurrent_ruby/chain.rb | 13 +++---- .../concurrent_ruby/instrumentation.rb | 18 ++++++++-- .../concurrent_ruby/prepend.rb | 4 +-- .../concurrent_ruby_instrumentation_test.rb | 36 ++++++++++++++++--- 5 files changed, 57 insertions(+), 22 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index 1e7f8eb1f9..a64fdda2c3 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -10,18 +10,14 @@ named :'concurrent_ruby' depends_on do - # The class that needs to be defined to prepend/chain onto. This can be used - # to determine whether the library is installed. defined?(::Concurrent) - # Add any additional requirements to verify whether this instrumentation - # should be installed end executes do - ::NewRelic::Agent.logger.info('Installing concurrent_ruby instrumentation') + ::NewRelic::Agent.logger.info('Installing concurrent-ruby instrumentation') if use_prepend? - prepend_instrument ::Concurrent, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend + prepend_instrument ::Concurrent::Promises::FactoryMethods, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend else chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 787f8285c9..aa65f6b3a0 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -5,15 +5,16 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby def self.instrument! - ::Concurrent.class_eval do + # this is a module, can I still use class_eval? + ::Concurrent::Promises::FactoryMethods.class_eval do include NewRelic::Agent::Instrumentation::ConcurrentRuby - alias_method(:method_to_instrument_without_new_relic, :method_to_instrument) - alias_method(:method_to_instrument, :method_to_instrument_with_new_relic) + alias_method(:future_without_new_relic, :future) + alias_method(:future, :future_with_new_relic) - def method_to_instrument(*args) - method_to_instrument_with_new_relic(*args) do - method_to_instrument_without_new_relic(*args) + def future(*args, &task) + future_with_new_relic(*args) do + future_without_new_relic(*args, &task) end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 70073688ac..2099c3c216 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -4,10 +4,22 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby + PROMISES_FUTURE_NAME = 'Concurrent::Promises#future' - def method_to_instrument_with_new_relic(*args) - # add instrumentation content here - yield + def future_with_new_relic(*args) + segment = NewRelic::Agent::Tracer.start_segment(name: segment_name(*args)) + begin + NewRelic::Agent::Tracer.capture_segment_error(segment) { yield } + ensure + ::NewRelic::Agent::Transaction::Segment.finish(segment) + end + end + + private + + def segment_name(*args) + keyword_args = args[0] + keyword_args && keyword_args.key?(:nr_name) ? keyword_args[:nr_name] : PROMISES_FUTURE_NAME end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index e063a53f10..0dba9a3ec6 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -6,8 +6,8 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby::Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby - def method_to_instrument(*args) - method_to_instrument_with_new_relic(*args) { super } + def future(*args, &task) + future_with_new_relic(*args) { super } end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 74e555a985..878ace50aa 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -3,13 +3,39 @@ # frozen_string_literal: true class ConcurrentRubyInstrumentationTest < Minitest::Test - def setup - @stats_engine = NewRelic::Agent.instance.stats_engine + def test_promises_future_creates_segment_with_default_name + txn = in_transaction do + Concurrent::Promises.future { 'hi' } + end + segment = txn.segments[1] + + assert_equal segment.name, NewRelic::Agent::Instrumentation::ConcurrentRuby::PROMISES_FUTURE_NAME end - def teardown - NewRelic::Agent.instance.stats_engine.clear_stats + def test_promises_future_creates_segment_with_custom_name + name = 'my little pony' + txn = in_transaction do + Concurrent::Promises.future(nr_name: name) { 'hi' } + end + segment = txn.segments[1] + + assert_equal segment.name, name end - # Add tests here + # hmm -- i think I'm not understanding how errors work in this context + # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 + def test_promises_future_captures_segment_error + skip + txn = nil + begin + txn = in_transaction('concurrent') do + Concurrent::Promises.future { raise 'boom!' } + end + rescue StandardError => e + # NOOP -- allowing span and transaction to notice error + end + + assert_segment_noticed_error txn, /concurrent$/, StandardError, /boom/i + assert_transaction_noticed_error txn, StandardError + end end From 561274d6b6bee6fc3f0752879d2e6dbeade9f663 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 7 Dec 2022 17:20:41 -0800 Subject: [PATCH 06/53] Rubocop --- .../concurrent_ruby/concurrent_ruby_instrumentation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 878ace50aa..ec0aa17dd6 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -23,7 +23,7 @@ def test_promises_future_creates_segment_with_custom_name end # hmm -- i think I'm not understanding how errors work in this context - # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 + # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 def test_promises_future_captures_segment_error skip txn = nil From d0791ff1052ee51689e391d42d0efc82b6942022 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 14 Dec 2022 16:54:00 -0500 Subject: [PATCH 07/53] Instrument Concurrent::ThreadPoolExecutor#post This method is ultimately called by both Concurrent::Promises#future and Concurrent::Future#execute. By instrumenting here, we'll have more coverage no matter which syntax applications are using. --- .../agent/instrumentation/concurrent_ruby.rb | 8 ++--- .../concurrent_ruby/instrumentation.rb | 21 +++++++------- .../concurrent_ruby/prepend.rb | 4 +-- .../concurrent_ruby_instrumentation_test.rb | 29 ++++++++++++------- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index a64fdda2c3..7c10772225 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -10,16 +10,16 @@ named :'concurrent_ruby' depends_on do - defined?(::Concurrent) + defined?(Concurrent) end executes do - ::NewRelic::Agent.logger.info('Installing concurrent-ruby instrumentation') + NewRelic::Agent.logger.info('Installing concurrent-ruby instrumentation') if use_prepend? - prepend_instrument ::Concurrent::Promises::FactoryMethods, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend + prepend_instrument(Concurrent::ThreadPoolExecutor, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend) else - chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby + chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby::Chain end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 2099c3c216..cca872e1d8 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -4,22 +4,21 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby - PROMISES_FUTURE_NAME = 'Concurrent::Promises#future' + DEFAULT_NAME = 'Concurrent::ThreadPoolExecutor#post' - def future_with_new_relic(*args) - segment = NewRelic::Agent::Tracer.start_segment(name: segment_name(*args)) + def post_with_new_relic(*args) + return yield unless NewRelic::Agent::Tracer.tracing_enabled? + + current = Thread.current[:newrelic_tracer_state] + segment = NewRelic::Agent::Tracer.start_segment(name: DEFAULT_NAME) begin - NewRelic::Agent::Tracer.capture_segment_error(segment) { yield } + NewRelic::Agent::Tracer.capture_segment_error(segment) do + Thread.current[:newrelic_tracer_state] = current + yield + end ensure ::NewRelic::Agent::Transaction::Segment.finish(segment) end end - - private - - def segment_name(*args) - keyword_args = args[0] - keyword_args && keyword_args.key?(:nr_name) ? keyword_args[:nr_name] : PROMISES_FUTURE_NAME - end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index 0dba9a3ec6..9661797f84 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -6,8 +6,8 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby::Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby - def future(*args, &task) - future_with_new_relic(*args) { super } + def post(*args, &task) + post_with_new_relic(*args) { super } end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index ec0aa17dd6..6000dd8848 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -9,27 +9,36 @@ def test_promises_future_creates_segment_with_default_name end segment = txn.segments[1] - assert_equal segment.name, NewRelic::Agent::Instrumentation::ConcurrentRuby::PROMISES_FUTURE_NAME + assert_equal segment.name, NewRelic::Agent::Instrumentation::ConcurrentRuby::DEFAULT_NAME end - def test_promises_future_creates_segment_with_custom_name - name = 'my little pony' + def test_promises_future_creates_segments_for_nested_instrumented_calls + future = nil txn = in_transaction do - Concurrent::Promises.future(nr_name: name) { 'hi' } + future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')) } end - segment = txn.segments[1] - assert_equal segment.name, name + assert_equal(3, txn.segments.length) end # hmm -- i think I'm not understanding how errors work in this context # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 + # Promises.future swallows all errors + # they can be raised the result of the future as a variable and calling #value! on it + # it also works if you call raise future + # future.reason => inspect version of error + # future.value => nil if there's an error + # future.rejected? => true + # future.value! => raises the error + # raise future => raises the error + # raise future rescue $! => rescues the error, returns inspected version + def test_promises_future_captures_segment_error - skip + skip "future doesn't raise errors, so they can't be captured" txn = nil - begin - txn = in_transaction('concurrent') do - Concurrent::Promises.future { raise 'boom!' } + txn = in_transaction('concurrent') do + Concurrent::Promises.stub(:future, raise('boom!')) do + future = Concurrent::Promises.future { 'hi' } end rescue StandardError => e # NOOP -- allowing span and transaction to notice error From 6e8cbee272ec8edd0d4b20a4cc7f8a5442453dd6 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Thu, 15 Dec 2022 17:24:51 -0800 Subject: [PATCH 08/53] WIP concurrent ruby tracing thread block --- .../concurrent_ruby/instrumentation.rb | 2 -- .../concurrent_ruby/prepend.rb | 20 ++++++++++- .../multiverse/suites/concurrent_ruby/Envfile | 3 +- .../concurrent_ruby_instrumentation_test.rb | 36 ++++++++++++++++--- 4 files changed, 53 insertions(+), 8 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index cca872e1d8..3661988b70 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -9,11 +9,9 @@ module ConcurrentRuby def post_with_new_relic(*args) return yield unless NewRelic::Agent::Tracer.tracing_enabled? - current = Thread.current[:newrelic_tracer_state] segment = NewRelic::Agent::Tracer.start_segment(name: DEFAULT_NAME) begin NewRelic::Agent::Tracer.capture_segment_error(segment) do - Thread.current[:newrelic_tracer_state] = current yield end ensure diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index 9661797f84..c64906090b 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -7,7 +7,25 @@ module ConcurrentRuby::Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby def post(*args, &task) - post_with_new_relic(*args) { super } + traced_task = traced_block_concurrent(*args, &task) + post_with_new_relic(*args) { super(*args, &traced_task) } + end + + # this is basically just a copy of Tracer#thread_block_with_current_transaction with the segment name changed + def traced_block_concurrent(*args, &block) + current_txn = ::Thread.current[:newrelic_tracer_state].current_transaction if ::Thread.current[:newrelic_tracer_state] && ::Thread.current[:newrelic_tracer_state].is_execution_traced? + traced_task = proc do + begin + if current_txn + NewRelic::Agent::Tracer.state.current_transaction = current_txn + # is this one just redundant? + segment = NewRelic::Agent::Tracer.start_segment(name: "Ruby/Inner_concurrent_ruby/#{::Thread.current.object_id}") + end + yield(*args) if block.respond_to?(:call) + ensure + ::NewRelic::Agent::Transaction::Segment.finish(segment) + end + end end end end diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile index 40d0658f94..a49c692724 100644 --- a/test/multiverse/suites/concurrent_ruby/Envfile +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -2,7 +2,8 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true -instrumentation_methods :chain, :prepend +# instrumentation_methods :chain, :prepend +instrumentation_methods :prepend # just for debugging gemfile <<-RB gem 'concurrent-ruby' diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 6000dd8848..189ceb6265 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -4,6 +4,7 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test def test_promises_future_creates_segment_with_default_name + skip txn = in_transaction do Concurrent::Promises.future { 'hi' } end @@ -13,12 +14,39 @@ def test_promises_future_creates_segment_with_default_name end def test_promises_future_creates_segments_for_nested_instrumented_calls - future = nil - txn = in_transaction do - future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')) } + skip + with_config(:'instrumentation.thread.tracing' => false) do + future = nil + txn = in_transaction do + future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } + future.wait! + end + + assert_equal(4, txn.segments.length) end + end - assert_equal(3, txn.segments.length) + def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thread_tracing_enabled + # skip + with_config(:'instrumentation.thread.tracing' => true) do + future = nil + txn = in_transaction do + future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } + future.wait! + end + + # intermittent failure with thread segment missing? idk why yet + # dummy + # Concurrent::ThreadPoolExecutor#post + # Ruby/Thread/2640 + # Ruby/Inner_concurrent_ruby/2640 + # External/www.example.com/Net::HTTP/GET + txn.segments.each do |s| + puts s.name + end + + assert_equal(5, txn.segments.length) + end end # hmm -- i think I'm not understanding how errors work in this context From 8d70c30929059188acece7f96b23d88507a2cd38 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Fri, 16 Dec 2022 10:14:24 -0800 Subject: [PATCH 09/53] DRYed up the block tracing methods for concurent ruby and threads --- .../agent/configuration/default_source.rb | 7 +++++++ .../instrumentation/concurrent_ruby/chain.rb | 5 ++--- .../concurrent_ruby/instrumentation.rb | 8 +++++--- .../concurrent_ruby/prepend.rb | 19 +------------------ .../instrumentation/thread/instrumentation.rb | 2 +- lib/new_relic/agent/tracer.rb | 11 +++++++---- lib/new_relic/traced_thread.rb | 2 +- .../concurrent_ruby_instrumentation_test.rb | 15 +++++++++------ 8 files changed, 33 insertions(+), 36 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 387d5522ef..b2ac30f8ff 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1810,6 +1810,13 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :allowed_from_server => false, :description => "Controls auto-instrumentation of the Thread class at start up to automatically add tracing to all Threads created in the application." }, + :'thread_ids_enabled' => { + :default => false, + :public => false, + :type => Boolean, + :allowed_from_server => false, + :description => "If enabled, will append the current thread and fiber object ids onto the segment names of segments created in threads and concurrent ruby" + }, :'instrumentation.tilt' => { :default => "auto", :public => true, diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index b2cd93e93e..01fa439a00 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -12,9 +12,8 @@ def self.instrument! alias_method(:post, :post_with_new_relic) def post(*args, &task) - post_with_new_relic(*args) do - post_without_new_relic(*args, &task) - end + traced_task = add_task_tracing(*args, &task) + post_with_new_relic(*args) { post_without_new_relic(*args, &traced_task) } end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 3661988b70..6e03625aed 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -11,12 +11,14 @@ def post_with_new_relic(*args) segment = NewRelic::Agent::Tracer.start_segment(name: DEFAULT_NAME) begin - NewRelic::Agent::Tracer.capture_segment_error(segment) do - yield - end + yield ensure ::NewRelic::Agent::Transaction::Segment.finish(segment) end end + + def add_task_tracing(*args, &task) + NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: 'Concurrent/task', &task) + end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index c64906090b..2fc15d0a53 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -7,25 +7,8 @@ module ConcurrentRuby::Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby def post(*args, &task) - traced_task = traced_block_concurrent(*args, &task) + traced_task = add_task_tracing(*args, &task) post_with_new_relic(*args) { super(*args, &traced_task) } end - - # this is basically just a copy of Tracer#thread_block_with_current_transaction with the segment name changed - def traced_block_concurrent(*args, &block) - current_txn = ::Thread.current[:newrelic_tracer_state].current_transaction if ::Thread.current[:newrelic_tracer_state] && ::Thread.current[:newrelic_tracer_state].is_execution_traced? - traced_task = proc do - begin - if current_txn - NewRelic::Agent::Tracer.state.current_transaction = current_txn - # is this one just redundant? - segment = NewRelic::Agent::Tracer.start_segment(name: "Ruby/Inner_concurrent_ruby/#{::Thread.current.object_id}") - end - yield(*args) if block.respond_to?(:call) - ensure - ::NewRelic::Agent::Transaction::Segment.finish(segment) - end - end - end end end diff --git a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb index f2b7635742..0e36b66d70 100644 --- a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb @@ -16,7 +16,7 @@ def initialize_with_newrelic_tracing def add_thread_tracing(*args, &block) return block if skip_tracing? - NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, &block) + NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: 'Ruby/Thread', &block) end def skip_tracing? diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index a4848fd369..09e286a0c3 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -409,15 +409,18 @@ def clear_state alias_method :tl_clear, :clear_state - def thread_block_with_current_transaction(*args, &block) + def thread_block_with_current_transaction(*args, segment_name:, &block) current_txn = ::Thread.current[:newrelic_tracer_state].current_transaction if ::Thread.current[:newrelic_tracer_state] && ::Thread.current[:newrelic_tracer_state].is_execution_traced? proc do begin if current_txn - NewRelic::Agent::Tracer.state.current_transaction = current_txn - segment = NewRelic::Agent::Tracer.start_segment(name: "Ruby/Thread/#{::Thread.current.object_id}") + NewRelic::Agent::Tracer.state.current_transaction = current_txn unless NewRelic::Agent::Tracer.state.current_transaction + segment_name += "/Thread#{::Thread.current.object_id}/Fiber#{::Fiber.current.object_id}" if NewRelic::Agent.config[:'thread_ids_enabled'] + segment = NewRelic::Agent::Tracer.start_segment(name: segment_name) + end + NewRelic::Agent::Tracer.capture_segment_error(segment) do + yield(*args) if block.respond_to?(:call) end - yield(*args) if block.respond_to?(:call) ensure ::NewRelic::Agent::Transaction::Segment.finish(segment) end diff --git a/lib/new_relic/traced_thread.rb b/lib/new_relic/traced_thread.rb index b6704c6078..01dfa7fc29 100644 --- a/lib/new_relic/traced_thread.rb +++ b/lib/new_relic/traced_thread.rb @@ -29,7 +29,7 @@ def initialize(*args, &block) def create_traced_block(*args, &block) return block if NewRelic::Agent.config[:'instrumentation.thread.tracing'] # if this is on, don't double trace - NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, &block) + NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: "Ruby/TracedThread", &block) end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 189ceb6265..86c1d90f67 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -35,17 +35,20 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thr future.wait! end - # intermittent failure with thread segment missing? idk why yet + # intermittent failure with thread segment missing? i think it might be fixed now tho # dummy # Concurrent::ThreadPoolExecutor#post # Ruby/Thread/2640 - # Ruby/Inner_concurrent_ruby/2640 + # Ruby/Inner_concurrent_ruby # External/www.example.com/Net::HTTP/GET - txn.segments.each do |s| - puts s.name - end + # txn.segments.each do |s| + # puts s.name + # end + + # concurrent ruby doesn't use ruby threads when running on java, so that segment will not be present + expected_segments = RUBY_PLATFORM == 'java' ? 4 : 5 - assert_equal(5, txn.segments.length) + assert_equal(expected_segments, txn.segments.length) end end From 572bf20d9f50bc4581aef20de1eaa796d6d62e20 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Fri, 16 Dec 2022 14:34:03 -0800 Subject: [PATCH 10/53] removed check current txn check from thread block Turns out we do need to copy the current transaction over every time. Concurrent ruby can create a thread prior to the transaction beginning or during a different transaction, but continue to use the same thread. This ensures the agent is always using the most accurate source for the current transaction --- lib/new_relic/agent/tracer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index 09e286a0c3..1f78357e44 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -414,7 +414,7 @@ def thread_block_with_current_transaction(*args, segment_name:, &block) proc do begin if current_txn - NewRelic::Agent::Tracer.state.current_transaction = current_txn unless NewRelic::Agent::Tracer.state.current_transaction + NewRelic::Agent::Tracer.state.current_transaction = current_txn segment_name += "/Thread#{::Thread.current.object_id}/Fiber#{::Fiber.current.object_id}" if NewRelic::Agent.config[:'thread_ids_enabled'] segment = NewRelic::Agent::Tracer.start_segment(name: segment_name) end From 47c56eb8ac57c6488e3c477bd3f31b528d1b6963 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Fri, 16 Dec 2022 14:44:26 -0800 Subject: [PATCH 11/53] resolve intermittent failures Concurrent ruby will reuse threads created before a transaction began, which causes the thread creation segment to not appear in the expected transaction. --- .../concurrent_ruby_instrumentation_test.rb | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 86c1d90f67..be69fdd2d8 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -4,17 +4,18 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test def test_promises_future_creates_segment_with_default_name - skip txn = in_transaction do - Concurrent::Promises.future { 'hi' } + future = Concurrent::Promises.future { 'hi' } + future.wait! end - segment = txn.segments[1] - assert_equal segment.name, NewRelic::Agent::Instrumentation::ConcurrentRuby::DEFAULT_NAME + expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task'] + + assert_equal(3, txn.segments.length) + assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end def test_promises_future_creates_segments_for_nested_instrumented_calls - skip with_config(:'instrumentation.thread.tracing' => false) do future = nil txn = in_transaction do @@ -22,12 +23,14 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls future.wait! end + expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task', 'External/www.example.com/Net::HTTP/GET'] + assert_equal(4, txn.segments.length) + assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end end def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thread_tracing_enabled - # skip with_config(:'instrumentation.thread.tracing' => true) do future = nil txn = in_transaction do @@ -35,20 +38,11 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thr future.wait! end - # intermittent failure with thread segment missing? i think it might be fixed now tho - # dummy - # Concurrent::ThreadPoolExecutor#post - # Ruby/Thread/2640 - # Ruby/Inner_concurrent_ruby - # External/www.example.com/Net::HTTP/GET - # txn.segments.each do |s| - # puts s.name - # end - - # concurrent ruby doesn't use ruby threads when running on java, so that segment will not be present - expected_segments = RUBY_PLATFORM == 'java' ? 4 : 5 - - assert_equal(expected_segments, txn.segments.length) + expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task', 'External/www.example.com/Net::HTTP/GET'] + # We can't check the number of segments when thread tracing is enabled because we can not rely on concurrent + # ruby creating threads during this transaction, as it can reuse threads that were created previously. + # Instead, we check to make sure the segments that should be present are. + assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end end From f7a883023e4d894552b000778950fcb83542aa56 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Fri, 16 Dec 2022 14:55:24 -0800 Subject: [PATCH 12/53] enable both chain and prepend for concurrent ruby --- test/multiverse/suites/concurrent_ruby/Envfile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile index a49c692724..40d0658f94 100644 --- a/test/multiverse/suites/concurrent_ruby/Envfile +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -2,8 +2,7 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true -# instrumentation_methods :chain, :prepend -instrumentation_methods :prepend # just for debugging +instrumentation_methods :chain, :prepend gemfile <<-RB gem 'concurrent-ruby' From 3784309abd3cd32e048b9097b4f22eb90c2e791a Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Fri, 16 Dec 2022 17:55:46 -0800 Subject: [PATCH 13/53] WIP instrument Rejected state --- .../agent/instrumentation/concurrent_ruby.rb | 15 +++++++++++++++ .../concurrent_ruby_instrumentation_test.rb | 16 ++++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index 7c10772225..81eebff4e6 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -18,8 +18,23 @@ if use_prepend? prepend_instrument(Concurrent::ThreadPoolExecutor, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend) + # prepend_instrument() + # prepend_instrument() + extra_prepend else chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby::Chain end end end + +def extra_prepend + # Concurrent::Promises::InternalStates::Rejected + Concurrent::Promises.const_get(:'InternalStates')::Rejected.prepend(TestingStuff) +end + +module TestingStuff + def initialize(*args) + NewRelic::Agent.notice_error(args.last) + super + end +end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index be69fdd2d8..6f5559f1bc 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -4,6 +4,7 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test def test_promises_future_creates_segment_with_default_name + skip txn = in_transaction do future = Concurrent::Promises.future { 'hi' } future.wait! @@ -16,6 +17,7 @@ def test_promises_future_creates_segment_with_default_name end def test_promises_future_creates_segments_for_nested_instrumented_calls + skip with_config(:'instrumentation.thread.tracing' => false) do future = nil txn = in_transaction do @@ -31,6 +33,7 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls end def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thread_tracing_enabled + skip with_config(:'instrumentation.thread.tracing' => true) do future = nil txn = in_transaction do @@ -59,17 +62,18 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thr # raise future rescue $! => rescues the error, returns inspected version def test_promises_future_captures_segment_error - skip "future doesn't raise errors, so they can't be captured" + # skip "future doesn't raise errors, so they can't be captured" txn = nil txn = in_transaction('concurrent') do - Concurrent::Promises.stub(:future, raise('boom!')) do - future = Concurrent::Promises.future { 'hi' } - end + future = Concurrent::Promises.future { raise 'hi' } + # future.reason + future.wait! rescue StandardError => e # NOOP -- allowing span and transaction to notice error end - assert_segment_noticed_error txn, /concurrent$/, StandardError, /boom/i - assert_transaction_noticed_error txn, StandardError + # binding.irb + + assert_segment_noticed_error txn, /Concurrent\/task/, /RuntimeError/, /hi/i end end From 728cf1e51fc2e79304bdce399e74192b5c17796f Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 14:38:19 -0800 Subject: [PATCH 14/53] Moved concurrent ruby instrumentation out into its own modules and added chain instrumentation for errors --- .../agent/instrumentation/concurrent_ruby.rb | 20 +++++------------- .../instrumentation/concurrent_ruby/chain.rb | 15 ++++++++++++- .../concurrent_ruby/prepend.rb | 21 ++++++++++++++----- .../concurrent_ruby_instrumentation_test.rb | 3 --- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index 81eebff4e6..7f557045d2 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -18,23 +18,13 @@ if use_prepend? prepend_instrument(Concurrent::ThreadPoolExecutor, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend) - # prepend_instrument() - # prepend_instrument() - extra_prepend + + [::Concurrent::Promises.const_get(:'InternalStates')::Rejected, + ::Concurrent::Promises.const_get(:'InternalStates')::PartiallyRejected].each do |klass| + klass.prepend(NewRelic::Agent::Instrumentation::ConcurrentRuby::ErrorPrepend) + end else chain_instrument NewRelic::Agent::Instrumentation::ConcurrentRuby::Chain end end end - -def extra_prepend - # Concurrent::Promises::InternalStates::Rejected - Concurrent::Promises.const_get(:'InternalStates')::Rejected.prepend(TestingStuff) -end - -module TestingStuff - def initialize(*args) - NewRelic::Agent.notice_error(args.last) - super - end -end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 01fa439a00..2344eb1dca 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -9,13 +9,26 @@ def self.instrument! include NewRelic::Agent::Instrumentation::ConcurrentRuby alias_method(:post_without_new_relic, :post) - alias_method(:post, :post_with_new_relic) def post(*args, &task) traced_task = add_task_tracing(*args, &task) post_with_new_relic(*args) { post_without_new_relic(*args, &traced_task) } end end + + [::Concurrent::Promises.const_get(:'InternalStates')::Rejected, + ::Concurrent::Promises.const_get(:'InternalStates')::PartiallyRejected].each do |klass| + klass.class_eval do + alias_method(:initialize_without_new_relic, :initialize) + + # Uses args.last to record the error becuase the methods that this will monkey patch + # look like: initialize(reason) & initialize(value, reason) + def initialize(*args) + NewRelic::Agent.notice_error(args.last) + initialize_without_new_relic(*args) + end + end + end end end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index 2fc15d0a53..d247fce647 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -3,12 +3,23 @@ # frozen_string_literal: true module NewRelic::Agent::Instrumentation - module ConcurrentRuby::Prepend - include NewRelic::Agent::Instrumentation::ConcurrentRuby + module ConcurrentRuby + module Prepend + include NewRelic::Agent::Instrumentation::ConcurrentRuby - def post(*args, &task) - traced_task = add_task_tracing(*args, &task) - post_with_new_relic(*args) { super(*args, &traced_task) } + def post(*args, &task) + traced_task = add_task_tracing(*args, &task) + post_with_new_relic(*args) { super(*args, &traced_task) } + end + end + + module ErrorPrepend + # Uses args.last to record the error becuase the methods that this will be prepepnded to + # look like: initialize(reason) & initialize(value, reason) + def initialize(*args) + NewRelic::Agent.notice_error(args.last) + super + end end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 6f5559f1bc..7d5f4450af 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -4,7 +4,6 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test def test_promises_future_creates_segment_with_default_name - skip txn = in_transaction do future = Concurrent::Promises.future { 'hi' } future.wait! @@ -17,7 +16,6 @@ def test_promises_future_creates_segment_with_default_name end def test_promises_future_creates_segments_for_nested_instrumented_calls - skip with_config(:'instrumentation.thread.tracing' => false) do future = nil txn = in_transaction do @@ -33,7 +31,6 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls end def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thread_tracing_enabled - skip with_config(:'instrumentation.thread.tracing' => true) do future = nil txn = in_transaction do From 6753e08082eccd3cbe2252bd673e8830561f1f35 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 14:49:46 -0800 Subject: [PATCH 15/53] removed rubocop disables that aren't needed --- test/agent_helper.rb | 4 ++-- .../multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb | 2 +- .../distributed_tracing_cross_agent_test.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/agent_helper.rb b/test/agent_helper.rb index c791640ec7..fcdc53839c 100644 --- a/test/agent_helper.rb +++ b/test/agent_helper.rb @@ -31,7 +31,7 @@ def fake_guid(length = 16) end def assert_between(floor, ceiling, value, message = "expected #{floor} <= #{value} <= #{ceiling}") - assert((floor <= value && value <= ceiling), message) # rubocop:disable Minitest/AssertWithExpectedArgument + assert((floor <= value && value <= ceiling), message) end def assert_in_delta(expected, actual, delta) @@ -236,7 +236,7 @@ def assert_metrics_recorded(expected) msg += "\nDid find specs: [\n#{matches.join(",\n")}\n]" unless matches.empty? msg += "\nAll specs in there were: #{format_metric_spec_list(all_specs)}" - assert(actual_stats, msg) # rubocop:disable Minitest/AssertWithExpectedArgument + assert(actual_stats, msg) end assert_stats_has_values(actual_stats, expected_spec, expected_attrs) diff --git a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb b/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb index ce32a378ee..bab31f0a45 100644 --- a/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb +++ b/test/multiverse/suites/sidekiq/sidekiq_instrumentation_test.rb @@ -201,7 +201,7 @@ def assert_metric_and_call_count(name, expected_call_count) metric = metrics.find { |m| m[0]['name'] == name } message = "Could not find metric named #{name}. Did have metrics:\n" + metrics.map { |m| m[0]['name'] }.join("\t\n") - assert(metric, message) # rubocop:disable Minitest/AssertWithExpectedArgument + assert(metric, message) call_count = metric[1][0] diff --git a/test/new_relic/agent/distributed_tracing/distributed_tracing_cross_agent_test.rb b/test/new_relic/agent/distributed_tracing/distributed_tracing_cross_agent_test.rb index 3a8c8633e2..3c144213de 100644 --- a/test/new_relic/agent/distributed_tracing/distributed_tracing_cross_agent_test.rb +++ b/test/new_relic/agent/distributed_tracing/distributed_tracing_cross_agent_test.rb @@ -171,7 +171,7 @@ def verify_attributes(test_case_attributes, actual_attributes, event_type) (test_case_attributes['expected'] || []).each do |key| msg = %Q(Missing expected #{event_type} attribute "#{key}") - assert actual_attributes.has_key?(key), msg # rubocop:disable Minitest/AssertWithExpectedArgument + assert actual_attributes.has_key?(key), msg end (test_case_attributes['unexpected'] || []).each do |key| From 233f0d0cf5e72556955a9606300b479c03c9e996 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 14:53:40 -0800 Subject: [PATCH 16/53] remove specific rubygems version to upgrade to --- .github/workflows/scripts/setup_bundler | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index fc4e25195d..b87d0e0c40 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -35,7 +35,8 @@ function update_to_desired_rubygems_version { update_rubygems else echo "DEBUG: running 'gem update --system --force'" - gem update --system 3.0.6 --force + # gem update --system 3.0.6 --force + gem update --system --force fi } From f74f0fdfa4318786c4102a65721351f21dc39a4e Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 14:59:42 -0800 Subject: [PATCH 17/53] pin rubys 2.3 2.4 2.5 to rubygems 3.0.6 --- .github/workflows/scripts/setup_bundler | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index b87d0e0c40..66a1641823 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -33,9 +33,10 @@ function update_to_desired_rubygems_version { gem install rubygems-update:2.7.11 echo "DEBUG: running 'rubygems-update'" update_rubygems + elif [[ $RUBY_VERSION =~ 2.[345] ]]; then + gem update --system 3.0.6 --force else echo "DEBUG: running 'gem update --system --force'" - # gem update --system 3.0.6 --force gem update --system --force fi } From f9eca26b899e4a877bee280524028698756b5c1a Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:10:37 -0800 Subject: [PATCH 18/53] what errors exactly happen with 3.0.6 rubygems --- .github/workflows/scripts/setup_bundler | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 66a1641823..5def92830d 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -33,11 +33,12 @@ function update_to_desired_rubygems_version { gem install rubygems-update:2.7.11 echo "DEBUG: running 'rubygems-update'" update_rubygems - elif [[ $RUBY_VERSION =~ 2.[345] ]]; then - gem update --system 3.0.6 --force + # elif [[ $RUBY_VERSION =~ 2.[345] ]]; then else echo "DEBUG: running 'gem update --system --force'" - gem update --system --force + gem update --system 3.0.6 --force + # else + # gem update --system --force fi } From 17fd78e8db33bc66dd577f64fb0136919a640d15 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:23:36 -0800 Subject: [PATCH 19/53] specific rubygems versions for different rubys --- .github/workflows/scripts/setup_bundler | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 5def92830d..5d8b0e0cac 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -33,10 +33,15 @@ function update_to_desired_rubygems_version { gem install rubygems-update:2.7.11 echo "DEBUG: running 'rubygems-update'" update_rubygems - # elif [[ $RUBY_VERSION =~ 2.[345] ]]; then + elif [[ $RUBY_VERSION =~ 2.[345] ]]; then + gem update --system 3.0.6 --force + elif [[$RUBY_VERSION =~ 2.[6]]]; then + gem update --system 3.1.6 --force else echo "DEBUG: running 'gem update --system --force'" - gem update --system 3.0.6 --force + gem update --system 3.4.1 --force + # gem update --system 3.1.6 --force + # 3.4.1 # else # gem update --system --force fi From d9cf4790ba4bb05ef6337e9b49c724763b11e0ba Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:35:22 -0800 Subject: [PATCH 20/53] more debugging statements for bundler script --- .github/workflows/scripts/setup_bundler | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 5d8b0e0cac..087d8bdeb7 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -34,17 +34,18 @@ function update_to_desired_rubygems_version { echo "DEBUG: running 'rubygems-update'" update_rubygems elif [[ $RUBY_VERSION =~ 2.[345] ]]; then + echo "DEBUG: running 'gem update --system 3.0.6 --force'" gem update --system 3.0.6 --force elif [[$RUBY_VERSION =~ 2.[6]]]; then + echo "DEBUG: running 'gem update --system 3.1.6 --force'" gem update --system 3.1.6 --force else - echo "DEBUG: running 'gem update --system --force'" + echo "DEBUG: running 'gem update --system 3.4.1 --force'" gem update --system 3.4.1 --force - # gem update --system 3.1.6 --force - # 3.4.1 - # else - # gem update --system --force fi + echo "Rubygems version installed: " + gem --version + echo "---------------------------" } function install_desired_bundler_version { From e6396f603c6a0b9947eb243d586cdfbfba58617f Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:47:17 -0800 Subject: [PATCH 21/53] silent rubygems update --- .github/workflows/scripts/setup_bundler | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 087d8bdeb7..b5440223db 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -35,13 +35,15 @@ function update_to_desired_rubygems_version { update_rubygems elif [[ $RUBY_VERSION =~ 2.[345] ]]; then echo "DEBUG: running 'gem update --system 3.0.6 --force'" - gem update --system 3.0.6 --force - elif [[$RUBY_VERSION =~ 2.[6]]]; then + gem update --system 3.0.6 --force --silent + elif [[ $RUBY_VERSION = "2.6.10" ]]; then + # Rails 4 still runs on 2.6 which requires bundler 1.17 + # rubygems > 3.1.6 forces bundler 2 to be default which breaks rails 4 echo "DEBUG: running 'gem update --system 3.1.6 --force'" - gem update --system 3.1.6 --force + gem update --system 3.1.6 --force --silent else echo "DEBUG: running 'gem update --system 3.4.1 --force'" - gem update --system 3.4.1 --force + gem update --system 3.4.1 --force --silent fi echo "Rubygems version installed: " gem --version From 68ae6fee8c00ab805baad99cbf92d04ed6abb201 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:52:21 -0800 Subject: [PATCH 22/53] echo command in same one line --- .github/workflows/scripts/setup_bundler | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index b5440223db..29e9dbb179 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -45,9 +45,7 @@ function update_to_desired_rubygems_version { echo "DEBUG: running 'gem update --system 3.4.1 --force'" gem update --system 3.4.1 --force --silent fi - echo "Rubygems version installed: " - gem --version - echo "---------------------------" + echo "DEBUG: Rubygems version installed: $(gem --version)" } function install_desired_bundler_version { From fe03b0d68c11c687e3340405f398a4109e344b72 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 15:56:03 -0800 Subject: [PATCH 23/53] dont return early on some ruby versions --- .github/workflows/scripts/setup_bundler | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 29e9dbb179..8efc0c5461 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -35,7 +35,7 @@ function update_to_desired_rubygems_version { update_rubygems elif [[ $RUBY_VERSION =~ 2.[345] ]]; then echo "DEBUG: running 'gem update --system 3.0.6 --force'" - gem update --system 3.0.6 --force --silent + gem update --system 3.0.6 --force elif [[ $RUBY_VERSION = "2.6.10" ]]; then # Rails 4 still runs on 2.6 which requires bundler 1.17 # rubygems > 3.1.6 forces bundler 2 to be default which breaks rails 4 @@ -111,10 +111,10 @@ function set_up_bundler { return fi - if ! using_old_rails && ! using_old_ruby; then - echo "DEBUG: Skipping Bundler setup, as neither an old Rails or Ruby version is in use" - return - fi + # if ! using_old_rails && ! using_old_ruby; then + # echo "DEBUG: Skipping Bundler setup, as neither an old Rails or Ruby version is in use" + # return + # fi update_to_desired_rubygems_version install_desired_bundler_version From 8257fd239aec82cc6efb789aa08c39ea0dd1a4cc Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 16:11:00 -0800 Subject: [PATCH 24/53] pin 2.6 to 306 --- .github/workflows/scripts/setup_bundler | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index 8efc0c5461..c9524315c4 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -33,17 +33,17 @@ function update_to_desired_rubygems_version { gem install rubygems-update:2.7.11 echo "DEBUG: running 'rubygems-update'" update_rubygems - elif [[ $RUBY_VERSION =~ 2.[345] ]]; then + elif [[ $RUBY_VERSION =~ 2.[3456] ]]; then echo "DEBUG: running 'gem update --system 3.0.6 --force'" - gem update --system 3.0.6 --force - elif [[ $RUBY_VERSION = "2.6.10" ]]; then + gem update --system 3.0.6 --force >/dev/null + # elif [[ $RUBY_VERSION = "2.6.10" ]]; then # Rails 4 still runs on 2.6 which requires bundler 1.17 # rubygems > 3.1.6 forces bundler 2 to be default which breaks rails 4 - echo "DEBUG: running 'gem update --system 3.1.6 --force'" - gem update --system 3.1.6 --force --silent + # echo "DEBUG: running 'gem update --system 3.1.6 --force'" + # gem update --system 3.1.6 --force >/dev/null else echo "DEBUG: running 'gem update --system 3.4.1 --force'" - gem update --system 3.4.1 --force --silent + gem update --system 3.4.1 --force >/dev/null fi echo "DEBUG: Rubygems version installed: $(gem --version)" } From 4c6e7491a098b6b7ce89326863036e6396cc9c79 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 16:23:30 -0800 Subject: [PATCH 25/53] remove commented otu code --- .github/workflows/scripts/setup_bundler | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/scripts/setup_bundler b/.github/workflows/scripts/setup_bundler index c9524315c4..b54750967a 100755 --- a/.github/workflows/scripts/setup_bundler +++ b/.github/workflows/scripts/setup_bundler @@ -33,14 +33,9 @@ function update_to_desired_rubygems_version { gem install rubygems-update:2.7.11 echo "DEBUG: running 'rubygems-update'" update_rubygems - elif [[ $RUBY_VERSION =~ 2.[3456] ]]; then + elif [[ $RUBY_VERSION =~ 2.[^7] ]]; then echo "DEBUG: running 'gem update --system 3.0.6 --force'" gem update --system 3.0.6 --force >/dev/null - # elif [[ $RUBY_VERSION = "2.6.10" ]]; then - # Rails 4 still runs on 2.6 which requires bundler 1.17 - # rubygems > 3.1.6 forces bundler 2 to be default which breaks rails 4 - # echo "DEBUG: running 'gem update --system 3.1.6 --force'" - # gem update --system 3.1.6 --force >/dev/null else echo "DEBUG: running 'gem update --system 3.4.1 --force'" gem update --system 3.4.1 --force >/dev/null @@ -111,11 +106,6 @@ function set_up_bundler { return fi - # if ! using_old_rails && ! using_old_ruby; then - # echo "DEBUG: Skipping Bundler setup, as neither an old Rails or Ruby version is in use" - # return - # fi - update_to_desired_rubygems_version install_desired_bundler_version configure_bundler From 4268a4c995f0aaf30f531298f36e5557ee1a0ec0 Mon Sep 17 00:00:00 2001 From: Tanna McClure Date: Tue, 27 Dec 2022 16:35:04 -0800 Subject: [PATCH 26/53] remove :: rubocop doesn't like --- lib/new_relic/agent/instrumentation/concurrent_ruby.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index 7f557045d2..ab525775a8 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -19,8 +19,8 @@ if use_prepend? prepend_instrument(Concurrent::ThreadPoolExecutor, NewRelic::Agent::Instrumentation::ConcurrentRuby::Prepend) - [::Concurrent::Promises.const_get(:'InternalStates')::Rejected, - ::Concurrent::Promises.const_get(:'InternalStates')::PartiallyRejected].each do |klass| + [Concurrent::Promises.const_get(:'InternalStates')::Rejected, + Concurrent::Promises.const_get(:'InternalStates')::PartiallyRejected].each do |klass| klass.prepend(NewRelic::Agent::Instrumentation::ConcurrentRuby::ErrorPrepend) end else From 4590d5acc9d7e6ebdde441c9fc3db4007be7cfdb Mon Sep 17 00:00:00 2001 From: "Kayla Reopelle (she/her)" <87386821+kaylareopelle@users.noreply.github.com> Date: Wed, 28 Dec 2022 13:25:16 -0500 Subject: [PATCH 27/53] Update config descriptions and comments There were some errors related to grammar and spelling. --- lib/new_relic/agent/configuration/default_source.rb | 4 ++-- .../agent/instrumentation/concurrent_ruby/prepend.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb index 0b427cf530..d5b4fafa4a 100644 --- a/lib/new_relic/agent/configuration/default_source.rb +++ b/lib/new_relic/agent/configuration/default_source.rb @@ -1575,7 +1575,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :type => String, :dynamic_name => true, :allowed_from_server => false, - :description => 'Controls auto-instrumentation of the concurrent_ruby library at start up. May be one of [auto|prepend|chain|disabled].' + :description => 'Controls auto-instrumentation of the concurrent-ruby library at start up. May be one of [auto|prepend|chain|disabled].' }, :'instrumentation.curb' => { :default => instrumentation_value_of(:disable_curb), @@ -1815,7 +1815,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil) :public => false, :type => Boolean, :allowed_from_server => false, - :description => "If enabled, will append the current thread and fiber object ids onto the segment names of segments created in threads and concurrent ruby" + :description => "If enabled, will append the current Thread and Fiber object ids onto the segment names of segments created in Threads and concurrent-ruby" }, :'instrumentation.tilt' => { :default => "auto", diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index d247fce647..d7e65376cb 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -14,7 +14,7 @@ def post(*args, &task) end module ErrorPrepend - # Uses args.last to record the error becuase the methods that this will be prepepnded to + # Uses args.last to record the error because the methods that this will be prepended to # look like: initialize(reason) & initialize(value, reason) def initialize(*args) NewRelic::Agent.notice_error(args.last) From d4a1f4486f37a7146373b36d45e5c0cf736d8811 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 13:35:32 -0500 Subject: [PATCH 28/53] Clean up test file --- .../concurrent_ruby_instrumentation_test.rb | 62 ++++++++----------- 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 7d5f4450af..84ffec2990 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -3,6 +3,19 @@ # frozen_string_literal: true class ConcurrentRubyInstrumentationTest < Minitest::Test + EXPECTED_SEGMENTS_FOR_NESTED_CALLS = [ + 'Concurrent::ThreadPoolExecutor#post', + 'Concurrent/task', + 'External/www.example.com/Net::HTTP/GET' + ] + + def concurrent_promises_calls_net_http_in_block + in_transaction do + future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } + future.wait! + end + end + def test_promises_future_creates_segment_with_default_name txn = in_transaction do future = Concurrent::Promises.future { 'hi' } @@ -17,60 +30,37 @@ def test_promises_future_creates_segment_with_default_name def test_promises_future_creates_segments_for_nested_instrumented_calls with_config(:'instrumentation.thread.tracing' => false) do - future = nil - txn = in_transaction do - future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } - future.wait! - end - - expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task', 'External/www.example.com/Net::HTTP/GET'] + txn = concurrent_promises_calls_net_http_in_block assert_equal(4, txn.segments.length) - assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) + assert EXPECTED_SEGMENTS_FOR_NESTED_CALLS.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end end def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thread_tracing_enabled with_config(:'instrumentation.thread.tracing' => true) do - future = nil - txn = in_transaction do - future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } - future.wait! - end + txn = concurrent_promises_calls_net_http_in_block - expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task', 'External/www.example.com/Net::HTTP/GET'] # We can't check the number of segments when thread tracing is enabled because we can not rely on concurrent # ruby creating threads during this transaction, as it can reuse threads that were created previously. # Instead, we check to make sure the segments that should be present are. - assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) + assert EXPECTED_SEGMENTS_FOR_NESTED_CALLS.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end end - # hmm -- i think I'm not understanding how errors work in this context - # https://github.com/ruby-concurrency/concurrent-ruby/blob/master/docs-source/promises.in.md?plain=1#L81 - # Promises.future swallows all errors - # they can be raised the result of the future as a variable and calling #value! on it - # it also works if you call raise future - # future.reason => inspect version of error - # future.value => nil if there's an error - # future.rejected? => true - # future.value! => raises the error - # raise future => raises the error - # raise future rescue $! => rescues the error, returns inspected version - def test_promises_future_captures_segment_error - # skip "future doesn't raise errors, so they can't be captured" txn = nil - txn = in_transaction('concurrent') do - future = Concurrent::Promises.future { raise 'hi' } - # future.reason - future.wait! - rescue StandardError => e - # NOOP -- allowing span and transaction to notice error + txn = in_transaction do + # TODO: OLD RUBIES - RUBY_VERSION 2.2 + # specific "begin" in block can be removed once we drop support for 2.2 + begin + future = Concurrent::Promises.future { raise 'hi' } + future.wait! + rescue StandardError => e + # NOOP -- allowing span and transaction to notice error + end end - # binding.irb - assert_segment_noticed_error txn, /Concurrent\/task/, /RuntimeError/, /hi/i end end From 3afaa977da82c7fa7242279d689e1f81415a93a2 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 13:39:01 -0500 Subject: [PATCH 29/53] More test refactors --- .../concurrent_ruby_instrumentation_test.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 84ffec2990..49268f3d18 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -9,19 +9,21 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test 'External/www.example.com/Net::HTTP/GET' ] - def concurrent_promises_calls_net_http_in_block + # Helper methods + def future_in_transaction(&block) in_transaction do - future = Concurrent::Promises.future { Net::HTTP.get(URI('http://www.example.com')); } + future = Concurrent::Promises.future { yield } future.wait! end end - def test_promises_future_creates_segment_with_default_name - txn = in_transaction do - future = Concurrent::Promises.future { 'hi' } - future.wait! - end + # Tests + def concurrent_promises_calls_net_http_in_block + future_in_transaction { Net::HTTP.get(URI('http://www.example.com')) } + end + def test_promises_future_creates_segment_with_default_name + txn = future_in_transaction { 'hi' } expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task'] assert_equal(3, txn.segments.length) @@ -49,7 +51,6 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thr end def test_promises_future_captures_segment_error - txn = nil txn = in_transaction do # TODO: OLD RUBIES - RUBY_VERSION 2.2 # specific "begin" in block can be removed once we drop support for 2.2 From 7147f51b6c8b2e5c3115dfeddb0348d51d8b40ad Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 18:46:57 -0500 Subject: [PATCH 30/53] Pass segment parent into thread block Previously, calls to Concurrent::Promises#future would create a fragmented span for the thread generated by Concurrent/Task. Now, the current segment is passed into thread_block_with_current_transaction to stop the fragmentation. --- .../concurrent_ruby/instrumentation.rb | 8 +++++++- .../agent/instrumentation/thread/instrumentation.rb | 7 ++++++- lib/new_relic/agent/tracer.rb | 10 +++++----- lib/new_relic/traced_thread.rb | 7 ++++++- .../concurrent_ruby_instrumentation_test.rb | 12 +++++++++--- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 6e03625aed..66754f0646 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -5,6 +5,7 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby DEFAULT_NAME = 'Concurrent::ThreadPoolExecutor#post' + TASK_NAME = 'Concurrent/Task' def post_with_new_relic(*args) return yield unless NewRelic::Agent::Tracer.tracing_enabled? @@ -18,7 +19,12 @@ def post_with_new_relic(*args) end def add_task_tracing(*args, &task) - NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: 'Concurrent/task', &task) + NewRelic::Agent::Tracer.thread_block_with_current_transaction( + *args, + segment_name: TASK_NAME, + parent: NewRelic::Agent::Tracer.current_segment, + &task + ) end end end diff --git a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb index 0e36b66d70..a9643b240f 100644 --- a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb @@ -16,7 +16,12 @@ def initialize_with_newrelic_tracing def add_thread_tracing(*args, &block) return block if skip_tracing? - NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: 'Ruby/Thread', &block) + NewRelic::Agent::Tracer.thread_block_with_current_transaction( + *args, + segment_name: 'Ruby/Thread', + parent: NewRelic::Agent::Tracer.current_segment, + &block + ) end def skip_tracing? diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index 1f78357e44..49358b0950 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -409,17 +409,17 @@ def clear_state alias_method :tl_clear, :clear_state - def thread_block_with_current_transaction(*args, segment_name:, &block) + def thread_block_with_current_transaction(*args, segment_name:, parent:, &block) current_txn = ::Thread.current[:newrelic_tracer_state].current_transaction if ::Thread.current[:newrelic_tracer_state] && ::Thread.current[:newrelic_tracer_state].is_execution_traced? proc do begin if current_txn NewRelic::Agent::Tracer.state.current_transaction = current_txn segment_name += "/Thread#{::Thread.current.object_id}/Fiber#{::Fiber.current.object_id}" if NewRelic::Agent.config[:'thread_ids_enabled'] - segment = NewRelic::Agent::Tracer.start_segment(name: segment_name) - end - NewRelic::Agent::Tracer.capture_segment_error(segment) do - yield(*args) if block.respond_to?(:call) + segment = NewRelic::Agent::Tracer.start_segment(name: segment_name, parent: parent) + NewRelic::Agent::Tracer.capture_segment_error(segment) do + yield(*args) if block.respond_to?(:call) + end end ensure ::NewRelic::Agent::Transaction::Segment.finish(segment) diff --git a/lib/new_relic/traced_thread.rb b/lib/new_relic/traced_thread.rb index 01dfa7fc29..bd2b4c5524 100644 --- a/lib/new_relic/traced_thread.rb +++ b/lib/new_relic/traced_thread.rb @@ -29,7 +29,12 @@ def initialize(*args, &block) def create_traced_block(*args, &block) return block if NewRelic::Agent.config[:'instrumentation.thread.tracing'] # if this is on, don't double trace - NewRelic::Agent::Tracer.thread_block_with_current_transaction(*args, segment_name: "Ruby/TracedThread", &block) + NewRelic::Agent::Tracer.thread_block_with_current_transaction( + *args, + segment_name: 'Ruby/TracedThread', + parent: NewRelic::Agent::Tracer.current_segment, + &block + ) end end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 49268f3d18..0bae538cf9 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -5,7 +5,7 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test EXPECTED_SEGMENTS_FOR_NESTED_CALLS = [ 'Concurrent::ThreadPoolExecutor#post', - 'Concurrent/task', + 'Concurrent/Task', 'External/www.example.com/Net::HTTP/GET' ] @@ -24,7 +24,7 @@ def concurrent_promises_calls_net_http_in_block def test_promises_future_creates_segment_with_default_name txn = future_in_transaction { 'hi' } - expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/task'] + expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/Task'] assert_equal(3, txn.segments.length) assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) @@ -62,6 +62,12 @@ def test_promises_future_captures_segment_error end end - assert_segment_noticed_error txn, /Concurrent\/task/, /RuntimeError/, /hi/i + assert_segment_noticed_error txn, /Concurrent\/Task/, /RuntimeError/, /hi/i + end + + def test_task_segment_has_correct_parent + txn = future_in_transaction { 'hi' } + task_segment = txn.segments.find{|n| n.name == 'Concurrent/Task'} + assert_equal task_segment.parent.name, txn.best_name end end From 0bbc679eeb83eb5da993419f04275a00ea0207f8 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 18:51:05 -0500 Subject: [PATCH 31/53] Move tracing_enabled? check to prepend/chain Since two methods are called in the post instrumentation moving tracing_enabled? to the prepend/chain modules allows the check run once --- .../agent/instrumentation/concurrent_ruby/chain.rb | 2 ++ .../instrumentation/concurrent_ruby/instrumentation.rb | 2 -- .../agent/instrumentation/concurrent_ruby/prepend.rb | 2 ++ .../concurrent_ruby_instrumentation_test.rb | 8 ++++++++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 2344eb1dca..041d785134 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -11,6 +11,8 @@ def self.instrument! alias_method(:post_without_new_relic, :post) def post(*args, &task) + return post_without_new_relic(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled? + traced_task = add_task_tracing(*args, &task) post_with_new_relic(*args) { post_without_new_relic(*args, &traced_task) } end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 66754f0646..eca2a2db58 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -8,8 +8,6 @@ module ConcurrentRuby TASK_NAME = 'Concurrent/Task' def post_with_new_relic(*args) - return yield unless NewRelic::Agent::Tracer.tracing_enabled? - segment = NewRelic::Agent::Tracer.start_segment(name: DEFAULT_NAME) begin yield diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index d7e65376cb..2ad191d246 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -8,6 +8,8 @@ module Prepend include NewRelic::Agent::Instrumentation::ConcurrentRuby def post(*args, &task) + return super(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled? + traced_task = add_task_tracing(*args, &task) post_with_new_relic(*args) { super(*args, &traced_task) } end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 0bae538cf9..642425f3d5 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -70,4 +70,12 @@ def test_task_segment_has_correct_parent task_segment = txn.segments.find{|n| n.name == 'Concurrent/Task'} assert_equal task_segment.parent.name, txn.best_name end + + def test_segment_not_created_if_tracing_disabled + NewRelic::Agent::Tracer.stub :tracing_enabled?, false do + txn = future_in_transaction { 'the revolution will not be televised' } + assert_predicate txn.segments, :one? + assert_equal txn.segments.first.name, txn.best_name + end + end end From 8aadf5473509a4c283549497e98bfb1d3e8d9e41 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 18:51:12 -0500 Subject: [PATCH 32/53] Prevent nil output errors While running a playground application, output was nil, raising an error when chomp was called. This will allow the method to complete if output is nil. --- lib/new_relic/helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/helper.rb b/lib/new_relic/helper.rb index e569b337ec..0cd415030a 100644 --- a/lib/new_relic/helper.rb +++ b/lib/new_relic/helper.rb @@ -63,7 +63,7 @@ def run_command(command) raise NewRelic::CommandRunFailedError.new("Failed to run command '#{command}': #{message}") end - output.chomp + output.chomp if output end # TODO: Open3 defers the actual execution of a binary to Process.spawn, From c720645723eb582bc62c57196983731c47acd557 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 28 Dec 2022 19:02:51 -0500 Subject: [PATCH 33/53] Rubocop --- .../concurrent_ruby/concurrent_ruby_instrumentation_test.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 642425f3d5..991cd9f20a 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -67,13 +67,15 @@ def test_promises_future_captures_segment_error def test_task_segment_has_correct_parent txn = future_in_transaction { 'hi' } - task_segment = txn.segments.find{|n| n.name == 'Concurrent/Task'} + task_segment = txn.segments.find { |n| n.name == 'Concurrent/Task' } + assert_equal task_segment.parent.name, txn.best_name end def test_segment_not_created_if_tracing_disabled NewRelic::Agent::Tracer.stub :tracing_enabled?, false do txn = future_in_transaction { 'the revolution will not be televised' } + assert_predicate txn.segments, :one? assert_equal txn.segments.first.name, txn.best_name end From 153ef9450dae547ca73e945183d3770a12cba2c8 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 09:26:35 -0500 Subject: [PATCH 34/53] Add test for transaction noticed error --- .../concurrent_ruby_instrumentation_test.rb | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 991cd9f20a..72aaf7acfa 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -17,13 +17,22 @@ def future_in_transaction(&block) end end - # Tests def concurrent_promises_calls_net_http_in_block future_in_transaction { Net::HTTP.get(URI('http://www.example.com')) } end + def simulate_error + future = Concurrent::Promises.future { raise 'hi' } + future.wait! + end + + def assert_segment_noticed_simulated_error + assert_segment_noticed_error txn, /Concurrent\/Task$/, /RuntimeError/, /hi/i + end + + # Tests def test_promises_future_creates_segment_with_default_name - txn = future_in_transaction { 'hi' } + txn = future_in_transaction { 'time keeps on slipping' } expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/Task'] assert_equal(3, txn.segments.length) @@ -55,18 +64,32 @@ def test_promises_future_captures_segment_error # TODO: OLD RUBIES - RUBY_VERSION 2.2 # specific "begin" in block can be removed once we drop support for 2.2 begin - future = Concurrent::Promises.future { raise 'hi' } - future.wait! + simulate_error rescue StandardError => e # NOOP -- allowing span and transaction to notice error end end - assert_segment_noticed_error txn, /Concurrent\/Task/, /RuntimeError/, /hi/i + assert_segment_noticed_simulated_error + end + + def test_noticed_error_at_segment_and_txn_on_error + txn = nil + begin + in_transaction do |test_txn| + txn = test_txn + simulate_error + end + rescue StandardError => e + # NOOP -- allowing span and transaction to notice error + end + + assert_segment_noticed_simulated_error + assert_transaction_noticed_error txn, /RuntimeError/ end def test_task_segment_has_correct_parent - txn = future_in_transaction { 'hi' } + txn = future_in_transaction { 'are you my mother?' } task_segment = txn.segments.find { |n| n.name == 'Concurrent/Task' } assert_equal task_segment.parent.name, txn.best_name From ac5fbaf7282976545ec66411ed30677f58003b35 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 11:06:00 -0500 Subject: [PATCH 35/53] Pass txn to assertion --- .../concurrent_ruby/concurrent_ruby_instrumentation_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 72aaf7acfa..17027dc38a 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -26,7 +26,7 @@ def simulate_error future.wait! end - def assert_segment_noticed_simulated_error + def assert_segment_noticed_simulated_error(txn) assert_segment_noticed_error txn, /Concurrent\/Task$/, /RuntimeError/, /hi/i end @@ -70,7 +70,7 @@ def test_promises_future_captures_segment_error end end - assert_segment_noticed_simulated_error + assert_segment_noticed_simulated_error(txn) end def test_noticed_error_at_segment_and_txn_on_error @@ -84,7 +84,7 @@ def test_noticed_error_at_segment_and_txn_on_error # NOOP -- allowing span and transaction to notice error end - assert_segment_noticed_simulated_error + assert_segment_noticed_simulated_error(txn) assert_transaction_noticed_error txn, /RuntimeError/ end From 24942ce25efb4c4cae510795ae23f6e7b40df31d Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 12:12:35 -0500 Subject: [PATCH 36/53] Add tests for thread_ids_enabled config --- test/new_relic/agent/tracer_test.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/new_relic/agent/tracer_test.rb b/test/new_relic/agent/tracer_test.rb index b02324fd82..fb31a1516a 100644 --- a/test/new_relic/agent/tracer_test.rb +++ b/test/new_relic/agent/tracer_test.rb @@ -367,6 +367,32 @@ def test_current_segment_in_nested_threads_auto end end + def test_thread_ids_included_when_enabled + with_config( + :'instrumentation.thread.tracing' => true, + :'thread_ids_enabled' => true + ) do + txn = in_transaction do + Thread.new { 'woof' }.join + end + + assert_match /Ruby\/Thread\/Thread\d{4}\/Fiber\d{4}/, txn.segments.last.name + end + end + + def test_thread_ids_absent_when_disabled + with_config( + :'instrumentation.thread.tracing' => true, + :'thread_ids_enabled' => false + ) do + txn = in_transaction do + Thread.new { 'woof' }.join + end + + assert_match /Ruby\/Thread$/, txn.segments.last.name + end + end + def test_start_segment name = "Custom/MyClass/myoperation" unscoped_metrics = [ From 68db668c97d054a1942939955c193297e8456506 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 15:03:09 -0500 Subject: [PATCH 37/53] Record only one segment per call Previously, a segment was created when the #post method was called and when the thread within the method invoked the block. The segment created when the #post method was called didn't have a lot of unique value, so we're reporting only the block segment --- .../agent/instrumentation/concurrent_ruby/chain.rb | 2 +- .../concurrent_ruby/instrumentation.rb | 14 ++------------ .../instrumentation/concurrent_ruby/prepend.rb | 2 +- .../concurrent_ruby_instrumentation_test.rb | 11 +++++------ 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 041d785134..75f1dd4404 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -14,7 +14,7 @@ def post(*args, &task) return post_without_new_relic(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled? traced_task = add_task_tracing(*args, &task) - post_with_new_relic(*args) { post_without_new_relic(*args, &traced_task) } + post_without_new_relic(*args, &traced_task) end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index eca2a2db58..10dff31658 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -4,22 +4,12 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby - DEFAULT_NAME = 'Concurrent::ThreadPoolExecutor#post' - TASK_NAME = 'Concurrent/Task' - - def post_with_new_relic(*args) - segment = NewRelic::Agent::Tracer.start_segment(name: DEFAULT_NAME) - begin - yield - ensure - ::NewRelic::Agent::Transaction::Segment.finish(segment) - end - end + SEGMENT_NAME = 'Concurrent/Task' def add_task_tracing(*args, &task) NewRelic::Agent::Tracer.thread_block_with_current_transaction( *args, - segment_name: TASK_NAME, + segment_name: SEGMENT_NAME, parent: NewRelic::Agent::Tracer.current_segment, &task ) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index 2ad191d246..84a70fe09c 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -11,7 +11,7 @@ def post(*args, &task) return super(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled? traced_task = add_task_tracing(*args, &task) - post_with_new_relic(*args) { super(*args, &traced_task) } + super(*args, &traced_task) end end diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 17027dc38a..5c6c5f4dfb 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -4,7 +4,6 @@ class ConcurrentRubyInstrumentationTest < Minitest::Test EXPECTED_SEGMENTS_FOR_NESTED_CALLS = [ - 'Concurrent::ThreadPoolExecutor#post', 'Concurrent/Task', 'External/www.example.com/Net::HTTP/GET' ] @@ -33,17 +32,17 @@ def assert_segment_noticed_simulated_error(txn) # Tests def test_promises_future_creates_segment_with_default_name txn = future_in_transaction { 'time keeps on slipping' } - expected_segments = ['Concurrent::ThreadPoolExecutor#post', 'Concurrent/Task'] + expected_segment = 'Concurrent/Task' - assert_equal(3, txn.segments.length) - assert expected_segments.to_set.subset?(txn.segments.map { |s| s.name }.to_set) + assert_equal(2, txn.segments.length) + assert txn.segments.map(&:name).include?(expected_segment) end def test_promises_future_creates_segments_for_nested_instrumented_calls with_config(:'instrumentation.thread.tracing' => false) do txn = concurrent_promises_calls_net_http_in_block - assert_equal(4, txn.segments.length) + assert_equal(3, txn.segments.length) assert EXPECTED_SEGMENTS_FOR_NESTED_CALLS.to_set.subset?(txn.segments.map { |s| s.name }.to_set) end end @@ -66,7 +65,7 @@ def test_promises_future_captures_segment_error begin simulate_error rescue StandardError => e - # NOOP -- allowing span and transaction to notice error + # NOOP -- allowing span to notice error end end From e776ea7214643fe5f4b18b574f11aa98229421ad Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 15:03:48 -0500 Subject: [PATCH 38/53] Record supportability metric on first invocation --- .../concurrent_ruby/instrumentation.rb | 3 +++ .../concurrent_ruby_instrumentation_test.rb | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb index 10dff31658..1854aa20d8 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/instrumentation.rb @@ -5,8 +5,11 @@ module NewRelic::Agent::Instrumentation module ConcurrentRuby SEGMENT_NAME = 'Concurrent/Task' + SUPPORTABILITY_METRIC = 'Supportability/ConcurrentRuby/Invoked' def add_task_tracing(*args, &task) + NewRelic::Agent.record_metric_once(SUPPORTABILITY_METRIC) + NewRelic::Agent::Tracer.thread_block_with_current_transaction( *args, segment_name: SEGMENT_NAME, diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index 5c6c5f4dfb..ceb79a8ca8 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -102,4 +102,16 @@ def test_segment_not_created_if_tracing_disabled assert_equal txn.segments.first.name, txn.best_name end end + + def test_supportability_metric_recorded_once + in_transaction do + Concurrent::Promises.future { 'one-banana' } + end + + in_transaction do + Concurrent::Promises.future { 'two-banana' } + end + + assert_metrics_recorded(NewRelic::Agent::Instrumentation::ConcurrentRuby::SUPPORTABILITY_METRIC) + end end From c7091f399173c3fb77f8bc070db941968a1b08d8 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 15:09:56 -0500 Subject: [PATCH 39/53] Add CHANGELOG entry --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4960fbf17a..8aad1fcad0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ Following the 3.2.0 release of Ruby, the New Relic Ruby Agent has confirmed compatibility with and now supports the official release of Ruby 3.2.0. [PR#1715](https://github.com/newrelic/newrelic-ruby-agent/pull/1715) +- **Add instrumentation for concurrent-ruby** + + Instrumentation for the [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby) gem has been added to the agent. When a transaction is already in progress and a call to a `Concurrent::` method that routes through `Concurrent::ThreadPoolExecutor#post` is made, a segment will be added to the transaction. Any content within the block passed to the `Concurrent::` method that is instrumented by the agent, such as a call to `Net::HTTP.get`, will have a nested segment created. [PR#1682] + + | Configuration name | Default | Behavior | + | --------------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------- | + | `instrumentation.concurrent_ruby` | auto | Controls auto-instrumentation of the elasticsearch library at start up. May be one of `auto`, `prepend`, `chain`, `disabled`. | ## v8.14.0 From 6ff329b7c51e1f7defdd4fa0aad9cc2a34458055 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 17:36:25 -0500 Subject: [PATCH 40/53] Remove guard clause --- lib/new_relic/agent/tracer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index 49358b0950..7a58c8e9ad 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -418,7 +418,7 @@ def thread_block_with_current_transaction(*args, segment_name:, parent:, &block) segment_name += "/Thread#{::Thread.current.object_id}/Fiber#{::Fiber.current.object_id}" if NewRelic::Agent.config[:'thread_ids_enabled'] segment = NewRelic::Agent::Tracer.start_segment(name: segment_name, parent: parent) NewRelic::Agent::Tracer.capture_segment_error(segment) do - yield(*args) if block.respond_to?(:call) + yield(*args) end end ensure From 4bcfb430e4ad5869ece64d90d6ea2e0ca9b43b8f Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 17:46:47 -0500 Subject: [PATCH 41/53] Replace assertion --- .../concurrent_ruby/concurrent_ruby_instrumentation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index ceb79a8ca8..bbb148c159 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -35,7 +35,7 @@ def test_promises_future_creates_segment_with_default_name expected_segment = 'Concurrent/Task' assert_equal(2, txn.segments.length) - assert txn.segments.map(&:name).include?(expected_segment) + assert_includes txn.segments.map(&:name), expected_segment end def test_promises_future_creates_segments_for_nested_instrumented_calls From 226d3f9d3489dcf02506cc7386836bac5cb1d705 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 29 Dec 2022 18:01:46 -0500 Subject: [PATCH 42/53] Make the match condition more flexible The CI had a Thread ID that was five digits --- test/new_relic/agent/tracer_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/new_relic/agent/tracer_test.rb b/test/new_relic/agent/tracer_test.rb index fb31a1516a..8718be2b50 100644 --- a/test/new_relic/agent/tracer_test.rb +++ b/test/new_relic/agent/tracer_test.rb @@ -376,7 +376,7 @@ def test_thread_ids_included_when_enabled Thread.new { 'woof' }.join end - assert_match /Ruby\/Thread\/Thread\d{4}\/Fiber\d{4}/, txn.segments.last.name + assert_match /Ruby\/Thread\/Thread\d+\/Fiber\d+/, txn.segments.last.name end end From 654470cac5f34726496960dea5f4254545657cbc Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Tue, 3 Jan 2023 10:20:07 -0800 Subject: [PATCH 43/53] Require 'fiber' No method error raised for Ruby 2.2.10 and JRuby tests for Fiber.current The 2.2.10 docs say require 'fiber' must be in the file to access this method --- lib/new_relic/agent/tracer.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index 7a58c8e9ad..c3f71db963 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -2,6 +2,7 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. # frozen_string_literal: true +require 'fiber' require 'new_relic/agent/transaction' require 'new_relic/agent/transaction/segment' require 'new_relic/agent/transaction/datastore_segment' From 1ce60a6a83658872d04842f06bb120de24adcb96 Mon Sep 17 00:00:00 2001 From: "Kayla Reopelle (she/her)" <87386821+kaylareopelle@users.noreply.github.com> Date: Tue, 3 Jan 2023 12:39:57 -0800 Subject: [PATCH 44/53] Replace `elasticsearch` with `concurrent-ruby` Co-authored-by: James Bunch --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8aad1fcad0..fa0c5bae8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ | Configuration name | Default | Behavior | | --------------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------- | - | `instrumentation.concurrent_ruby` | auto | Controls auto-instrumentation of the elasticsearch library at start up. May be one of `auto`, `prepend`, `chain`, `disabled`. | + | `instrumentation.concurrent_ruby` | auto | Controls auto-instrumentation of the concurrent-ruby library at start up. May be one of `auto`, `prepend`, `chain`, `disabled`. | ## v8.14.0 From 90ff58e56c77d2e387b664e6caffeea0396ccb51 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Tue, 3 Jan 2023 13:07:37 -0800 Subject: [PATCH 45/53] Return unless args.last is Exception --- lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb | 2 +- lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb index 75f1dd4404..c41b264992 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb @@ -26,7 +26,7 @@ def post(*args, &task) # Uses args.last to record the error becuase the methods that this will monkey patch # look like: initialize(reason) & initialize(value, reason) def initialize(*args) - NewRelic::Agent.notice_error(args.last) + NewRelic::Agent.notice_error(args.last) if args.last.is_a?(Exception) initialize_without_new_relic(*args) end end diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb index 84a70fe09c..15e56bb201 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby/prepend.rb @@ -19,7 +19,7 @@ module ErrorPrepend # Uses args.last to record the error because the methods that this will be prepended to # look like: initialize(reason) & initialize(value, reason) def initialize(*args) - NewRelic::Agent.notice_error(args.last) + NewRelic::Agent.notice_error(args.last) if args.last.is_a?(Exception) super end end From 22fb1ea1589a8d9687daa6a521adb6a6b8025d5f Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Tue, 3 Jan 2023 13:16:15 -0800 Subject: [PATCH 46/53] Refactor segments within transaction --- .../concurrent_ruby_instrumentation_test.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb index bbb148c159..432cbab38d 100644 --- a/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb +++ b/test/multiverse/suites/concurrent_ruby/concurrent_ruby_instrumentation_test.rb @@ -29,6 +29,10 @@ def assert_segment_noticed_simulated_error(txn) assert_segment_noticed_error txn, /Concurrent\/Task$/, /RuntimeError/, /hi/i end + def assert_expected_segments_in_transaction(txn) + assert_predicate (txn.segments.map(&:name) & EXPECTED_SEGMENTS_FOR_NESTED_CALLS), :any? + end + # Tests def test_promises_future_creates_segment_with_default_name txn = future_in_transaction { 'time keeps on slipping' } @@ -43,7 +47,7 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls txn = concurrent_promises_calls_net_http_in_block assert_equal(3, txn.segments.length) - assert EXPECTED_SEGMENTS_FOR_NESTED_CALLS.to_set.subset?(txn.segments.map { |s| s.name }.to_set) + assert_expected_segments_in_transaction(txn) end end @@ -51,10 +55,10 @@ def test_promises_future_creates_segments_for_nested_instrumented_calls_with_thr with_config(:'instrumentation.thread.tracing' => true) do txn = concurrent_promises_calls_net_http_in_block - # We can't check the number of segments when thread tracing is enabled because we can not rely on concurrent - # ruby creating threads during this transaction, as it can reuse threads that were created previously. + # We can't check the number of segments when thread tracing is enabled because we cannot rely on concurrent-ruby + # creating threads during this transaction, as it can reuse threads that were created previously. # Instead, we check to make sure the segments that should be present are. - assert EXPECTED_SEGMENTS_FOR_NESTED_CALLS.to_set.subset?(txn.segments.map { |s| s.name }.to_set) + assert_expected_segments_in_transaction(txn) end end From 4c97f2b2d4edf6402e64b8f9b7ff3c0ab59ffef6 Mon Sep 17 00:00:00 2001 From: hramadan Date: Wed, 4 Jan 2023 13:41:04 -0800 Subject: [PATCH 47/53] Capture segment errors outside current_txn --- lib/new_relic/agent/tracer.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index c3f71db963..11d18cd810 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -418,9 +418,9 @@ def thread_block_with_current_transaction(*args, segment_name:, parent:, &block) NewRelic::Agent::Tracer.state.current_transaction = current_txn segment_name += "/Thread#{::Thread.current.object_id}/Fiber#{::Fiber.current.object_id}" if NewRelic::Agent.config[:'thread_ids_enabled'] segment = NewRelic::Agent::Tracer.start_segment(name: segment_name, parent: parent) - NewRelic::Agent::Tracer.capture_segment_error(segment) do - yield(*args) - end + end + NewRelic::Agent::Tracer.capture_segment_error(segment) do + yield(*args) end ensure ::NewRelic::Agent::Transaction::Segment.finish(segment) From b3d4531fb253c4432751202958d05d7f980ee6db Mon Sep 17 00:00:00 2001 From: hramadan Date: Wed, 4 Jan 2023 13:41:38 -0800 Subject: [PATCH 48/53] Small typo fix A 12-year old typo! :D --- .../agent/instrumentation/controller_instrumentation.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/new_relic/agent/instrumentation/controller_instrumentation.rb b/lib/new_relic/agent/instrumentation/controller_instrumentation.rb index c16ff5fc6c..9c14c17d1d 100644 --- a/lib/new_relic/agent/instrumentation/controller_instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/controller_instrumentation.rb @@ -340,7 +340,7 @@ def self.class_name(traced_obj, options = {}) # # Seldomly used options: # - # * :class_name => aClass.name is used to override the name + # * :class_name => Class.name is used to override the name # of the class when used inside the metric name. Default is the # current class. # * :path => metric_path is *deprecated* in the public API. It From 654965ba24f383b6b240354b4c7c6c3d0d12ec22 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 4 Jan 2023 14:20:36 -0800 Subject: [PATCH 49/53] Lock support to 1.1.5 and above --- CHANGELOG.md | 2 +- lib/new_relic/agent/instrumentation/concurrent_ruby.rb | 3 ++- test/multiverse/suites/concurrent_ruby/Envfile | 10 +++++++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa0c5bae8e..8bee0e2193 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - **Add instrumentation for concurrent-ruby** - Instrumentation for the [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby) gem has been added to the agent. When a transaction is already in progress and a call to a `Concurrent::` method that routes through `Concurrent::ThreadPoolExecutor#post` is made, a segment will be added to the transaction. Any content within the block passed to the `Concurrent::` method that is instrumented by the agent, such as a call to `Net::HTTP.get`, will have a nested segment created. [PR#1682] + Instrumentation for the [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby) gem has been added to the agent. for versions 1.1.5 and above. When a transaction is already in progress and a call to a `Concurrent::` method that routes through `Concurrent::ThreadPoolExecutor#post` is made, a segment will be added to the transaction. Any content within the block passed to the `Concurrent::` method that is instrumented by the agent, such as a call to `Net::HTTP.get`, will have a nested segment created. [PR#1682] | Configuration name | Default | Behavior | | --------------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------- | diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb index ab525775a8..00ff47b5e8 100644 --- a/lib/new_relic/agent/instrumentation/concurrent_ruby.rb +++ b/lib/new_relic/agent/instrumentation/concurrent_ruby.rb @@ -10,7 +10,8 @@ named :'concurrent_ruby' depends_on do - defined?(Concurrent) + defined?(Concurrent) && + Gem::Version.new(Concurrent::VERSION) >= Gem::Version.new('1.1.5') end executes do diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile index 40d0658f94..1c53ec43d7 100644 --- a/test/multiverse/suites/concurrent_ruby/Envfile +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -4,6 +4,14 @@ instrumentation_methods :chain, :prepend +# The 1.1.x series of Concurrent Ruby starts to use the error classes +# we reference in our logic to notice errors after 1.1.5 +# 1.1.4 and below do not use these classes. +CONCURRENT_RUBY_VERSIONS = [ + [nil, 2.2], + ['1.1.5', 2.2] +] + gemfile <<-RB - gem 'concurrent-ruby' + gem 'concurrent-ruby', '1.1.5' RB From 428e568487bbbf1c168e497624b3453462c9359a Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Wed, 4 Jan 2023 14:28:56 -0800 Subject: [PATCH 50/53] Make parent an optional kwarg This also removes the inclusion of parent in the thread_block_with_current_transaction calls made in Thread and TracedThread instrumentation --- lib/new_relic/agent/instrumentation/thread/instrumentation.rb | 1 - lib/new_relic/agent/tracer.rb | 2 +- lib/new_relic/traced_thread.rb | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb index a9643b240f..f85f816c64 100644 --- a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb +++ b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb @@ -19,7 +19,6 @@ def add_thread_tracing(*args, &block) NewRelic::Agent::Tracer.thread_block_with_current_transaction( *args, segment_name: 'Ruby/Thread', - parent: NewRelic::Agent::Tracer.current_segment, &block ) end diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb index 11d18cd810..1fbe5d8c47 100644 --- a/lib/new_relic/agent/tracer.rb +++ b/lib/new_relic/agent/tracer.rb @@ -410,7 +410,7 @@ def clear_state alias_method :tl_clear, :clear_state - def thread_block_with_current_transaction(*args, segment_name:, parent:, &block) + def thread_block_with_current_transaction(*args, segment_name:, parent: nil, &block) current_txn = ::Thread.current[:newrelic_tracer_state].current_transaction if ::Thread.current[:newrelic_tracer_state] && ::Thread.current[:newrelic_tracer_state].is_execution_traced? proc do begin diff --git a/lib/new_relic/traced_thread.rb b/lib/new_relic/traced_thread.rb index bd2b4c5524..e8aaec19bd 100644 --- a/lib/new_relic/traced_thread.rb +++ b/lib/new_relic/traced_thread.rb @@ -32,7 +32,6 @@ def create_traced_block(*args, &block) NewRelic::Agent::Tracer.thread_block_with_current_transaction( *args, segment_name: 'Ruby/TracedThread', - parent: NewRelic::Agent::Tracer.current_segment, &block ) end From 1822b468cc85dbbd4b25048826a56b6a1374af46 Mon Sep 17 00:00:00 2001 From: "Kayla Reopelle (she/her)" <87386821+kaylareopelle@users.noreply.github.com> Date: Wed, 4 Jan 2023 15:49:09 -0800 Subject: [PATCH 51/53] Update CHANGELOG.md Co-authored-by: James Bunch --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 638cbaa442..049a73cffc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - **Add instrumentation for concurrent-ruby** - Instrumentation for the [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby) gem has been added to the agent. for versions 1.1.5 and above. When a transaction is already in progress and a call to a `Concurrent::` method that routes through `Concurrent::ThreadPoolExecutor#post` is made, a segment will be added to the transaction. Any content within the block passed to the `Concurrent::` method that is instrumented by the agent, such as a call to `Net::HTTP.get`, will have a nested segment created. [PR#1682] + Instrumentation for the [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby) gem has been added to the agent. for versions 1.1.5 and above. When a transaction is already in progress and a call to a `Concurrent::` method that routes through `Concurrent::ThreadPoolExecutor#post` is made, a segment will be added to the transaction. Any content within the block passed to the `Concurrent::` method that is instrumented by the agent, such as a call to `Net::HTTP.get`, will have a nested segment created. [PR#1682](https://github.com/newrelic/newrelic-ruby-agent/pull/1682) | Configuration name | Default | Behavior | | --------------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------- | From 2bd96205ac095339b092ed556607f808ae2c49df Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 5 Jan 2023 10:32:15 -0800 Subject: [PATCH 52/53] Update Envfile to call create_gemfiles --- test/multiverse/suites/concurrent_ruby/Envfile | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile index 1c53ec43d7..fccc3363fa 100644 --- a/test/multiverse/suites/concurrent_ruby/Envfile +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -12,6 +12,10 @@ CONCURRENT_RUBY_VERSIONS = [ ['1.1.5', 2.2] ] -gemfile <<-RB - gem 'concurrent-ruby', '1.1.5' -RB +def gem_list(concurrent_version = nil) + <<-RB + gem 'concurrent-ruby'#{concurrent_ruby} + RB +end + +create_gemfiles(CONCURRENT_RUBY_VERSIONS, gem_list) From 1775b5740ab9182101f878f88344cde9c288d8f3 Mon Sep 17 00:00:00 2001 From: Kayla Reopelle Date: Thu, 5 Jan 2023 11:02:48 -0800 Subject: [PATCH 53/53] Fix variable name --- test/multiverse/suites/concurrent_ruby/Envfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/multiverse/suites/concurrent_ruby/Envfile b/test/multiverse/suites/concurrent_ruby/Envfile index fccc3363fa..c564663a44 100644 --- a/test/multiverse/suites/concurrent_ruby/Envfile +++ b/test/multiverse/suites/concurrent_ruby/Envfile @@ -14,7 +14,7 @@ CONCURRENT_RUBY_VERSIONS = [ def gem_list(concurrent_version = nil) <<-RB - gem 'concurrent-ruby'#{concurrent_ruby} + gem 'concurrent-ruby'#{concurrent_version} RB end