From 3fb522ce82831f597e83f5ec680772e6bd850068 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 12 Jul 2024 14:44:34 -0700 Subject: [PATCH] Add ARM/DBM peer entity tags Signed-off-by: Marco Costa --- Steepfile | 4 - .../contrib/propagation/sql_comment.rb | 16 ++- .../contrib/propagation/sql_comment/ext.rb | 39 ++++++++ lib/datadog/tracing/metadata/ext.rb | 3 + .../contrib/propagation/sql_comment.rbs | 2 +- .../contrib/propagation/sql_comment/ext.rbs | 3 + .../contrib/propagation/sql_comment/mode.rbs | 8 +- sig/datadog/tracing/metadata/ext.rbs | 1 + .../contrib/propagation/sql_comment_spec.rb | 99 ++++++++++++++----- 9 files changed, 142 insertions(+), 33 deletions(-) diff --git a/Steepfile b/Steepfile index 83d86357de3..02fb04de0e0 100644 --- a/Steepfile +++ b/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' diff --git a/lib/datadog/tracing/contrib/propagation/sql_comment.rb b/lib/datadog/tracing/contrib/propagation/sql_comment.rb index ee32914b0d1..e2d90c663a1 100644 --- a/lib/datadog/tracing/contrib/propagation/sql_comment.rb +++ b/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 diff --git a/lib/datadog/tracing/contrib/propagation/sql_comment/ext.rb b/lib/datadog/tracing/contrib/propagation/sql_comment/ext.rb index 3d0325f30a5..5ac4ebf69a6 100644 --- a/lib/datadog/tracing/contrib/propagation/sql_comment/ext.rb +++ b/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 diff --git a/lib/datadog/tracing/metadata/ext.rb b/lib/datadog/tracing/metadata/ext.rb index 0034cd77a46..a8d8cde8f73 100644 --- a/lib/datadog/tracing/metadata/ext.rb +++ b/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' diff --git a/sig/datadog/tracing/contrib/propagation/sql_comment.rbs b/sig/datadog/tracing/contrib/propagation/sql_comment.rbs index b803269f682..1f28f3526dc 100644 --- a/sig/datadog/tracing/contrib/propagation/sql_comment.rbs +++ b/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 diff --git a/sig/datadog/tracing/contrib/propagation/sql_comment/ext.rbs b/sig/datadog/tracing/contrib/propagation/sql_comment/ext.rbs index 2f2affe407f..bb75fdbde97 100644 --- a/sig/datadog/tracing/contrib/propagation/sql_comment/ext.rbs +++ b/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" diff --git a/sig/datadog/tracing/contrib/propagation/sql_comment/mode.rbs b/sig/datadog/tracing/contrib/propagation/sql_comment/mode.rbs index 50b5df36f43..1f2f2d80bb7 100644 --- a/sig/datadog/tracing/contrib/propagation/sql_comment/mode.rbs +++ b/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 diff --git a/sig/datadog/tracing/metadata/ext.rbs b/sig/datadog/tracing/metadata/ext.rbs index aeca3b827a1..742e225a258 100644 --- a/sig/datadog/tracing/metadata/ext.rbs +++ b/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 diff --git a/spec/datadog/tracing/contrib/propagation/sql_comment_spec.rb b/spec/datadog/tracing/contrib/propagation/sql_comment_spec.rb index 24349aaa5d5..df5aa435007 100644 --- a/spec/datadog/tracing/contrib/propagation/sql_comment_spec.rb +++ b/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,7 +85,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 @@ -93,14 +93,62 @@ 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