diff --git a/lib/ddtrace/contrib/active_record/patcher.rb b/lib/ddtrace/contrib/active_record/patcher.rb index fb796ecdfaf..3d9a64c2f39 100644 --- a/lib/ddtrace/contrib/active_record/patcher.rb +++ b/lib/ddtrace/contrib/active_record/patcher.rb @@ -80,12 +80,18 @@ def self.sql(_name, start, finish, _id, payload) span_type: span_type ) + # Find out if the SQL query has been cached in this request. This meta is really + # helpful to users because some spans may have 0ns of duration because the query + # is simply cached from memory, so the notification is fired with start == finish. + cached = payload[:cached] || (payload[:name] == 'CACHE') + # the span should have the query ONLY in the Resource attribute, # so that the ``sql.query`` tag will be set in the agent with an # obfuscated version span.span_type = Datadog::Ext::SQL::TYPE span.set_tag('active_record.db.vendor', adapter_name) span.set_tag('active_record.db.name', database_name) + span.set_tag('active_record.db.cached', cached) if cached span.set_tag('out.host', adapter_host) span.set_tag('out.port', adapter_port) span.start_time = start diff --git a/lib/ddtrace/contrib/rails/active_record.rb b/lib/ddtrace/contrib/rails/active_record.rb index 6a90d73ec13..b8c2af1053e 100644 --- a/lib/ddtrace/contrib/rails/active_record.rb +++ b/lib/ddtrace/contrib/rails/active_record.rb @@ -33,13 +33,10 @@ def self.sql(_name, start, finish, _id, payload) span_type: span_type ) - # find out if the SQL query has been cached in this request. This meta is really + # Find out if the SQL query has been cached in this request. This meta is really # helpful to users because some spans may have 0ns of duration because the query # is simply cached from memory, so the notification is fired with start == finish. - # TODO[manu]: this feature has been merged into master but has not yet released. - # We're supporting this action as a best effort, but we should add a test after - # a new version of Rails is out. - cached = payload[:cached] + cached = payload[:cached] || (payload[:name] == 'CACHE') # the span should have the query ONLY in the Resource attribute, # so that the ``sql.query`` tag will be set in the agent with an diff --git a/test/contrib/rails/database_test.rb b/test/contrib/rails/database_test.rb index 32251b7eb94..9524756e0a3 100644 --- a/test/contrib/rails/database_test.rb +++ b/test/contrib/rails/database_test.rb @@ -37,6 +37,25 @@ class DatabaseTracingTest < ActiveSupport::TestCase assert_nil(span.get_tag('sql.query')) end + test 'active record is sets cached tag' do + # Make sure query caching is enabled... + Article.cache do + # Do two queries (second should cache.) + Article.count + Article.count + + # Assert correct number of spans + spans = @tracer.writer.spans + assert_equal(spans.length, 2) + + # Assert cached flag not present on first query + assert_nil(spans.first.get_tag('rails.db.cached')) + + # Assert cached flag set correctly on second query + assert_equal('true', spans.last.get_tag('rails.db.cached')) + end + end + test 'doing a database call uses the proper service name if it is changed' do # update database configuration update_config(:database_service, 'customer-db') diff --git a/test/contrib/sinatra/tracer_activerecord_test.rb b/test/contrib/sinatra/tracer_activerecord_test.rb index ac9531d4904..a1646d60c61 100644 --- a/test/contrib/sinatra/tracer_activerecord_test.rb +++ b/test/contrib/sinatra/tracer_activerecord_test.rb @@ -4,12 +4,27 @@ require 'contrib/sinatra/tracer_test_base' class TracerActiveRecordTest < TracerTestBase + class ApplicationRecord < ActiveRecord::Base + self.abstract_class = true + end + + class Article < ApplicationRecord + end + class TracerActiveRecordTestApp < Sinatra::Application post '/request' do conn = settings.datadog_test_conn conn.connection.execute('SELECT 42') '' end + + post '/cached_request' do + Article.cache do + # Do two queries (second should cache.) + Article.count + Article.count + end + end end def app @@ -32,6 +47,18 @@ def setup super end + def migrate_db + Article.exists? + rescue ActiveRecord::StatementInvalid + ActiveRecord::Schema.define(version: 20180101000000) do + create_table 'articles', force: :cascade do |t| + t.string 'title' + t.datetime 'created_at', null: false + t.datetime 'updated_at', null: false + end + end + end + def test_request post '/request' assert_equal(200, last_response.status) @@ -66,4 +93,30 @@ def test_request assert_equal(0, sinatra_span.status) assert_nil(sinatra_span.parent) end + + # Testing AR query caching requires use of a model. + # Create a model, query it a few times, make sure cached tag gets set. + def test_cached_tag + # Make sure Article table exists + migrate_db + + # Do query with cached query + post '/cached_request' + + # Assert correct number of spans (ignoring transactions, etc.) + spans = all_spans.select { |s| s.resource.include?('SELECT COUNT(*) FROM "articles"') } + assert_equal(2, spans.length) + + # Assert cached flag not present on first query + assert_nil(spans.first.get_tag('active_record.db.cached')) + + # Assert cached flag set correctly on second query + assert_equal('true', spans.last.get_tag('active_record.db.cached')) + end + + private + + def all_spans + @writer.spans(:keep) + end end