DEBUG-2334 DI internal settings reorg + telemetry + error handling (#…

…4089)

- Moves internal DI settings to "internal" subkey
- Minimum send interval for the network submission worker moved to a new internal configuration setting from the constant
- Injects telemetry object into DI components
- Reports unhandled exceptions to telemetry in DI code already in master
- Line probes now require a single match of the provided path suffix, if multiple paths match an exception is raised (this will be reported via UI to the user, on the assumption that user wanted a specific file instrumented and we don't know which of the multiple matching files is the correct one)
- When probe installation fails for reasons other than the referenced target being missing (presumably not yet loaded), the failure is noted and that probe is not attempted again in the future (this PR contains part of the code for this change)
- Improves probe notifier worker flush logic to be event-driven instead of sleeping

benchmarks/di_instrument.rb

@@ -73,7 +73,11 @@ def run_benchmark
     settings = Datadog.configuration
     # We benchmark untargeted and targeted trace points; untargeted ones
     # are prohibited by default, permit them.
-    settings.dynamic_instrumentation.untargeted_trace_points = true
+    begin
+      settings.dynamic_instrumentation.internal.untargeted_trace_points = true
+    rescue NoMethodError
+      settings.dynamic_instrumentation.untargeted_trace_points = true
+    end
     redactor = Datadog::DI::Redactor.new(settings)
     serializer = Datadog::DI::Serializer.new(settings, redactor)
     logger = Logger.new(STDERR)

lib/datadog/di/code_tracker.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+# rubocop:disable Lint/AssignmentInCondition
+
 module Datadog
   module DI
     # Tracks loaded Ruby code by source file and maintains a map from
@@ -77,6 +79,23 @@ def start
                 registry[path] = tp.instruction_sequence
               end
             end
+          # Since this method normally is called from customer applications,
+          # rescue any exceptions that might not be handled to not break said
+          # customer applications.
+          rescue => exc
+            # TODO we do not have DI.component defined yet, remove steep:ignore
+            # before release.
+            if component = DI.component # steep:ignore
+              raise if component.settings.dynamic_instrumentation.internal.propagate_all_exceptions
+              component.logger.warn("Unhandled exception in script_compiled trace point: #{exc.class}: #{exc}")
+              component.telemetry&.report(exc, description: "Unhandled exception in script_compiled trace point")
+              # TODO test this path
+            else
+              # If we don't have a component, we cannot log anything properly.
+              # Do not just print a warning to avoid spamming customer logs.
+              # Don't reraise the exception either.
+              # TODO test this path
+            end
           end
         end
       end
@@ -112,15 +131,18 @@ def active?
       def iseqs_for_path_suffix(suffix)
         registry_lock.synchronize do
           exact = registry[suffix]
-          return [exact] if exact
+          return [suffix, exact] if exact
 
           inexact = []
           registry.each do |path, iseq|
             if Utils.path_matches_suffix?(path, suffix)
-              inexact << iseq
+              inexact << [path, iseq]
             end
           end
-          inexact
+          if inexact.length > 1
+            raise Error::MultiplePathsMatch, "Multiple paths matched requested suffix"
+          end
+          inexact.first
         end
       end
 
@@ -164,3 +186,5 @@ def clear
     end
   end
 end
+
+# rubocop:enable Lint/AssignmentInCondition

lib/datadog/di/configuration/settings.rb

@@ -12,8 +12,6 @@ def self.extended(base)
 
         def self.add_settings!(base)
           base.class_eval do
-            # The setting has "internal" prefix to prevent it from being
-            # prematurely turned on by customers.
             settings :dynamic_instrumentation do
               option :enabled do |o|
                 o.type :bool
@@ -26,48 +24,6 @@ def self.add_settings!(base)
                 o.default false
               end
 
-              # This option instructs dynamic instrumentation to use
-              # untargeted trace points when installing line probes and
-              # code tracking is not active.
-              # WARNING: untargeted trace points carry a massive performance
-              # penalty for the entire file in which a line probe is placed.
-              #
-              # If this option is set to false, which is the default,
-              # dynamic instrumentation will add probes that reference
-              # unknown files to the list of pending probes, and when
-              # the respective files are loaded, the line probes will be
-              # installed using targeted trace points. If the file in
-              # question is already loaded when the probe is received
-              # (for example, it is in a third-party library loaded during
-              # application boot), and code tracking was not active when
-              # the file was loaded, such files will not be instrumentable
-              # via line probes.
-              #
-              # If this option is set to true
-              #
-              # activated, DI will in
-              # activated or because the files being targeted have beenIf true and code tracking is not enabled, dynamic instrumentation
-              # will use untargeted trace points.
-              # If false and code tracking is not enabled, dynamic
-              # instrumentation will not instrument any files loaded
-              # WARNING: these trace points will greatly degrade performance
-              # of all code in the instrumented files.
-              option :untargeted_trace_points do |o|
-                o.type :bool
-                o.default false
-              end
-
-              # If true, all of the catch-all rescue blocks in DI
-              # will propagate the exceptions onward.
-              # WARNING: for internal Datadog use only - this will break
-              # the DI product and potentially the library in general in
-              # a multitude of ways, cause resource leakage, permanent
-              # performance decreases, etc.
-              option :propagate_all_exceptions do |o|
-                o.type :bool
-                o.default false
-              end
-
               # An array of variable and key names to redact in addition to
               # the built-in list of identifiers.
               #
@@ -154,6 +110,75 @@ def self.add_settings!(base)
                 o.type :int
                 o.default 20
               end
+
+              # Settings in the 'internal' group are for internal Datadog
+              # use only, and are needed to test dynamic instrumentation or
+              # experiment with features not released to customers.
+              settings :internal do
+                # This option instructs dynamic instrumentation to use
+                # untargeted trace points when installing line probes and
+                # code tracking is not active.
+                # WARNING: untargeted trace points carry a massive performance
+                # penalty for the entire file in which a line probe is placed.
+                #
+                # If this option is set to false, which is the default,
+                # dynamic instrumentation will add probes that reference
+                # unknown files to the list of pending probes, and when
+                # the respective files are loaded, the line probes will be
+                # installed using targeted trace points. If the file in
+                # question is already loaded when the probe is received
+                # (for example, it is in a third-party library loaded during
+                # application boot), and code tracking was not active when
+                # the file was loaded, such files will not be instrumentable
+                # via line probes.
+                #
+                # If this option is set to true, dynamic instrumentation will
+                # install untargeted trace points for all line probes,
+                # regardless of whether the referenced file is loaded.
+                # This permits instrumenting code which was loaded prior to
+                # code tracking being activated and instrumenting lines when
+                # code tracking is not activated at all. However, untargeted
+                # trace points are extremely slow and will greatly degrade
+                # performance of *all* code executed while they are installed,
+                # not just the instrumentation target.
+                option :untargeted_trace_points do |o|
+                  o.type :bool
+                  o.default false
+                end
+
+                # If true, all of the catch-all rescue blocks in DI
+                # will propagate the exceptions onward.
+                # WARNING: for internal Datadog use only - this will break
+                # the DI product and potentially the library in general in
+                # a multitude of ways, cause resource leakage, permanent
+                # performance decreases, etc.
+                option :propagate_all_exceptions do |o|
+                  o.type :bool
+                  o.default false
+                end
+
+                # Minimum interval, in seconds, between probe status and
+                # snapshot submissions to the agent. Probe notifier worker will
+                # batch together payloads submitted during each interval.
+                # A longer interval reduces the overhead imposed by dynamic
+                # instrumentation on the application, but also increases the
+                # time when application code cannot run (when the batches are
+                # being sent out by the probe notifier worker) and creates a
+                # possibility of dropping payloads if the queue gets too long.
+                option :min_send_interval do |o|
+                  o.type :int
+                  o.default 3
+                end
+
+                # Enable dynamic instrumentation in development environments.
+                # Currently DI does not fully implement support for code
+                # unloading and reloading, and is not supported in
+                # non-production environments.
+                option :development do |o|
+                  o.type :bool
+                  o.default false
+                end
+              end
             end
           end
         end

lib/datadog/di/error.rb

@@ -26,6 +26,23 @@ class AgentCommunicationError < Error
       # that does not in fact exist anywhere (e.g. due to a misspelling).
       class DITargetNotDefined < Error
       end
+
+      # Raised when trying to install a probe whose installation failed
+      # earlier in the same process. This exception should contain the
+      # original exception report from initial installation attempt.
+      class ProbePreviouslyFailed < Error
+      end
+
+      # Raised when installing a line probe and multiple files match the
+      # specified path suffix.
+      # A probe must be installed into one file only, since UI only
+      # supports one instrumented location for a probe.
+      # If multiple files match, UI cannot properly render the data from
+      # all of them, and arbitrarily choosing one file may be not what the
+      # user intended. Instrumentation will fail when multiple files match
+      # and the user will need to make their suffix more precise.
+      class MultiplePathsMatch < Error
+      end
     end
   end
 end

lib/datadog/di/instrumenter.rb

@@ -54,10 +54,11 @@ module DI
     #
     # @api private
     class Instrumenter
-      def initialize(settings, serializer, logger, code_tracker: nil)
+      def initialize(settings, serializer, logger, code_tracker: nil, telemetry: nil)
         @settings = settings
         @serializer = serializer
         @logger = logger
+        @telemetry = telemetry
         @code_tracker = code_tracker
 
         @lock = Mutex.new
@@ -66,6 +67,7 @@ def initialize(settings, serializer, logger, code_tracker: nil)
       attr_reader :settings
       attr_reader :serializer
       attr_reader :logger
+      attr_reader :telemetry
       attr_reader :code_tracker
 
       # This is a substitute for Thread::Backtrace::Location
@@ -172,12 +174,12 @@ def hook_line(probe, &block)
         # we use mock objects and the methods may be mocked with
         # individual invocations, yielding different return values on
         # different calls to the same method.
-        permit_untargeted_trace_points = settings.dynamic_instrumentation.untargeted_trace_points
+        permit_untargeted_trace_points = settings.dynamic_instrumentation.internal.untargeted_trace_points
 
         iseq = nil
         if code_tracker
-          iseq = code_tracker.iseqs_for_path_suffix(probe.file).first # steep:ignore
-          unless iseq
+          ret = code_tracker.iseqs_for_path_suffix(probe.file) # steep:ignore
+          unless ret
             if permit_untargeted_trace_points
               # Continue withoout targeting the trace point.
               # This is going to cause a serious performance penalty for
@@ -204,6 +206,10 @@ def hook_line(probe, &block)
           raise Error::DITargetNotDefined, "File not in code tracker registry: #{probe.file}"
         end
 
+        if ret
+          actual_path, iseq = ret
+        end
+
         # If trace point is not targeted, we only need one trace point per file.
         # Creating a trace point for each probe does work but the performance
         # penalty will be taken for each trace point defined in the file.
@@ -217,18 +223,26 @@ def hook_line(probe, &block)
         # this optimization just yet and create a trace point for each probe.
 
         tp = TracePoint.new(:line) do |tp|
-          # If trace point is not targeted, we must verify that the invocation
-          # is the file & line that we want, because untargeted trace points
-          # are invoked for *each* line of Ruby executed.
-          if iseq || tp.lineno == probe.line_no && probe.file_matches?(tp.path)
-            if rate_limiter.nil? || rate_limiter.allow?
-              # & is to stop steep complaints, block is always present here.
-              block&.call(probe: probe, trace_point: tp, caller_locations: caller_locations)
+          begin
+            # If trace point is not targeted, we must verify that the invocation
+            # is the file & line that we want, because untargeted trace points
+            # are invoked for *each* line of Ruby executed.
+            if iseq || tp.lineno == probe.line_no && probe.file_matches?(tp.path)
+              if rate_limiter.nil? || rate_limiter.allow?
+                # & is to stop steep complaints, block is always present here.
+                block&.call(probe: probe, trace_point: tp, caller_locations: caller_locations)
+              end
             end
+          rescue => exc
+            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
           end
         rescue => exc
           raise if settings.dynamic_instrumentation.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
         end
 
@@ -244,6 +258,8 @@ def hook_line(probe, &block)
           end
 
           probe.instrumentation_trace_point = tp
+          # actual_path could be nil if we don't use targeted trace points.
+          probe.instrumented_path = actual_path
 
           if iseq
             tp.enable(target: iseq, target_line: line_no)

lib/datadog/di/probe.rb

@@ -71,6 +71,8 @@ def initialize(id:, type:,
 
         @rate_limit = rate_limit || (@capture_snapshot ? 1 : 5000)
         @rate_limiter = Datadog::Core::TokenBucket.new(@rate_limit)
+
+        @emitting_notified = false
       end
 
       attr_reader :id
@@ -157,6 +159,16 @@ def file_matches?(path)
       # Line trace point for line probes. Normally this would be a targeted
       # trace point.
       attr_accessor :instrumentation_trace_point
+
+      # Actual path to the file instrumented by the probe, for line probes,
+      # when code tracking is available and line trace point is targeted.
+      # For untargeted line trace points instrumented path will be nil.
+      attr_accessor :instrumented_path
+
+      attr_writer :emitting_notified
+      def emitting_notified?
+        !!@emitting_notified
+      end
     end
   end
 end