DEBUG-3439 DEBUG-3440 remove prefixes from probe paths when path matching (#4346)

Author: p-datadog

lib/datadog/di/code_tracker.rb

@@ -140,16 +140,23 @@ def iseqs_for_path_suffix(suffix)
           exact = registry[suffix]
           return [suffix, exact] if exact
 
-          inexact = []
-          registry.each do |path, iseq|
-            if Utils.path_matches_suffix?(path, suffix)
-              inexact << [path, iseq]
+          suffix = suffix.dup
+          loop do
+            inexact = []
+            registry.each do |path, iseq|
+              if Utils.path_matches_suffix?(path, suffix)
+                inexact << [path, iseq]
+              end
             end
+            if inexact.length > 1
+              raise Error::MultiplePathsMatch, "Multiple paths matched requested suffix"
+            end
+            if inexact.any?
+              return inexact.first
+            end
+            return nil unless suffix.include?('/')
+            suffix.sub!(%r{.*/+}, '')
           end
-          if inexact.length > 1
-            raise Error::MultiplePathsMatch, "Multiple paths matched requested suffix"
-          end
-          inexact.first
         end
       end
 

lib/datadog/di/probe.rb

@@ -155,19 +155,16 @@ def location
       # Returns whether the provided +path+ matches the user-designated
       # file (of a line probe).
       #
-      # If file is an absolute path (i.e., it starts with a slash), the file
-      # must be identical to path to match.
-      #
-      # If file is not an absolute path, the path matches if the file is its suffix,
-      # at a path component boundary.
+      # Delegates to Utils.path_can_match_spec? which performs fuzzy
+      # matching. See the comments in utils.rb for details.
       def file_matches?(path)
         if path.nil?
           raise ArgumentError, "Cannot match against a nil path"
         end
         unless file
           raise ArgumentError, "Probe does not have a file to match against"
         end
-        Utils.path_matches_suffix?(path, file)
+        Utils.path_can_match_spec?(path, file)
       end
 
       # Instrumentation module for method probes.

lib/datadog/di/utils.rb

@@ -3,6 +3,82 @@
 module Datadog
   module DI
     module Utils
+      # General path matching considerations
+      # ------------------------------------
+      #
+      # The following use cases must be supported:
+      # 1. The "probe path" is relative path to the file from source code
+      #    repository root. The project is deployed from the repository root,
+      #    such that that same relative path exists at runtime from the
+      #    root of the application.
+      # 2. The "probe path" is a relative path to the file in a monorepo
+      #    where the project being deployed is in a subdirectory.
+      #    This the "probe path" contains additional directory components
+      #    in the beginning that do not exist in the runtime environment.
+      # 3. The "probe path" is an absolute path to the file on the customer's
+      #    development system. As specified this path definitely does not
+      #    exist at runtime, and can start with a prefix that is unknown
+      #    to any both UI and tracer code.
+      # 4. Same as (3), but the customer is using a Windows computer for
+      #    development and has the path specified in the wrong case
+      #    (which works fine on their development machine).
+      # 5. The "probe path" is the basename or any suffix of the path to
+      #    the desired file, typed manually by the customer into the UI.
+      #
+      # A related concern is that if multiple runtime paths match the path
+      # specification in the probe, the tracer must return an error to the
+      # backend/UI rather than instrumenting any of the matching paths.
+      #
+      # The logic for path matching should therefore, generally, be as follows:
+      # 1. If the "probe path" is absolute, see if it exists at runtime.
+      #    If so, take it as the desired path and finish.
+      # 2. Attempt to identify the application root, by checking if the current
+      #    working directory contains a file called Gemfile. If yes, assume
+      #    the current working directory is the application root, otherwise
+      #    consider the application root to be unknown.
+      # 3. If the application root is known and the "probe path" is relative,
+      #    concatenate the "probe path" to the application root and check
+      #    if the resulting path exists at runtime. If so, take it as the
+      #    desired path and finish.
+      # 4. If the "probe path" is relative, go through the known file paths,
+      #    filter these paths down to those whose suffix is the "probe path",
+      #    and check how many we are left with. If exactly one, assume this
+      #    is the desired path and finish. If more than one, return an error
+      #    "multiple matching paths".
+      # 5. If the application root is known, for each suffix of the "probe path",
+      #    see if that relative paths concatenated to the application root
+      #    results in a known file. If a known file is found, assume this
+      #    is the wanted file and finish.
+      # 6. For each suffix of the "probe path", filter the set of known paths
+      #    down to those that end in the suffix. If exactly one path remains
+      #    for a given suffix, assume this is the wanted path and finish.
+      #    If more than one path remains for a given suffix, return the error
+      #    "multiple matching paths".
+      # 7. Repeat step 5 but perform case-insensitive comparison.
+      # 8. Repeat step 6 but perform case-insensitive comparison.
+      #
+      # Note that we do not look for "probe paths" under the current working
+      # directory at runtime if the current working directory does not contain
+      # a Gemfile, to avoid finding files from potentially undesired bases.
+      #
+      # What "known file"/"known path" means also differs based on the
+      # operation being performed:
+      # - If the operation is "install a probe", "known file/path" can
+      #   include files on the filesystem that have not been loaded as
+      #   well as paths from the code tracker.
+      # - If the operation is "check if any pending probes match the file
+      #   that was just loaded", we would only consider the path that was
+      #   just loaded and not check the filesystem.
+      #
+      # Filesystem inquiries are obviously quite expensive and should be
+      # cached. For the vast majority of applications it should be safe to
+      # indefinitely cache whether a particular filesystem paths exists
+      # in both positive and negative.
+      #
+      # As a "quick fix", currently after performing the suffix matching
+      # we just strip leading directory components from the "probe path"
+      # until we get a match via a "suffix of the suffix".
+
       # Returns whether the provided +path+ matches the user-designated
       # file suffix (of a line probe).
       #
@@ -41,6 +117,21 @@ module Utils
           # !!(path =~ %r,(/|\A)#{Regexp.quote(suffix)}\z,)
         end
       end
+
+      # Returns whether the provided +path+ matches the "probe path" in
+      # +spec+. Attempts all of the fuzzy matches by stripping directories
+      # from the front of +spec+. Does not consider othr known paths to
+      # identify the case of (potentially) multiple matching paths for +spec+.
+      module_function def path_can_match_spec?(path, spec)
+        return true if path_matches_suffix?(path, spec)
+
+        spec = spec.dup
+        loop do
+          return false unless spec.include?('/')
+          spec.sub!(%r{.*/+}, '')
+          return true if path_matches_suffix?(path, spec)
+        end
+      end
     end
   end
 end

sig/datadog/di/utils.rbs

@@ -2,6 +2,7 @@ module Datadog
   module DI
     module Utils
       def self?.path_matches_suffix?: (String path, String suffix) -> bool
+      def self?.path_can_match_spec?: (String path, String suffix) -> bool
     end
   end
 end

spec/datadog/di/integration/everything_from_remote_config_spec.rb

@@ -230,10 +230,10 @@ def target_method
 
   let(:payloads) { [] }
 
-  def do_rc(expect_hook: true)
+  def do_rc(expect_hook: :hook_method)
     expect(probe_manager).to receive(:add_probe).and_call_original
     if expect_hook
-      expect(instrumenter).to receive(:hook_method).and_call_original
+      expect(instrumenter).to receive(expect_hook).and_call_original
     end
     # Events can be batched, meaning +post+ could be called once or twice
     # depending on how threads are scheduled by the VM.
@@ -370,13 +370,22 @@ def assert_received_and_errored
   context 'line probe' do
     with_code_tracking
 
-    context 'line probe received targeting loaded code not in code tracker' do
-      let(:probe_spec) do
-        {id: '11', name: 'bar', type: 'LOG_PROBE', where: {
-          sourceFile: 'instrumentation_integration_test_class.rb', lines: [22]
-        }}
+    shared_context 'targeting integration test class via load' do
+      before do
+        begin
+          Object.send(:remove_const, :InstrumentationIntegrationTestClass)
+        rescue
+          nil
+        end
+        load File.join(File.dirname(__FILE__), 'instrumentation_integration_test_class.rb')
+
+        # We want the probe status to be reported, therefore need to
+        # disable exception propagation.
+        settings.dynamic_instrumentation.internal.propagate_all_exceptions = false
       end
+    end
 
+    shared_context 'targeting integration test class via require' do
       before do
         begin
           Object.send(:remove_const, :InstrumentationIntegrationTestClass)
@@ -391,14 +400,46 @@ def assert_received_and_errored
         expect($LOADED_FEATURES.detect do |path|
           File.basename(path) == 'instrumentation_integration_test_class.rb'
         end).to be_truthy
-        component.code_tracker.clear
 
         # We want the probe status to be reported, therefore need to
         # disable exception propagation.
         settings.dynamic_instrumentation.internal.propagate_all_exceptions = false
       end
+    end
+
+    context 'line probe with path containing extra prefix directories' do
+      let(:probe_spec) do
+        {id: '11', name: 'bar', type: 'LOG_PROBE', where: {
+          sourceFile: 'junk/prefix/instrumentation_integration_test_class.rb', lines: [22]
+        }}
+      end
+
+      include_context 'targeting integration test class via load'
 
       it 'instruments code and adds probe to installed list' do
+        expect_lazy_log(logger, :debug, /received probe from RC:/)
+
+        do_rc(expect_hook: :hook_line)
+        assert_received_and_installed
+
+        expect(probe_manager.installed_probes.length).to eq 1
+      end
+    end
+
+    context 'line probe received targeting loaded code not in code tracker' do
+      let(:probe_spec) do
+        {id: '11', name: 'bar', type: 'LOG_PROBE', where: {
+          sourceFile: 'instrumentation_integration_test_class.rb', lines: [22]
+        }}
+      end
+
+      include_context 'targeting integration test class via require'
+
+      before do
+        component.code_tracker.clear
+      end
+
+      it 'marks RC payload as errored' do
         expect_lazy_log_many(logger, :debug,
           /received probe from RC:/,
           /error processing probe configuration:.*File matching probe path.*was loaded and is not in code tracker registry/,)

spec/datadog/di/utils_spec.rb

@@ -5,6 +5,8 @@
   di_test
 
   describe '.path_matches_suffix?' do
+    # NB; when updating this list also add to the list in
+    # path_can_match_spec? test below.
     [
       ['exact match - absolute path', '/foo/bar.rb', '/foo/bar.rb', true],
       # Suffix matching is only applicable to relative paths.
@@ -15,7 +17,7 @@
       ['extra leading slash in file', '//foo/bar.rb', '/foo/bar.rb', false],
       ['extra slash in the middle of file', 'foo//bar.rb', '/foo/bar.rb', false],
       ['nothing in common', 'blah.rb', '/foo/bar.rb', false],
-      ['path is a suffix of file', '/a/foo/bar.rbr', '/foo/bar.rb', false],
+      ['path is a suffix of file', '/a/foo/bar.rb', '/foo/bar.rb', false],
 
       # We expect path to always be an absolute path.
       ['path is relative', 'bar.rb', 'bar.rb', false],
@@ -29,4 +31,36 @@
       end
     end
   end
+
+  describe '.path_can_match_spec?' do
+    # NB; when updating this list also add to the list in
+    # path_matches_suffix? test above.
+    [
+      ['exact match - absolute path', '/foo/bar.rb', '/foo/bar.rb', true],
+      # Prefixes of suffix are removed until there is a match
+      ['absolute path is a suffix', '/bar.rb', '/foo/bar.rb', true],
+      # ... but not if basename does not match
+      ['absolute path is a suffix', '/bar.rb', '/foo/bar1.rb', false],
+      ['suffix - multiple path components', 'foo/bar.rb', '/foo/bar.rb', true],
+      ['suffix - at basename', 'bar.rb', '/foo/bar.rb', true],
+      ['suffix - not at path component boundary', 'ar.rb', '/foo/bar.rb', false],
+      # Extra leading slashes are removed
+      ['extra leading slash in file', '//foo/bar.rb', '/foo/bar.rb', true],
+      # Extra slashes in the middle are also removed
+      ['extra slash in the middle of file', 'foo//bar.rb', '/foo/bar.rb', true],
+      ['nothing in common', 'blah.rb', '/foo/bar.rb', false],
+      ['path is a suffix of file', '/a/foo/bar.rb', '/foo/bar.rb', true],
+
+      # We expect path to always be an absolute path.
+      ['path is relative', 'bar.rb', 'bar.rb', false],
+    ].each do |name, suffix_, path_, result_|
+      suffix, path, result = suffix_, path_, result_
+
+      context name do
+        it "is #{result}" do
+          expect(described_class.path_can_match_spec?(path, suffix)).to be result
+        end
+      end
+    end
+  end
 end