DI: report evaluation errors in the correct schema (#4951)

Author: p-datadog

lib/datadog/di/el/compiler.rb

@@ -17,7 +17,7 @@ module EL
       # @api private
       class Compiler
         def compile(ast)
-          Expression.new(compile_partial(ast))
+          compile_partial(ast)
         end
 
         private

lib/datadog/di/el/expression.rb

@@ -7,11 +7,13 @@ module EL
       #
       # @api private
       class Expression
-        def initialize(compiled_expr)
+        def initialize(dsl_expr, compiled_expr)
           unless String === compiled_expr
             raise ArgumentError, "compiled_expr must be a string"
           end
 
+          @dsl_expr = dsl_expr
+
           cls = Class.new(Evaluator)
           cls.class_exec do
             eval(<<-RUBY, Object.new.send(:binding), __FILE__, __LINE__ + 1) # standard:disable Security/Eval
@@ -24,6 +26,7 @@ def evaluate(context)
           @evaluator = cls.new
         end
 
+        attr_reader :dsl_expr
         attr_reader :evaluator
 
         def evaluate(context)

lib/datadog/di/probe_builder.rb

@@ -30,6 +30,13 @@ def build_from_remote_config(config)
         # The validations here are not yet comprehensive.
         type = config.fetch('type')
         type_symbol = PROBE_TYPES[type] or raise ArgumentError, "Unrecognized probe type: #{type}"
+        cond = if cond_spec = config['when']
+          unless cond_spec['dsl'] && cond_spec['json']
+            raise ArgumentError, "Malformed condition specification for probe: #{config}"
+          end
+          compiled = EL::Compiler.new.compile(cond_spec['json'])
+          EL::Expression.new(cond_spec['dsl'], compiled)
+        end
         Probe.new(
           id: config.fetch("id"),
           type: type_symbol,
@@ -47,7 +54,7 @@ def build_from_remote_config(config)
           max_capture_depth: config["capture"]&.[]("maxReferenceDepth"),
           max_capture_attribute_count: config["capture"]&.[]("maxFieldCount"),
           rate_limit: config["sampling"]&.[]("snapshotsPerSecond"),
-          condition: (cond = config.dig('when', 'json')) && EL::Compiler.new.compile(cond), # steep:ignore
+          condition: cond,
         )
       rescue KeyError => exc
         raise ArgumentError, "Malformed remote configuration entry for probe: #{exc.class}: #{exc}: #{config}"
@@ -59,7 +66,11 @@ def build_template_segments(segments)
             if str = segment['str']
               str
             elsif ast = segment['json']
-              EL::Compiler.new.compile(ast)
+              unless dsl = segment['dsl']
+                raise ArgumentError, "Missing dsl for json in segment: #{segment}"
+              end
+              compiled = EL::Compiler.new.compile(ast)
+              EL::Expression.new(dsl, compiled)
             else
               # TODO report to telemetry?
             end

lib/datadog/di/probe_notification_builder.rb

@@ -188,7 +188,10 @@ def evaluate_template(template_segments, context)
             raise ArgumentError, "Invalid template segment type: #{segment}"
           end
         rescue => exc
-          evaluation_errors << "#{exc.class}: #{exc}"
+          evaluation_errors << {
+            message: "#{exc.class}: #{exc}",
+            expr: segment.dsl_expr,
+          }
           '[evaluation error]'
         end.join
         [message, evaluation_errors]

sig/datadog/di/el/expression.rbs

@@ -2,13 +2,15 @@ module Datadog
   module DI
     module EL
       class Expression
-        @compiled_expr: untyped
+        @dsl_expr: untyped
 
-        def initialize: (untyped compiled_expr) -> void
+        @evaluator: untyped
 
-        attr_reader compiled_expr: untyped
+        def initialize: (untyped dsl_expr, untyped compiled_expr) -> void
 
-        def ==: (untyped other) -> untyped
+        attr_reader dsl_expr: untyped
+
+        attr_reader evaluator: untyped
 
         def evaluate: (untyped context) -> untyped
 

sig/datadog/di/probe.rbs

@@ -25,6 +25,7 @@ module Datadog
       @executed_on_line: bool?
 
       def initialize: (id: String, type: Symbol, ?file: String?, ?line_no: Integer?, ?type_name: String?, ?method_name: String?, ?template: String?, ?template_segments: Array[untyped]?, ?capture_snapshot: bool,
+	?condition: DI::EL::Expression?,
 	?max_capture_depth: Integer, ?max_capture_attribute_count: Integer?, ?rate_limit: Integer) -> void
 
       attr_reader condition: DI::EL::Expression?

spec/datadog/di/el/integration_spec.rb

@@ -40,9 +40,10 @@ class ELTestIvarClass
           let(:expected) { spec.fetch('compiled') }
 
           let(:compiled) { compiler.compile(ast) }
+          let(:expr) { Datadog::DI::EL::Expression.new('(expression)', compiled) }
 
           let(:evaluated) do
-            compiled.evaluate(context)
+            expr.evaluate(context)
           end
 
           let(:context) do

spec/datadog/di/instrumenter_spec.rb

@@ -933,6 +933,7 @@
         context 'when condition is met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               # We use "arg1" here, actual variable name is not currently available
               "ref('arg1') == 41"
             )
@@ -944,6 +945,7 @@
         context 'when condition is not met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               # We use "arg1" here, actual variable name is not currently available
               "ref('arg1') == 42"
             )
@@ -957,6 +959,7 @@
         context 'when condition is met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "ref('kwarg') == 42"
             )
           end
@@ -967,6 +970,7 @@
         context 'when condition is not met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "ref('kwarg') == 41"
             )
           end
@@ -980,6 +984,7 @@
 
         let(:condition) do
           Datadog::DI::EL::Expression.new(
+            '(expression)',
             "unknown_function('kwarg') == 42"
           )
         end
@@ -1451,6 +1456,7 @@
         context 'when condition is met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "ref('local') == 42"
             )
           end
@@ -1466,6 +1472,7 @@
         context 'when condition is not met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "ref('local') == 43"
             )
           end
@@ -1489,6 +1496,7 @@
         context 'when condition is met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "iref('@ivar') == 42"
             )
           end
@@ -1504,6 +1512,7 @@
         context 'when condition is not met' do
           let(:condition) do
             Datadog::DI::EL::Expression.new(
+              '(expression)',
               "iref('@ivar') == 43"
             )
           end

spec/datadog/di/integration/everything_from_remote_config_spec.rb

@@ -398,25 +398,125 @@ def assert_received_and_errored
           where: {
             typeName: 'EverythingFromRemoteConfigSpecTestClass', methodName: 'target_method',
           },
-          when: {json: {foo: 'bar'}},
+          when: {json: {foo: 'bar'}, dsl: '(expression)'},
         }
       end
 
       let(:propagate_all_exceptions) { false }
 
-      it 'catches the exception' do
+      it 'catches the exception and reports probe status error' do
         expect_lazy_log(logger, :debug, /di: unhandled exception handling a probe in DI remote receiver: Datadog::DI::Error::InvalidExpression: Unknown operation: foo/)
 
         do_rc(expect_add_probe: false)
         expect(probe_manager.installed_probes.length).to eq 0
 
         payload = payloads.first
         expect(payload).to be_a(Hash)
+        expect(payload).to match(
+          ddsource: 'dd_debugger',
+          debugger: {
+            diagnostics: {
+              parentId: nil,
+              probeId: '11',
+              probeVersion: 0,
+              runtimeId: String,
+              status: 'ERROR',
+            },
+          },
+          path: '/debugger/v1/diagnostics',
+          service: 'rspec',
+          timestamp: Integer,
+          message: String,
+        )
         expect(payload[:message]).to match(
           /Instrumentation for probe .* failed: Unknown operation: foo/,
         )
       end
     end
+
+    context 'when there is a message template' do
+      let(:probe_spec) do
+        {
+          id: '11', name: 'bar', type: 'LOG_PROBE',
+          where: {
+            typeName: 'EverythingFromRemoteConfigSpecTestClass', methodName: 'target_method',
+          },
+          segments: [
+            # String segment
+            {str: 'hello '},
+            # Expression segment - valid at runtime
+            {json: {eq: [{ref: '@ivar'}, 51]}, dsl: '(good expression)'},
+            # Another expression which fails evaluation at runtime
+            {json: {filter: [{ref: '@ivar'}, 'x']}, dsl: '(failing expression)'},
+          ],
+        }
+      end
+
+      let(:expected_snapshot_payload) do
+        {
+          path: '/debugger/v1/input',
+          # We do not have active span/trace in the test.
+          "dd.span_id": nil,
+          "dd.trace_id": nil,
+          "debugger.snapshot": {
+            captures: nil,
+            evaluationErrors: [
+              {'expr' => '(failing expression)', 'message' => 'Datadog::DI::Error::ExpressionEvaluationError: Bad collection type for filter: NilClass'},
+            ],
+            id: LOWERCASE_UUID_REGEXP,
+            language: 'ruby',
+            probe: {
+              id: '11',
+              location: {
+                method: 'target_method',
+                type: 'EverythingFromRemoteConfigSpecTestClass',
+              },
+              version: 0,
+            },
+            stack: Array,
+            timestamp: Integer,
+          },
+          ddsource: 'dd_debugger',
+          duration: Integer,
+          host: nil,
+          logger: {
+            method: 'target_method',
+            name: nil,
+            thread_id: nil,
+            thread_name: 'Thread.main',
+            version: 2,
+          },
+          # false is the result of first expression evaluation
+          # second expression fails evaluation
+          message: 'hello false[evaluation error]',
+          service: 'rspec',
+          timestamp: Integer,
+        }
+      end
+
+      it 'evaluates expressions and reports errors' do
+        expect_lazy_log(logger, :debug, /di: received log probe/)
+
+        do_rc
+        assert_received_and_installed
+
+        # invocation
+
+        expect(EverythingFromRemoteConfigSpecTestClass.new.target_method).to eq 42
+
+        component.probe_notifier_worker.flush
+
+        # assertions
+
+        expect(payloads.length).to eq 2
+
+        emitting_payload = payloads.shift
+        expect(emitting_payload).to match(expected_emitting_payload)
+
+        snapshot_payload = payloads.shift
+        expect(order_hash_keys(snapshot_payload)).to match(deep_stringify_keys(order_hash_keys(expected_snapshot_payload)))
+      end
+    end
   end
 
   context 'line probe' do
@@ -514,7 +614,7 @@ def assert_received_and_errored
           where: {
             sourceFile: 'hook_line_load.rb', lines: [14],
           },
-          when: {json: {'contains' => [{'ref' => 'bar'}, 'baz']}},
+          when: {json: {'contains' => [{'ref' => 'bar'}, 'baz']}, dsl: '(expression)'},
         }
       end
 

spec/datadog/di/integration/instrumentation_spec.rb

@@ -509,7 +509,7 @@ def run_test
           let(:segments) do
             [
               {str: 'hello '},
-              {json: {ref: '@duration'}},
+              {json: {ref: '@duration'}, dsl: '@duration'},
               {str: ' ms'},
             ]
           end
@@ -538,7 +538,7 @@ def run_test
           let(:segments) do
             [
               {str: 'hello '},
-              {json: {ref: '@return'}},
+              {json: {ref: '@return'}, dsl: '@return'},
             ]
           end
 
@@ -560,7 +560,7 @@ def run_test
           let(:segments) do
             [
               {str: 'hello '},
-              {json: {ref: '@exception'}},
+              {json: {ref: '@exception'}, dsl: '@exception'},
             ]
           end