Remove Ruby/Thread and Ruby/Fiber spans

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## v9.7.0
 
-Version 9.7.0 changes the endpoint used to access the cluster name for Elasticsearch instrumentation and adds support for Falcon.
+Version 9.7.0 changes the endpoint used to access the cluster name for Elasticsearch instrumentation, adds support for Falcon, and removes the creation of the Ruby/Thread and Ruby/Fiber spans.
 
 - **Feature: Use root path to access Elasticsearch cluster name**
 
@@ -22,6 +22,10 @@ Version 9.7.0 changes the endpoint used to access the cluster name for Elasticse
 
   The agent now supports the web server [Falcon](https://socketry.github.io/falcon/). [PR#2383](https://github.com/newrelic/newrelic-ruby-agent/pull/2383)
 
+- **Feature: Remove spans with name Ruby/Thread and Ruby/Fiber**
+
+  Due to the lack of helpful information and the confusion commonly caused by the spans named Ruby/Thread and Ruby/Fiber, these spans have been removed. However, the agents ability to monitor instrumented code running in a thread or fiber will remain unchanged. [PR#2389](https://github.com/newrelic/newrelic-ruby-agent/pull/2389)
+
 ## v9.6.0
 
 Version 9.6.0 adds instrumentation for Async::HTTP, Ethon, and HTTPX, adds the ability to ignore specific routes with Roda, gleans Docker container IDs from cgroups v2-based containers, records additional synthetics attributes, fixes an issue with Rails 7.1 that could cause duplicate log records to be sent to New Relic, fixes a deprecation warning for the Sidekiq error handler, adds additional attributes for OpenTelemetry compatibility, and resolves some technical debt, thanks to the community.

lib/new_relic/agent/instrumentation/fiber/instrumentation.rb

@@ -14,10 +14,7 @@ def initialize_with_newrelic_tracing
     def add_thread_tracing(&block)
       return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?
 
-      NewRelic::Agent::Tracer.thread_block_with_current_transaction(
-        segment_name: 'Ruby/Fiber',
-        &block
-      )
+      NewRelic::Agent::Tracer.thread_block_with_current_transaction(&block)
     end
   end
 end

lib/new_relic/agent/instrumentation/thread/instrumentation.rb

@@ -16,10 +16,7 @@ def initialize_with_newrelic_tracing
         def add_thread_tracing(*args, &block)
           return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?
 
-          NewRelic::Agent::Tracer.thread_block_with_current_transaction(
-            segment_name: 'Ruby/Thread',
-            &block
-          )
+          NewRelic::Agent::Tracer.thread_block_with_current_transaction(&block)
         end
       end
     end

lib/new_relic/agent/tracer.rb

@@ -418,16 +418,17 @@ def thread_tracing_enabled?
           NewRelic::Agent.config[:'instrumentation.thread.tracing']
         end
 
-        def thread_block_with_current_transaction(segment_name:, parent: nil, &block)
+        def thread_block_with_current_transaction(segment_name: nil, 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 |*args|
             begin
               if current_txn && !current_txn.finished?
                 NewRelic::Agent::Tracer.state.current_transaction = current_txn
+                ::Thread.current[:newrelic_thread_span_parent] = parent
                 current_txn.async = true
-                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)
+                segment_name = "#{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) if segment_name
               end
               NewRelic::Agent::Tracer.capture_segment_error(segment) do
                 yield(*args)

lib/new_relic/agent/transaction/tracing.rb

@@ -22,7 +22,7 @@ def total_time
 
         def add_segment(segment, parent = nil)
           segment.transaction = self
-          segment.parent = parent || current_segment
+          segment.parent = parent || thread_starting_span || current_segment
           set_current_segment(segment)
           if @segments.length < segment_limit
             @segments << segment
@@ -39,6 +39,16 @@ def add_segment(segment, parent = nil)
           segment.transaction_assigned
         end
 
+        def thread_starting_span
+          # if the previous current segment was in another thread, use the thread local parent
+          if ::Thread.current[:newrelic_thread_span_parent] &&
+              current_segment &&
+              current_segment.starting_segment_key != NewRelic::Agent::Tracer.current_segment_key
+
+            ::Thread.current[:newrelic_thread_span_parent]
+          end
+        end
+
         def segment_complete(segment)
           # if parent was in another thread, remove the current_segment entry for this thread
           if segment.parent && segment.parent.starting_segment_key != NewRelic::Agent::Tracer.current_segment_key

test/multiverse/suites/thread/thread_fiber_instrumentation_test.rb

@@ -33,29 +33,30 @@ def run_nested_parent_test(async_class1, async_class2 = nil)
     async_class2 ||= async_class1
 
     in_transaction do |txn|
+      parent_segment = NewRelic::Agent::Tracer.current_segment
       do_segment(name: 'Outer') do |outer_segment|
         async1 = async_class1.new do
-          fiber_segment = NewRelic::Agent::Tracer.current_segment
-
-          assert_parent outer_segment, fiber_segment
+          assert_parent parent_segment, outer_segment
           async2 = nil
           do_segment(name: 'Inner') do |inner_segment|
-            assert_parent fiber_segment, inner_segment
+            assert_parent outer_segment, inner_segment
             async2 = async_class2.new do
-              assert_parent inner_segment, NewRelic::Agent::Tracer.current_segment
+              do_segment(name: 'InnerInner') do |inner_inner_segment|
+                assert_parent inner_segment, inner_inner_segment
+              end
             end
           end
 
           do_segment(name: 'Inner2') do |inner_2_segment|
-            assert_parent fiber_segment, inner_2_segment
+            assert_parent outer_segment, inner_2_segment
           end
           run_or_wait(async2)
         end
         run_or_wait(async1)
       end
     end
 
-    assert_equal 6, harvest_span_events![0][:events_seen]
+    assert_equal 5, harvest_span_events![0][:events_seen]
   end
 
   def test_parents_thread_thread

test/new_relic/agent/tracer_test.rb

@@ -383,7 +383,6 @@ def test_current_segment_in_nested_threads_auto
           threads.each(&:join)
           txn.finish
 
-          assert_equal 2, txn.segments.count { |s| s.name == 'Ruby/Thread' }
           assert_nil Tracer.current_segment
         end
       end
@@ -419,11 +418,11 @@ def test_thread_ids_included_when_enabled
             Thread.new { 'woof' }.join
           end
 
-          assert_match %r{Ruby/Thread/Thread\d+/Fiber\d+}, txn.segments.last.name
+          assert_match %r{/Thread\d+/Fiber\d+}, txn.segments.last.name
         end
       end
 
-      def test_thread_ids_absent_when_disabled
+      def test_thread_spans_absent_when_ids_disabled
         with_config(
           :'instrumentation.thread.tracing' => true,
           :'thread_ids_enabled' => false
@@ -432,7 +431,7 @@ def test_thread_ids_absent_when_disabled
             Thread.new { 'woof' }.join
           end
 
-          assert_match %r{Ruby/Thread$}, txn.segments.last.name
+          assert_equal 1, txn.segments.count
         end
       end