Merge pull request #2083 from newrelic/2062_fiber_instrumentation_ini…

…tialize Fix for compatibility issue with Async 2.6.2
newrelic · Jun 16, 2023 · ba5a5ae · ba5a5ae
2 parents bc0b619 + 3a6f817
commit ba5a5ae
Show file tree

Hide file tree

Showing 14 changed files with 50 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ## dev
 
-Version <dev> of the agent adds the ability to filter logs by level, expands instrumentation for Action Cable,provides bugfix for Code-Level Metrics, and a bugfix for `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` being incorrectly private.
+Version <dev> of the agent adds the ability to filter logs by level, expands instrumentation for Action Cable,provides bugfix for Code-Level Metrics, a bugfix for how `Fiber` args are treated, and a bugfix for `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` being incorrectly private.
 
 - **Feature: Filter forwarded logs based on level**
 
@@ -28,6 +28,10 @@ Version <dev> of the agent adds the ability to filter logs by level, expands ins
 
   As part of a refactor included in a previous release of the agent, the method `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` was incorrectly made private. This method is now public again. Thanks to [@dark-panda](https://github.com/dark-panda) for reporting this issue. [PR#](https://github.com/newrelic/newrelic-ruby-agent/pull/2078)
 
+- **Bugfix: Fix the way args are handled for Fibers**
+
+  Previously, the agent treated Fiber args the same as it was treating Thread args, which is not correct. Args are passed to `Fiber#resume`, and not `Fiber.new`. This has been fixed, and the agent will properly preserve args for both Fiber and Thread classes. This also caused an error to occur when using Async 2.6.2, due to mismatching initalize definitions for Fiber prepended modules. This has been fixed as well. Thanks to [@travisbell](https://github.com/travisbell) for bringing this to our attention. [PR#2083](https://github.com/newrelic/newrelic-ruby-agent/pull/2083)
+
 ## v9.2.2
 
   Version 9.2.2 of the agent fixes a bug with the `Transaction#finished?` method.

diff --git a/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb b/lib/new_relic/agent/instrumentation/concurrent_ruby/chain.rb
@@ -13,7 +13,7 @@ def self.instrument!
         def post(*args, &task)
           return post_without_new_relic(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled?
 
-          traced_task = add_task_tracing(*args, &task)
+          traced_task = add_task_tracing(&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
@@ -7,11 +7,10 @@ module ConcurrentRuby
     SEGMENT_NAME = 'Concurrent/Task'
     SUPPORTABILITY_METRIC = 'Supportability/ConcurrentRuby/Invoked'
 
-    def add_task_tracing(*args, &task)
+    def add_task_tracing(&task)
       NewRelic::Agent.record_metric_once(SUPPORTABILITY_METRIC)
 
       NewRelic::Agent::Tracer.thread_block_with_current_transaction(
-        *args,
         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
@@ -10,7 +10,7 @@ module Prepend
       def post(*args, &task)
         return super(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled?
 
-        traced_task = add_task_tracing(*args, &task)
+        traced_task = add_task_tracing(&task)
         super(*args, &traced_task)
       end
     end

diff --git a/lib/new_relic/agent/instrumentation/fiber/chain.rb b/lib/new_relic/agent/instrumentation/fiber/chain.rb
@@ -10,9 +10,16 @@ def self.instrument!
 
         alias_method(:initialize_without_new_relic, :initialize)
 
-        def initialize(*args, &block)
-          traced_block = add_thread_tracing(*args, &block)
-          initialize_with_newrelic_tracing { initialize_without_new_relic(*args, &traced_block) }
+        if RUBY_VERSION < '2.7.0'
+          def initialize(*_args, &block)
+            traced_block = add_thread_tracing(&block)
+            initialize_with_newrelic_tracing { initialize_without_new_relic(&traced_block) }
+          end
+        else
+          def initialize(**kwargs, &block)
+            traced_block = add_thread_tracing(&block)
+            initialize_with_newrelic_tracing { initialize_without_new_relic(**kwargs, &traced_block) }
+          end
         end
       end
     end

diff --git a/lib/new_relic/agent/instrumentation/fiber/instrumentation.rb b/lib/new_relic/agent/instrumentation/fiber/instrumentation.rb
@@ -11,11 +11,10 @@ def initialize_with_newrelic_tracing
       yield
     end
 
-    def add_thread_tracing(*args, &block)
+    def add_thread_tracing(&block)
       return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?
 
       NewRelic::Agent::Tracer.thread_block_with_current_transaction(
-        *args,
         segment_name: 'Ruby/Fiber',
         &block
       )

diff --git a/lib/new_relic/agent/instrumentation/fiber/prepend.rb b/lib/new_relic/agent/instrumentation/fiber/prepend.rb
@@ -9,9 +9,16 @@ module MonitoredFiber
     module Prepend
       include NewRelic::Agent::Instrumentation::MonitoredFiber
 
-      def initialize(*args, &block)
-        traced_block = add_thread_tracing(*args, &block)
-        initialize_with_newrelic_tracing { super(*args, &traced_block) }
+      if RUBY_VERSION < '2.7.0'
+        def initialize(*_args, &block)
+          traced_block = add_thread_tracing(&block)
+          initialize_with_newrelic_tracing { super(&traced_block) }
+        end
+      else
+        def initialize(**kawrgs, &block)
+          traced_block = add_thread_tracing(&block)
+          initialize_with_newrelic_tracing { super(**kawrgs, &traced_block) }
+        end
       end
     end
   end

diff --git a/lib/new_relic/agent/instrumentation/thread/chain.rb b/lib/new_relic/agent/instrumentation/thread/chain.rb
@@ -14,7 +14,7 @@ def self.instrument!
           alias_method(:initialize_without_new_relic, :initialize)
 
           def initialize(*args, &block)
-            traced_block = add_thread_tracing(*args, &block)
+            traced_block = add_thread_tracing(&block)
             initialize_with_newrelic_tracing { initialize_without_new_relic(*args, &traced_block) }
           end
         end

diff --git a/lib/new_relic/agent/instrumentation/thread/instrumentation.rb b/lib/new_relic/agent/instrumentation/thread/instrumentation.rb
@@ -17,7 +17,6 @@ def add_thread_tracing(*args, &block)
           return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?
 
           NewRelic::Agent::Tracer.thread_block_with_current_transaction(
-            *args,
             segment_name: 'Ruby/Thread',
             &block
           )

diff --git a/lib/new_relic/agent/instrumentation/thread/prepend.rb b/lib/new_relic/agent/instrumentation/thread/prepend.rb
@@ -12,7 +12,7 @@ module Prepend
           include NewRelic::Agent::Instrumentation::MonitoredThread
 
           def initialize(*args, &block)
-            traced_block = add_thread_tracing(*args, &block)
+            traced_block = add_thread_tracing(&block)
             initialize_with_newrelic_tracing { super(*args, &traced_block) }
           end
         end

diff --git a/lib/new_relic/agent/tracer.rb b/lib/new_relic/agent/tracer.rb
@@ -419,10 +419,10 @@ def thread_tracing_enabled?
           NewRelic::Agent.config[:'instrumentation.thread.tracing']
         end
 
-        def thread_block_with_current_transaction(*args, segment_name:, parent: nil, &block)
+        def thread_block_with_current_transaction(segment_name:, parent: nil, &block)
           parent ||= current_segment
           current_txn = ::Thread.current[:newrelic_tracer_state]&.current_transaction if ::Thread.current[:newrelic_tracer_state]&.is_execution_traced?
-          proc do
+          proc do |*args|
             begin
               if current_txn && !current_txn.finished?
                 NewRelic::Agent::Tracer.state.current_transaction = current_txn

diff --git a/lib/new_relic/traced_thread.rb b/lib/new_relic/traced_thread.rb
@@ -22,15 +22,14 @@ class TracedThread < Thread
     # @api public
     def initialize(*args, &block)
       NewRelic::Agent.record_api_supportability_metric(:traced_thread)
-      traced_block = create_traced_block(*args, &block)
+      traced_block = create_traced_block(&block)
       super(*args, &traced_block)
     end
 
-    def create_traced_block(*args, &block)
+    def create_traced_block(&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
       )

diff --git a/test/multiverse/suites/thread/Envfile b/test/multiverse/suites/thread/Envfile
@@ -4,4 +4,6 @@
 
 instrumentation_methods :chain, :prepend
 
-gemfile nil
+gemfile <<~RB
+  # this can't be empty or the tests won't run
+RB
diff --git a/test/multiverse/suites/thread/thread_fiber_instrumentation_test.rb b/test/multiverse/suites/thread/thread_fiber_instrumentation_test.rb
@@ -73,4 +73,16 @@ def test_parents_thread_fiber
   def test_parents_fiber_thread
     run_nested_parent_test(Fiber, Thread)
   end
+
+  def test_thread_instrumentation_args_preserved
+    Thread.new('arg 1', 2) do |arg1, arg2|
+      assert_equal ['arg 1', 2], [arg1, arg2]
+    end.join
+  end
+
+  def test_fiber_instrumentation_args_preserved
+    Fiber.new do |arg1, arg2|
+      assert_equal ['arg 1', 2], [arg1, arg2]
+    end.resume('arg 1', 2)
+  end
 end