Add ARM/DBM peer entity tags

Signed-off-by: Marco Costa <marco.costa@datadoghq.com>

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/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(Tracing::Metadata::Ext::TAG_PEER_DB_NAME),
+              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,49 @@ 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.
+            #
+            # 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.
+            KEY_HOSTNAME = 'ddh'
+
+            # 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.name`
+            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.
+            KEY_PEER_SERVICE = 'ddprs'
+
             KEY_TRACEPARENT = 'traceparent'
           end
         end

lib/datadog/tracing/metadata/ext.rb

@@ -17,6 +17,9 @@ module Ext
         TAG_PEER_HOSTNAME = 'peer.hostname'
         # Name of external service that performed the work
         TAG_PEER_SERVICE = 'peer.service'
+        # Name of the database. This is *not* the database hostname.
+        # This attribute used to be called `db.instance`.
+        TAG_PEER_DB_NAME = 'peer.db.name'
 
         TAG_KIND = 'span.kind'
 

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

sig/datadog/tracing/metadata/ext.rbs

@@ -4,6 +4,7 @@ module Datadog
       module Ext
         TAG_COMPONENT: ::String
         TAG_OPERATION: ::String
+        TAG_PEER_DB_NAME: ::String
         TAG_PEER_HOSTNAME: ::String
         TAG_PEER_SERVICE: ::String
         TAG_KIND: ::String

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 peer.db.name' do
+          let(:span_op) do
+            Datadog::Tracing::SpanOperation.new(
+              'sample_span',
+              service: 'db_service',
+              tags: { 'peer.db.name' => '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