Add Datadog::Error value-object

p-lambert · p-lambert · commit 390caff21231 · 2017-08-28T23:24:52.000-04:00
This ensures all error-related data is properly encoded.
diff --git a/lib/ddtrace.rb b/lib/ddtrace.rb
@@ -1,6 +1,7 @@
 require 'ddtrace/monkey'
 require 'ddtrace/pin'
 require 'ddtrace/tracer'
+require 'ddtrace/error'
 
 # \Datadog global namespace that includes all tracing functionality for Tracer and Span classes.
 module Datadog
diff --git a/lib/ddtrace/contrib/rails/action_controller.rb b/lib/ddtrace/contrib/rails/action_controller.rb
@@ -66,11 +66,7 @@ def self.process_action(_name, start, finish, _id, payload)
               else
                 status = '500'
               end
-              if status.starts_with?('5')
-                span.status = 1
-                span.set_tag(Datadog::Ext::Errors::TYPE, error[0])
-                span.set_tag(Datadog::Ext::Errors::MSG, error[1])
-              end
+              span.set_error(error) if status.starts_with?('5')
             end
           ensure
             span.start_time = start
diff --git a/lib/ddtrace/contrib/rails/action_view.rb b/lib/ddtrace/contrib/rails/action_view.rb
@@ -74,13 +74,7 @@ def self.render_template(_name, start, finish, _id, payload)
             template_name = Datadog::Contrib::Rails::Utils.normalize_template_name(payload.fetch(:identifier))
             span.set_tag('rails.template_name', template_name)
             span.set_tag('rails.layout', payload.fetch(:layout))
-
-            if payload[:exception]
-              error = payload[:exception]
-              span.status = 1
-              span.set_tag(Datadog::Ext::Errors::TYPE, error[0])
-              span.set_tag(Datadog::Ext::Errors::MSG, error[1])
-            end
+            span.set_error(payload[:exception]) if payload[:exception]
           ensure
             span.start_time = start
             span.finish(finish)
@@ -102,13 +96,7 @@ def self.render_partial(_name, start, finish, _id, payload)
           begin
             template_name = Datadog::Contrib::Rails::Utils.normalize_template_name(payload.fetch(:identifier))
             span.set_tag('rails.template_name', template_name)
-
-            if payload[:exception]
-              error = payload[:exception]
-              span.status = 1
-              span.set_tag(Datadog::Ext::Errors::TYPE, error[0])
-              span.set_tag(Datadog::Ext::Errors::MSG, error[1])
-            end
+            span.set_error(payload[:exception]) if payload[:exception]
           ensure
             span.start_time = start
             span.finish(finish)
diff --git a/lib/ddtrace/contrib/rails/active_support.rb b/lib/ddtrace/contrib/rails/active_support.rb
@@ -97,13 +97,7 @@ def self.trace_cache(resource, _name, start, finish, _id, payload)
             store, = *Array.wrap(::Rails.configuration.cache_store).flatten
             span.set_tag('rails.cache.backend', store)
             span.set_tag('rails.cache.key', payload.fetch(:key))
-
-            if payload[:exception]
-              error = payload[:exception]
-              span.status = 1
-              span.set_tag(Datadog::Ext::Errors::TYPE, error[0])
-              span.set_tag(Datadog::Ext::Errors::MSG, error[1])
-            end
+            span.set_error(payload[:exception]) if payload[:exception]
           ensure
             span.start_time = start
             span.finish(finish)
diff --git a/lib/ddtrace/contrib/sinatra/tracer.rb b/lib/ddtrace/contrib/sinatra/tracer.rb
@@ -151,17 +151,7 @@ def render(engine, data, *)
               span.resource = "#{request.request_method} #{@datadog_route}"
               span.set_tag('sinatra.route.path', @datadog_route)
               span.set_tag(Datadog::Ext::HTTP::STATUS_CODE, response.status)
-
-              if response.server_error?
-                span.status = 1
-
-                err = env['sinatra.error']
-                if err
-                  span.set_tag(Datadog::Ext::Errors::TYPE, err.class)
-                  span.set_tag(Datadog::Ext::Errors::MSG, err.message)
-                end
-              end
-
+              span.set_error(env['sinatra.error']) if response.server_error?
               span.finish()
             ensure
               @datadog_request_span = nil
diff --git a/lib/ddtrace/error.rb b/lib/ddtrace/error.rb
@@ -0,0 +1,34 @@
+# Datadog global namespace
+module Datadog
+  # Error is a value-object responsible for sanitizing/encapsulating error data
+  class Error
+    attr_reader :type, :message, :backtrace
+
+    def self.build_from(value)
+      case value
+      when Error then value
+      when Array then new(*value)
+      when Exception then new(value.class, value.message, value.backtrace)
+      when Net::HTTPResponse then new(value.code, value.message)
+      end
+    end
+
+    def initialize(type, message, backtrace = [])
+      backtrace = Array(backtrace).join("\n")
+
+      @type = sanitize(type)
+      @message = sanitize(message)
+      @backtrace = sanitize(backtrace)
+    end
+
+    private
+
+    def sanitize(value)
+      value = value.to_s
+
+      return value if value.encoding == ::Encoding::UTF_8
+
+      value.encode(::Encoding::UTF_8)
+    end
+  end
+end
diff --git a/lib/ddtrace/ext/errors.rb b/lib/ddtrace/ext/errors.rb
@@ -1,6 +1,7 @@
 module Datadog
   module Ext
     module Errors
+      STATUS = 1
       MSG = 'error.msg'.freeze
       TYPE = 'error.type'.freeze
       STACK = 'error.stack'.freeze
diff --git a/lib/ddtrace/span.rb b/lib/ddtrace/span.rb
@@ -92,11 +92,14 @@ def get_metric(key)
 
     # Mark the span with the given error.
     def set_error(e)
-      return if e.nil?
-      @status = 1
-      @meta[Datadog::Ext::Errors::MSG] = e.message if e.respond_to?(:message) && e.message
-      @meta[Datadog::Ext::Errors::TYPE] = e.class.to_s
-      @meta[Datadog::Ext::Errors::STACK] = e.backtrace.join("\n") if e.respond_to?(:backtrace) && e.backtrace
+      e = Error.build_from(e)
+
+      return unless e
+
+      @status = Ext::Errors::STATUS
+      set_tag(Ext::Errors::TYPE, e.type)
+      set_tag(Ext::Errors::MSG, e.message)
+      set_tag(Ext::Errors::STACK, e.backtrace)
     end
 
     # Mark the span finished at the current time and submit it.
diff --git a/test/contrib/rails/controller_test.rb b/test/contrib/rails/controller_test.rb
@@ -108,7 +108,7 @@ class TracingControllerTest < ActionController::TestCase
     assert_equal(1, span.status, 'span should be flagged as an error')
     assert_equal('ZeroDivisionError', span.get_tag('error.type'), 'type should contain the class name of the error')
     assert_equal('divided by 0', span.get_tag('error.msg'), 'msg should state we tried to divided by 0')
-    assert_nil(span.get_tag('error.stack'))
+    assert_empty(span.get_tag('error.stack'))
   end
 
   test 'http error code should be trapped and reported as such, even with no exception' do
diff --git a/test/contrib/rails/rack_middleware_test.rb b/test/contrib/rails/rack_middleware_test.rb
@@ -87,7 +87,7 @@ class FullStackTest < ActionDispatch::IntegrationTest
     assert_equal(controller_span.status, 1)
     assert_equal(controller_span.get_tag('error.type'), 'ZeroDivisionError')
     assert_equal(controller_span.get_tag('error.msg'), 'divided by 0')
-    assert_nil(controller_span.get_tag('error.stack')) # error stack is in rack span
+    assert_empty(controller_span.get_tag('error.stack')) # error stack is in rack span
 
     assert_equal('rack.request', request_span.name)
     assert_equal(request_span.span_type, 'http')
@@ -141,7 +141,7 @@ class FullStackTest < ActionDispatch::IntegrationTest
     assert_equal(controller_span.status, 1)
     assert_equal(controller_span.get_tag('error.type'), 'ZeroDivisionError')
     assert_equal(controller_span.get_tag('error.msg'), 'divided by 0')
-    assert_nil(controller_span.get_tag('error.stack')) # error stack is in rack span
+    assert_empty(controller_span.get_tag('error.stack')) # error stack is in rack span
 
     assert_equal('rack.request', request_span.name)
     assert_equal(request_span.span_type, 'http')
diff --git a/test/error_test.rb b/test/error_test.rb
@@ -0,0 +1,58 @@
+require 'helper'
+require 'ddtrace/error'
+
+module Datadog
+  class ErrorTest < Minitest::Test
+    def setup
+      @error = Error.new('StandardError', 'message', %w[x y z])
+    end
+
+    def test_type
+      assert_equal('StandardError', @error.type)
+    end
+
+    def test_message
+      assert_equal('message', @error.message)
+    end
+
+    def test_backtrace
+      assert_equal("x\ny\nz", @error.backtrace)
+    end
+
+    def test_default_values
+      error = Error.new(nil, nil)
+
+      assert_equal('', error.type)
+      assert_equal('', error.message)
+      assert_equal('', error.backtrace)
+    end
+
+    # Empty strings were being interpreted as ASCII strings breaking `msgpack`
+    # decoding on the agent-side.
+    def test_enconding
+      error = Datadog::Error.new(nil, nil)
+
+      assert_equal(::Encoding::UTF_8, error.type.encoding)
+      assert_equal(::Encoding::UTF_8, error.message.encoding)
+      assert_equal(::Encoding::UTF_8, error.backtrace.encoding)
+    end
+
+    def test_array_coercion
+      error_payload = ['ZeroDivisionError', 'divided by 0']
+      error = Error.build_from(error_payload)
+
+      assert_equal('ZeroDivisionError', error.type)
+      assert_equal('divided by 0', error.message)
+      assert_equal('', error.backtrace)
+    end
+
+    def test_exception_coercion
+      exception = ZeroDivisionError.new('divided by 0')
+      error = Error.build_from(exception)
+
+      assert_equal('ZeroDivisionError', error.type)
+      assert_equal('divided by 0', error.message)
+      assert_equal('', error.backtrace)
+    end
+  end
+end
