DEBUG-2334 Use :return/:b_return trace points to target 'end' line of…

… methods and blocks with line probes (#4109)

Adds return and b_return trace point types to the line trace point, permitting targeting end lines of blocks and methods with line probes.

The return trace point types have the unfortunate side effect of always installing even if the target line has no executable ruby code (e.g. a comment line). This means users will not be getting notified when they try to instrument non-executable code lines. I think it is worthwhile to permit instrumenting end lines even if it means no diagnostics when trying to instrument non-instrumentable lines, in other words, it is better to make more use cases work at the expense of worse diagnostics when something that wouldn't work is attempted.

lib/datadog/di/instrumenter.rb

@@ -222,7 +222,16 @@ def hook_line(probe, &block)
         # overhead of targeted trace points is minimal, don't worry about
         # this optimization just yet and create a trace point for each probe.
 
-        tp = TracePoint.new(:line) do |tp|
+        types = if iseq
+          # When targeting trace points we can target the 'end' line of a method.
+          # However, by adding the :return trace point we lose diagnostics
+          # for lines that contain no executable code (e.g. comments only)
+          # and thus cannot actually be instrumented.
+          [:line, :return, :b_return]
+        else
+          [:line]
+        end
+        tp = TracePoint.new(*types) do |tp|
           begin
             # If trace point is not targeted, we must verify that the invocation
             # is the file & line that we want, because untargeted trace points
@@ -240,7 +249,7 @@ def hook_line(probe, &block)
             # TODO test this path
           end
         rescue => exc
-          raise if settings.dynamic_instrumentation.propagate_all_exceptions
+          raise if settings.dynamic_instrumentation.internal.propagate_all_exceptions
           logger.warn("Unhandled exception in line trace point: #{exc.class}: #{exc}")
           telemetry&.report(exc, description: "Unhandled exception in line trace point")
           # TODO test this path

spec/datadog/di/integration/instrumentation_integration_test_class.rb

@@ -8,5 +8,24 @@ def test_method
     # padding
     # padding
     a * 2 # line 10
+  end # line 11
+
+  def test_method_with_block
+    array = [1]
+    array.each do |value|
+      value_copy = value
+    end # line 17
   end
-end
+
+  def test_method_with_conditional
+    if false
+      a = 1
+    else # line 23
+      a = 2
+    end # line 25
+    a
+  end
+
+end # line 29
+
+# Comment - line 31

spec/datadog/di/integration/instrumentation_spec.rb

@@ -296,48 +296,168 @@ def run_test
           load File.join(File.dirname(__FILE__), 'instrumentation_integration_test_class.rb')
         end
 
-        it 'invokes probe' do
-          expect(component.transport).to receive(:send_request).at_least(:once)
-          probe_manager.add_probe(probe)
-          component.probe_notifier_worker.flush
-          expect(probe_manager.installed_probes.length).to eq 1
-          expect(component.probe_notifier_worker).to receive(:add_snapshot).once.and_call_original
-          expect(InstrumentationIntegrationTestClass.new.test_method).to eq(42)
+        shared_examples 'simple log probe' do
+          it 'invokes probe' do
+            expect(component.transport).to receive(:send_request).at_least(:once)
+            probe_manager.add_probe(probe)
+            component.probe_notifier_worker.flush
+            expect(probe_manager.installed_probes.length).to eq 1
+            expect(component.probe_notifier_worker).to receive(:add_snapshot).once.and_call_original
+            expect(InstrumentationIntegrationTestClass.new.test_method).to eq(42)
+          end
+
+          describe 'payload' do
+            let(:payload) do
+              probe_manager.add_probe(probe)
+              payload = nil
+              expect(component.probe_notifier_worker).to receive(:add_snapshot) do |payload_|
+                payload = payload_
+              end
+              expect(InstrumentationIntegrationTestClass.new.test_method).to eq(42)
+              component.probe_notifier_worker.flush
+              expect(payload).to be_a(Hash)
+              payload
+            end
+
+            let(:snapshot) do
+              payload.fetch(:"debugger.snapshot")
+            end
+
+            it 'does not have captures' do
+              expect(component.transport).to receive(:send_request).at_least(:once)
+              expect(snapshot.fetch(:captures)).to be nil
+            end
+
+            let(:stack) do
+              snapshot.fetch(:stack)
+            end
+
+            let(:top_stack_frame) do
+              stack.first
+            end
+
+            it 'has instrumented location as top stack frame' do
+              expect(component.transport).to receive(:send_request).at_least(:once)
+              expect(File.basename(top_stack_frame.fetch(:fileName))).to eq 'instrumentation_integration_test_class.rb'
+            end
+          end
         end
 
-        describe 'payload' do
-          let(:payload) do
+        include_examples 'simple log probe'
+
+        context 'target line is the end line of a method' do
+          let(:probe) do
+            Datadog::DI::Probe.new(id: "1234", type: :log,
+              file: 'instrumentation_integration_test_class.rb', line_no: 11,
+              capture_snapshot: false,)
+          end
+
+          include_examples 'simple log probe'
+        end
+
+        context 'target line is the end line of a block' do
+          let(:probe) do
+            Datadog::DI::Probe.new(id: "1234", type: :log,
+              file: 'instrumentation_integration_test_class.rb', line_no: 17,
+              capture_snapshot: false,)
+          end
+
+          it 'invokes probe' do
+            expect(component.transport).to receive(:send_request).at_least(:once)
             probe_manager.add_probe(probe)
-            payload = nil
-            expect(component.probe_notifier_worker).to receive(:add_snapshot) do |payload_|
-              payload = payload_
+            component.probe_notifier_worker.flush
+            expect(probe_manager.installed_probes.length).to eq 1
+            expect(component.probe_notifier_worker).to receive(:add_snapshot).once.and_call_original
+            expect(InstrumentationIntegrationTestClass.new.test_method_with_block).to eq([1])
+          end
+
+          describe 'payload' do
+            let(:payload) do
+              probe_manager.add_probe(probe)
+              payload = nil
+              expect(component.probe_notifier_worker).to receive(:add_snapshot) do |payload_|
+                payload = payload_
+              end
+              expect(InstrumentationIntegrationTestClass.new.test_method_with_block).to eq([1])
+              component.probe_notifier_worker.flush
+              expect(payload).to be_a(Hash)
+              payload
             end
-            expect(InstrumentationIntegrationTestClass.new.test_method).to eq(42)
+
+            let(:snapshot) do
+              payload.fetch(:"debugger.snapshot")
+            end
+
+            it 'does not have captures' do
+              expect(component.transport).to receive(:send_request).at_least(:once)
+              expect(snapshot.fetch(:captures)).to be nil
+            end
+
+            let(:stack) do
+              snapshot.fetch(:stack)
+            end
+
+            let(:top_stack_frame) do
+              stack.first
+            end
+
+            it 'has instrumented location as top stack frame' do
+              expect(component.transport).to receive(:send_request).at_least(:once)
+              expect(File.basename(top_stack_frame.fetch(:fileName))).to eq 'instrumentation_integration_test_class.rb'
+            end
+          end
+        end
+
+        shared_examples 'installs but does not invoke probe' do
+          it 'installs but does not invoke probe' do
+            expect(component.transport).to receive(:send_request).once
+            probe_manager.add_probe(probe)
             component.probe_notifier_worker.flush
-            expect(payload).to be_a(Hash)
-            payload
+            expect(probe_manager.installed_probes.length).to eq 1
+            expect(component.probe_notifier_worker).not_to receive(:add_snapshot)
+            call_target
           end
+        end
 
-          let(:snapshot) do
-            payload.fetch(:"debugger.snapshot")
+        context 'target line is else of a conditional' do
+          let(:probe) do
+            Datadog::DI::Probe.new(id: "1234", type: :log,
+              file: 'instrumentation_integration_test_class.rb', line_no: 23,
+              capture_snapshot: false,)
           end
 
-          it 'does not have captures' do
-            expect(component.transport).to receive(:send_request).at_least(:once)
-            expect(snapshot.fetch(:captures)).to be nil
+          let(:call_target) do
+            expect(InstrumentationIntegrationTestClass.new.test_method_with_conditional).to eq(2)
           end
 
-          let(:stack) do
-            snapshot.fetch(:stack)
+          include_examples 'installs but does not invoke probe'
+        end
+
+        context 'target line is end of a conditional' do
+          let(:probe) do
+            Datadog::DI::Probe.new(id: "1234", type: :log,
+              file: 'instrumentation_integration_test_class.rb', line_no: 25,
+              capture_snapshot: false,)
           end
 
-          let(:top_stack_frame) do
-            stack.first
+          let(:call_target) do
+            expect(InstrumentationIntegrationTestClass.new.test_method_with_conditional).to eq(2)
           end
 
-          it 'has instrumented location as top stack frame' do
-            expect(component.transport).to receive(:send_request).at_least(:once)
-            expect(File.basename(top_stack_frame.fetch(:fileName))).to eq 'instrumentation_integration_test_class.rb'
+          include_examples 'installs but does not invoke probe'
+        end
+
+        context 'target line contains a comment (no executable code)' do
+          let(:probe) do
+            Datadog::DI::Probe.new(id: "1234", type: :log,
+              file: 'instrumentation_integration_test_class.rb', line_no: 31,
+              capture_snapshot: false,)
+          end
+
+          # We currently are not told that the line is not executable.
+          it 'installs probe' do
+            expect(probe_manager.add_probe(probe)).to be true
+            expect(probe_manager.installed_probes.length).to eq 1
           end
         end
       end