Merge pull request #3789 from DataDog/dbm-tags

Steepfile

@@ -375,10 +375,6 @@ target :datadog do
   ignore 'lib/datadog/tracing/contrib/presto/instrumentation.rb'
   ignore 'lib/datadog/tracing/contrib/presto/integration.rb'
   ignore 'lib/datadog/tracing/contrib/presto/patcher.rb'
-  ignore 'lib/datadog/tracing/contrib/propagation/sql_comment.rb'
-  ignore 'lib/datadog/tracing/contrib/propagation/sql_comment/comment.rb'
-  ignore 'lib/datadog/tracing/contrib/propagation/sql_comment/ext.rb'
-  ignore 'lib/datadog/tracing/contrib/propagation/sql_comment/mode.rb'
   ignore 'lib/datadog/tracing/contrib/que/configuration/settings.rb'
   ignore 'lib/datadog/tracing/contrib/que/ext.rb'
   ignore 'lib/datadog/tracing/contrib/que/integration.rb'

lib/datadog/tracing/contrib/active_record/events/sql.rb

@@ -62,6 +62,7 @@ def on_start(span, event, _id, payload)
               cached = payload[:cached] || (payload[:name] == PAYLOAD_CACHE)
 
               span.set_tag(Ext::TAG_DB_VENDOR, adapter_name)
+              span.set_tag(Contrib::Ext::DB::TAG_INSTANCE, config[:database])
               span.set_tag(Ext::TAG_DB_NAME, config[:database])
               span.set_tag(Ext::TAG_DB_CACHED, cached) if cached
               span.set_tag(Tracing::Metadata::Ext::NET::TAG_TARGET_HOST, config[:host]) if config[:host]

lib/datadog/tracing/contrib/ext.rb

@@ -7,7 +7,21 @@ module Contrib
       module Ext
         # @public_api
         module DB
+          # Name of the database. This is *not* the database hostname.
+          #
+          # For databases which support such a concept, the default schema/database/namespace
+          # as configured in the connection string.
+          #
+          # If the tracer is already tracking changes to the default schema/database throughout the lifetime of
+          # the session (i.e. the client executes USE {NEW_SCHEMA} and now the default schema has changed from what
+          # was set upon connection initialization), then ideally this attribute reflects the “current” value.
+          # If the tracer is not already tracking changes then just leaving it to the default value set upon
+          # initialization is OK.
+          #
+          # This is the equivalent of OTel’s `db.namespace`
+          # @see https://opentelemetry.io/docs/specs/semconv/database/database-spans/#common-attributes
           TAG_INSTANCE = 'db.instance'
+
           TAG_USER = 'db.user'
           TAG_SYSTEM = 'db.system'
           TAG_STATEMENT = 'db.statement'

lib/datadog/tracing/contrib/mysql2/instrumentation.rb

@@ -50,6 +50,7 @@ def query(sql, options = {})
                 # Set analytics sample rate
                 Contrib::Analytics.set_sample_rate(span, analytics_sample_rate) if analytics_enabled?
 
+                span.set_tag(Contrib::Ext::DB::TAG_INSTANCE, query_options[:database])
                 span.set_tag(Ext::TAG_DB_NAME, query_options[:database])
                 span.set_tag(Tracing::Metadata::Ext::NET::TAG_TARGET_HOST, query_options[:host])
                 span.set_tag(Tracing::Metadata::Ext::NET::TAG_TARGET_PORT, query_options[:port])

lib/datadog/tracing/contrib/propagation/sql_comment.rb

@@ -22,13 +22,23 @@ def self.annotate!(span_op, mode)
           def self.prepend_comment(sql, span_op, trace_op, mode)
             return sql unless mode.enabled?
 
+            parent_service = datadog_configuration.service
+            peer_service = span_op.get_tag(Tracing::Metadata::Ext::TAG_PEER_SERVICE)
+
             tags = {
-              Ext::KEY_DATABASE_SERVICE => span_op.get_tag(Tracing::Metadata::Ext::TAG_PEER_SERVICE) || span_op.service,
               Ext::KEY_ENVIRONMENT => datadog_configuration.env,
-              Ext::KEY_PARENT_SERVICE => datadog_configuration.service,
-              Ext::KEY_VERSION => datadog_configuration.version
+              Ext::KEY_PARENT_SERVICE => parent_service,
+              Ext::KEY_VERSION => datadog_configuration.version,
+              Ext::KEY_HOSTNAME => span_op.get_tag(Tracing::Metadata::Ext::TAG_PEER_HOSTNAME),
+              Ext::KEY_DB_NAME => span_op.get_tag(Contrib::Ext::DB::TAG_INSTANCE),
+              Ext::KEY_PEER_SERVICE => peer_service,
             }
 
+            db_service = peer_service || span_op.service
+            if parent_service != db_service # Only set if it's different from parent_service; otherwise it's redundant
+              tags[Ext::KEY_DATABASE_SERVICE] = db_service
+            end
+
             if mode.full?
               # When tracing is disabled, trace_operation is a dummy object that does not contain data to build traceparent
               if datadog_configuration.tracing.enabled

lib/datadog/tracing/contrib/propagation/sql_comment/ext.rb

@@ -20,10 +20,38 @@ module Ext
             # The value should be `true` when `full` mode
             TAG_DBM_TRACE_INJECTED = '_dd.dbm_trace_injected'
 
+            # Database service/sql span service (i.e. the service executing the actual query)
+            #
+            # If fake services are disabled:
+            #   This value will be the same as the parent service
+            #
+            # If fake services are enabled:
+            #   This value is NOT the same as the parent service
+            #
+            # This should NOT be overridden by peer.service.
             KEY_DATABASE_SERVICE = 'dddbs'
+
+            # The global service environment (e.g. DD_ENV)
             KEY_ENVIRONMENT = 'dde'
+
+            # The global service name (e.g. DD_SERVICE)
             KEY_PARENT_SERVICE = 'ddps'
+
+            # The global service version (e.g. DD_VERSION)
             KEY_VERSION = 'ddpv'
+
+            # The hostname of the database server, as provided to the database client upon instantiation.
+            # @see Datadog::Tracing::Metadata::Ext::TAG_PEER_HOSTNAME
+            KEY_HOSTNAME = 'ddh'
+
+            # @see Datadog::Tracing::Contrib::Ext::DB::TAG_INSTANCE
+            KEY_DB_NAME = 'dddb'
+
+            # Users can use this attribute to specify the identity of the dependency/database they are connecting to.
+            # We should grab this attribute only if the user is EXPLICITLY specifying it.
+            # @see Datadog::Tracing::Metadata::Ext::TAG_PEER_SERVICE
+            KEY_PEER_SERVICE = 'ddprs'
+
             KEY_TRACEPARENT = 'traceparent'
           end
         end

lib/datadog/tracing/contrib/trilogy/instrumentation.rb

@@ -49,6 +49,7 @@ def query(sql)
                 # Set analytics sample rate
                 Contrib::Analytics.set_sample_rate(span, analytics_sample_rate) if analytics_enabled?
 
+                span.set_tag(Contrib::Ext::DB::TAG_INSTANCE, connection_options[:database])
                 span.set_tag(Ext::TAG_DB_NAME, connection_options[:database])
                 span.set_tag(Tracing::Metadata::Ext::NET::TAG_TARGET_HOST, connection_options[:host])
                 span.set_tag(Tracing::Metadata::Ext::NET::TAG_TARGET_PORT, connection_options[:port])

lib/datadog/tracing/metadata/ext.rb

@@ -14,6 +14,10 @@ module Ext
         # Type of operation being performed (e.g. )
         TAG_OPERATION = 'operation'
         # Hostname of external service interacted with
+        #
+        # This tag also doesn't strictly need to be a “hostname”. It can be a raw IP address and in some cases it
+        # can even be a unix domain socket (i.e. postgres client setting host=/var/run/postgres).
+        # It should be whatever the client uses to point at the server it’s trying to talk to.
         TAG_PEER_HOSTNAME = 'peer.hostname'
         # Name of external service that performed the work
         TAG_PEER_SERVICE = 'peer.service'

sig/datadog/tracing/contrib/ext.rbs

@@ -6,6 +6,7 @@ module Datadog
           PEER_SERVICE_SOURCES: Array[string]
           TAG_INSTANCE: "db.instance"
 
+          TAG_PEER_DB_NAME: string
           TAG_USER: "db.user"
 
           TAG_SYSTEM: "db.system"

sig/datadog/tracing/contrib/propagation/sql_comment.rbs

@@ -4,7 +4,7 @@ module Datadog
       module Propagation
         module SqlComment
           def self.annotate!: (untyped span_op, untyped mode) -> (nil | untyped)
-          def self.prepend_comment: (untyped sql, untyped span_op, untyped trace_op, untyped mode) -> (untyped | ::String)
+          def self.prepend_comment: (String sql, SpanOperation span_op, TraceOperation trace_op, untyped mode) -> String
 
           def self.datadog_configuration: () -> untyped
         end

sig/datadog/tracing/contrib/propagation/sql_comment/ext.rbs

@@ -6,6 +6,9 @@ module Datadog
           module Ext
             ENV_DBM_PROPAGATION_MODE: "DD_DBM_PROPAGATION_MODE"
             DISABLED: "disabled"
+            KEY_DB_NAME: string
+            KEY_HOSTNAME: string
+            KEY_PEER_SERVICE: string
             SERVICE: "service"
             FULL: "full"
             TAG_DBM_TRACE_INJECTED: "_dd.dbm_trace_injected"

sig/datadog/tracing/contrib/propagation/sql_comment/mode.rbs

@@ -3,7 +3,13 @@ module Datadog
     module Contrib
       module Propagation
         module SqlComment
-          Mode: untyped
+          class Mode < ::Struct[String]
+            attr_accessor mode: String
+
+            def enabled?: -> bool
+            def service?: -> bool
+            def full?: -> bool
+          end
         end
       end
     end

spec/datadog/tracing/contrib/active_record/tracer_spec.rb

@@ -48,6 +48,7 @@
       expect(span.type).to eq('sql')
       expect(span.resource.strip).to eq('SELECT COUNT(*) FROM `articles`')
       expect(span.get_tag('active_record.db.vendor')).to eq('mysql2')
+      expect(span.get_tag('db.instance')).to eq('mysql')
       expect(span.get_tag('active_record.db.name')).to eq('mysql')
       expect(span.get_tag('active_record.db.cached')).to eq(nil)
       expect(span.get_tag('out.host')).to eq(ENV.fetch('TEST_MYSQL_HOST', '127.0.0.1'))

spec/datadog/tracing/contrib/mysql2/patcher_spec.rb

@@ -99,6 +99,7 @@
 
           expect(spans.count).to eq(1)
           expect(span.get_tag('span.kind')).to eq('client')
+          expect(span.get_tag('db.instance')).to eq(database)
           expect(span.get_tag('mysql2.db.name')).to eq(database)
           expect(span.get_tag('out.host')).to eq(host)
           expect(span.get_tag('out.port')).to eq(port)

spec/datadog/tracing/contrib/propagation/sql_comment_spec.rb

@@ -5,7 +5,7 @@
   let(:propagation_mode) { Datadog::Tracing::Contrib::Propagation::SqlComment::Mode.new(mode) }
 
   describe '.annotate!' do
-    let(:span_op) { Datadog::Tracing::SpanOperation.new('sql_comment_propagation_span', service: 'database_service') }
+    let(:span_op) { Datadog::Tracing::SpanOperation.new('sql_comment_propagation_span', service: 'db_service') }
 
     context 'when `disabled` mode' do
       let(:mode) { 'disabled' }
@@ -50,16 +50,16 @@
     context 'when tracing is enabled' do
       before do
         Datadog.configure do |c|
-          c.env = 'production'
-          c.service = "Traders' Joe"
-          c.version = '1.0.0'
+          c.env = 'dev'
+          c.service = 'api'
+          c.version = '1.2'
         end
       end
 
       let(:span_op) do
         Datadog::Tracing::SpanOperation.new(
           'sample_span',
-          service: 'database_service'
+          service: 'db_service'
         )
       end
       let(:trace_op) do
@@ -85,22 +85,70 @@
 
         it do
           is_expected.to eq(
-            "/*dddbs='database_service',dde='production',ddps='Traders%27%20Joe',ddpv='1.0.0'*/ #{sql_statement}"
+            "/*dde='dev',ddps='api',ddpv='1.2',dddbs='db_service'*/ #{sql_statement}"
           )
         end
 
         context 'when given a span operation tagged with peer.service' do
           let(:span_op) do
             Datadog::Tracing::SpanOperation.new(
               'sample_span',
-              service: 'database_service',
-              tags: { 'peer.service' => 'sample_peer_service' }
+              service: 'db_service',
+              tags: { 'peer.service' => 'db_peer_service' }
             )
           end
 
           it do
             is_expected.to eq(
-              "/*dddbs='sample_peer_service',dde='production',ddps='Traders%27%20Joe',ddpv='1.0.0'*/ #{sql_statement}"
+              "/*dde='dev',ddps='api',ddpv='1.2',ddprs='db_peer_service',dddbs='db_peer_service'*/ #{sql_statement}"
+            )
+          end
+
+          context 'matching the global service' do
+            let(:span_op) do
+              Datadog::Tracing::SpanOperation.new(
+                'sample_span',
+                service: 'db_service',
+                tags: { 'peer.service' => 'api' }
+              )
+            end
+
+            it 'omits the redundant dddbs' do
+              is_expected.to eq(
+                "/*dde='dev',ddps='api',ddpv='1.2',ddprs='api'*/ #{sql_statement}"
+              )
+            end
+          end
+        end
+
+        context 'when given a span operation tagged with db.instance' do
+          let(:span_op) do
+            Datadog::Tracing::SpanOperation.new(
+              'sample_span',
+              service: 'db_service',
+              tags: { 'db.instance' => 'db_name' }
+            )
+          end
+
+          it do
+            is_expected.to eq(
+              "/*dde='dev',ddps='api',ddpv='1.2',dddb='db_name',dddbs='db_service'*/ #{sql_statement}"
+            )
+          end
+        end
+
+        context 'when given a span operation tagged with peer.hostname' do
+          let(:span_op) do
+            Datadog::Tracing::SpanOperation.new(
+              'sample_span',
+              service: 'db_service',
+              tags: { 'peer.hostname' => 'db_host' }
+            )
+          end
+
+          it do
+            is_expected.to eq(
+              "/*dde='dev',ddps='api',ddpv='1.2',ddh='db_host',dddbs='db_service'*/ #{sql_statement}"
             )
           end
         end
@@ -112,10 +160,11 @@
 
         it {
           is_expected.to eq(
-            "/*dddbs='database_service',"\
-            "dde='production',"\
-            "ddps='Traders%27%20Joe',"\
-            "ddpv='1.0.0',"\
+            '/*'\
+            "dde='dev',"\
+            "ddps='api',"\
+            "ddpv='1.2',"\
+            "dddbs='db_service',"\
             "traceparent='#{traceparent}'*/ "\
             "#{sql_statement}"
           )
@@ -125,17 +174,19 @@
           let(:span_op) do
             Datadog::Tracing::SpanOperation.new(
               'sample_span',
-              service: 'database_service',
-              tags: { 'peer.service' => 'sample_peer_service' }
+              service: 'db_service',
+              tags: { 'peer.service' => 'db_peer_service' }
             )
           end
 
           it {
             is_expected.to eq(
-              "/*dddbs='sample_peer_service',"\
-              "dde='production',"\
-              "ddps='Traders%27%20Joe',"\
-              "ddpv='1.0.0',"\
+              '/*'\
+              "dde='dev',"\
+              "ddps='api',"\
+              "ddpv='1.2',"\
+              "ddprs='db_peer_service',"\
+              "dddbs='db_peer_service',"\
               "traceparent='#{traceparent}'*/ "\
               "#{sql_statement}"
             )
@@ -147,9 +198,9 @@
     describe 'when propagates with `full` mode but tracing is disabled ' do
       before do
         Datadog.configure do |c|
-          c.env = 'production'
-          c.service = "Traders' Joe"
-          c.version = '1.0.0'
+          c.env = 'dev'
+          c.service = 'api'
+          c.version = '1.2'
           c.tracing.enabled = false
         end
       end
@@ -160,7 +211,7 @@
         result = nil
 
         Datadog::Tracing.trace('dummy.sql') do |span_op, trace_op|
-          span_op.service = 'database_service'
+          span_op.service = 'db_service'
 
           result = described_class.prepend_comment(sql_statement, span_op, trace_op, propagation_mode)
         end
@@ -170,7 +221,7 @@
 
       it do
         is_expected.to eq(
-          "/*dddbs='database_service',dde='production',ddps='Traders%27%20Joe',ddpv='1.0.0'*/ #{sql_statement}"
+          "/*dde='dev',ddps='api',ddpv='1.2',dddbs='db_service'*/ #{sql_statement}"
         )
       end
 

spec/datadog/tracing/contrib/rails/database_spec.rb

@@ -45,6 +45,7 @@
       expect(span.type).to eq('sql')
       expect(span.service).to eq(adapter_name)
       expect(span.get_tag('active_record.db.vendor')).to eq(adapter_name)
+      expect(span.get_tag('db.instance')).to eq(database_name)
       expect(span.get_tag('active_record.db.name')).to eq(database_name)
       expect(span.get_tag('active_record.db.cached')).to be_nil
       expect(adapter_host.to_s).to eq(span.get_tag('out.host'))

spec/datadog/tracing/contrib/trilogy/patcher_spec.rb

@@ -85,6 +85,7 @@
 
           expect(spans.count).to eq(1)
           expect(span.get_tag('span.kind')).to eq('client')
+          expect(span.get_tag('db.instance')).to eq(database)
           expect(span.get_tag('trilogy.db.name')).to eq(database)
           expect(span.get_tag('out.host')).to eq(host)
           expect(span.get_tag('out.port')).to eq(port.to_f)