Merge pull request #181 from DataDog/pedro/fix-error-handling

Add `Datadog::Error` value-object

Author: p-lambert

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

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

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)

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)

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

lib/ddtrace/error.rb

@@ -0,0 +1,37 @@
+# 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 ContainsMessage then new(value.class, value.message)
+      else BlankError
+      end
+    end
+
+    def initialize(type = nil, message = nil, backtrace = nil)
+      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
+
+    BlankError = Error.new
+    ContainsMessage = ->(v) { v.respond_to?(:message) }
+  end
+end

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

lib/ddtrace/span.rb

@@ -92,11 +92,12 @@ 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)
+
+      @status = Ext::Errors::STATUS
+      set_tag(Ext::Errors::TYPE, e.type) unless e.type.empty?
+      set_tag(Ext::Errors::MSG, e.message) unless e.message.empty?
+      set_tag(Ext::Errors::STACK, e.backtrace) unless e.backtrace.empty?
     end
 
     # Mark the span finished at the current time and submit it.

test/error_test.rb

@@ -0,0 +1,77 @@
+require 'helper'
+require 'ddtrace/error'
+
+module Datadog
+  class ErrorTest < Minitest::Test
+    CustomMessage = Struct.new(:message)
+
+    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
+
+      assert_empty(error.type)
+      assert_empty(error.message)
+      assert_empty(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
+
+      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_empty(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_empty(error.backtrace)
+    end
+
+    def test_message_coercion
+      message = CustomMessage.new('custom-message')
+      error = Error.build_from(message)
+
+      assert_equal('Datadog::ErrorTest::CustomMessage', error.type)
+      assert_equal('custom-message', error.message)
+      assert_empty(error.backtrace)
+    end
+
+    def test_nil_coercion
+      error = Error.build_from(nil)
+
+      assert_empty(error.type)
+      assert_empty(error.message)
+      assert_empty(error.backtrace)
+    end
+  end
+end