Merge pull request #25 from palazzem/sql-bugfix

[rails] improved support for ActiveRecord

docs/GettingStarted

@@ -148,6 +148,8 @@ The available settings are:
   only particular functions or views
 * +default_service+: set the service name used by the tracer. Usually this configuration must be updated with
   a meaningful name
+* +default_database_service+: set the database service name used by the tracer. By default the tracer uses the
+  configured adapter name, so if you're using PostgreSQL it will be simply +postgres+.
 * +template_base_path+: used when the template name is parsed in the auto instrumented code. If you don't store
   your templates in the +views/+ folder, you may need to change this value
 * +tracer+: is the global tracer used by the tracing application. Usually you don't need to change that value

lib/ddtrace/contrib/rails/active_record.rb

@@ -16,11 +16,18 @@ def self.instrument
 
         def self.sql(_name, start, finish, _id, payload)
           tracer = ::Rails.configuration.datadog_trace.fetch(:tracer)
+          database_service = ::Rails.configuration.datadog_trace.fetch(:default_database_service)
           adapter_name = ::ActiveRecord::Base.connection_config[:adapter]
           adapter_name = Datadog::Contrib::Rails::Utils.normalize_vendor(adapter_name)
           span_type = Datadog::Ext::SQL::TYPE
 
-          span = tracer.trace("#{adapter_name}.query", service: adapter_name, span_type: span_type)
+          span = tracer.trace(
+            "#{adapter_name}.query",
+            resource: payload.fetch(:sql),
+            service: database_service,
+            span_type: span_type
+          )
+
           span.span_type = Datadog::Ext::SQL::TYPE
           span.set_tag(Datadog::Ext::SQL::QUERY, payload.fetch(:sql))
           span.set_tag('rails.db.vendor', adapter_name)

lib/ddtrace/contrib/rails/framework.rb

@@ -6,6 +6,7 @@
 require 'ddtrace/contrib/rails/action_view'
 require 'ddtrace/contrib/rails/active_record'
 require 'ddtrace/contrib/rails/active_support'
+require 'ddtrace/contrib/rails/utils'
 
 module Datadog
   module Contrib
@@ -36,6 +37,17 @@ def self.configure(config)
             Datadog::Ext::AppTypes::WEB
           )
 
+          # set default database service details and store it in the configuration
+          adapter_name = ::ActiveRecord::Base.connection_config[:adapter]
+          adapter_name = Datadog::Contrib::Rails::Utils.normalize_vendor(adapter_name)
+          database_service = datadog_config.fetch(:default_database_service, adapter_name)
+          datadog_config[:default_database_service] = database_service
+          datadog_config[:tracer].set_service_info(
+            database_service,
+            adapter_name,
+            Datadog::Ext::AppTypes::DB
+          )
+
           # update global configurations
           ::Rails.configuration.datadog_trace = datadog_config
         end

test/contrib/rails/database_test.rb

@@ -21,9 +21,26 @@ class DatabaseTracingTest < ActiveSupport::TestCase
     span = spans[-1]
     assert_equal(span.name, 'postgres.query')
     assert_equal(span.span_type, 'sql')
+    assert_includes(span.resource, 'SELECT COUNT(*) FROM')
     assert_equal(span.service, 'postgres')
     assert_equal(span.get_tag('rails.db.vendor'), 'postgres')
-    assert_includes(span.get_tag('sql.query'), 'SELECT COUNT(*)')
+    assert_includes(span.get_tag('sql.query'), 'SELECT COUNT(*) FROM')
     assert span.to_hash[:duration] > 0
   end
+
+  test 'doing a database call uses the proper service name if it is changed' do
+    # update database configuration
+    update_config(:default_database_service, 'customer-db')
+
+    # make the query and assert the proper spans
+    Article.count
+    spans = @tracer.writer.spans()
+    assert_equal(spans.length, 1)
+
+    span = spans[-1]
+    assert_equal(span.service, 'customer-db')
+
+    # reset the original configuration
+    reset_config()
+  end
 end

test/contrib/rails/tracer_test.rb

@@ -3,6 +3,16 @@
 require 'contrib/rails/test_helper'
 
 class TracerTest < ActionController::TestCase
+  setup do
+    # don't pollute the global tracer
+    @tracer = get_test_tracer
+    Rails.configuration.datadog_trace[:tracer] = @tracer
+  end
+
+  teardown do
+    reset_config()
+  end
+
   test 'the configuration is correctly called' do
     assert Rails.configuration.datadog_trace[:enabled]
     assert Rails.configuration.datadog_trace[:auto_instrument]
@@ -11,9 +21,46 @@ class TracerTest < ActionController::TestCase
     assert Rails.configuration.datadog_trace[:tracer]
   end
 
-  test 'a default service is properly set' do
-    Datadog::Contrib::Rails::Framework.configure({})
+  test 'a default service and database should be properly set' do
+    tracer = Datadog.tracer
+    assert_equal(
+      tracer.services,
+      'rails-app' => {
+        'app' => 'rails', 'app_type' => 'web'
+      },
+      'postgres' => {
+        'app' => 'postgres', 'app_type' => 'db'
+      }
+    )
+  end
+
+  test 'database service can be changed by user' do
+    update_config(:default_database_service, 'customer-db')
     tracer = Rails.configuration.datadog_trace[:tracer]
-    assert_equal(tracer.services, 'rails-app' => { 'app' => 'rails', 'app_type' => 'web' })
+
+    assert_equal(
+      tracer.services,
+      'rails-app' => {
+        'app' => 'rails', 'app_type' => 'web'
+      },
+      'customer-db' => {
+        'app' => 'postgres', 'app_type' => 'db'
+      }
+    )
+  end
+
+  test 'application service can be changed by user' do
+    update_config(:default_service, 'my-custom-app')
+    tracer = Rails.configuration.datadog_trace[:tracer]
+
+    assert_equal(
+      tracer.services,
+      'my-custom-app' => {
+        'app' => 'rails', 'app_type' => 'web'
+      },
+      'postgres' => {
+        'app' => 'postgres', 'app_type' => 'db'
+      }
+    )
   end
 end

test/helper.rb

@@ -41,3 +41,20 @@ def send(*)
     # noop
   end
 end
+
+# update Datadog user configuration; you should pass:
+#
+# * +key+: the key that should be updated
+# * +value+: the value of the key
+def update_config(key, value)
+  ::Rails.configuration.datadog_trace[key] = value
+  config = { config: ::Rails.application.config }
+  Datadog::Contrib::Rails::Framework.configure(config)
+end
+
+# reset default configuration and replace any dummy tracer
+# with the global one
+def reset_config
+  ::Rails.configuration.datadog_trace = {}
+  Datadog::Contrib::Rails::Framework.configure({})
+end