From 4046576782dd10e59d34ba8c3ea124da5ec20a95 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 24 Feb 2025 13:32:57 +0100 Subject: [PATCH 01/21] Fix apm disablement tag when appsec is disabled --- .github/forced-tests-list.json | 17 +++++++++++++++-- .github/workflows/system-tests.yml | 3 +++ .../appsec/contrib/rack/request_middleware.rb | 2 -- lib/datadog/appsec/ext.rb | 1 - lib/datadog/tracing/metadata/ext.rb | 2 ++ lib/datadog/tracing/trace_operation.rb | 4 +++- lib/datadog/tracing/trace_segment.rb | 7 +++++-- lib/datadog/tracing/tracer.rb | 6 ++++++ .../tracing/transport/trace_formatter.rb | 7 +++++++ sig/datadog/tracing/metadata/ext.rbs | 1 + sig/datadog/tracing/trace_operation.rbs | 2 +- sig/datadog/tracing/trace_segment.rbs | 4 +++- 12 files changed, 46 insertions(+), 10 deletions(-) diff --git a/.github/forced-tests-list.json b/.github/forced-tests-list.json index 077404aaa41..098be4f2815 100644 --- a/.github/forced-tests-list.json +++ b/.github/forced-tests-list.json @@ -1,3 +1,16 @@ { - -} \ No newline at end of file + "AGENT_NOT_SUPPORTING_SPAN_EVENTS": + [ + "tests/test_span_events.py" + ], + "PARAMETRIC": + [ + "tests/parametric/test_span_events.py" + ], + "DEFAULT": [ + "tests/test_graphql.py" + ], + "SCA_STANDALONE": [ + "tests/appsec/test_asm_standalone.py::Test_SCAStandalone_Telemetry" + ] +} diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 48764276f43..93fe0aad31e 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -320,6 +320,9 @@ jobs: - library: ruby app: rails70 scenario: DEBUGGER_PII_REDACTION + - library: ruby + app: rails70 + scenario: SCA_STANDALONE - library: ruby app: rack scenario: SAMPLING diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index c0d782a8d11..944a9994f88 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_middleware.rb @@ -147,8 +147,6 @@ def add_appsec_tags(processor, context) return unless trace && span span.set_metric(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1) - # We add this tag when ASM standalone is enabled to make sure we don't bill APM - span.set_metric(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled span.set_tag('_dd.runtime_family', 'ruby') span.set_tag('_dd.appsec.waf.version', Datadog::AppSec::WAF::VERSION::BASE_STRING) diff --git a/lib/datadog/appsec/ext.rb b/lib/datadog/appsec/ext.rb index 6c76e708ad1..6b76b36e0a2 100644 --- a/lib/datadog/appsec/ext.rb +++ b/lib/datadog/appsec/ext.rb @@ -12,7 +12,6 @@ module Ext ACTIVE_CONTEXT_KEY = :datadog_appsec_active_context TAG_APPSEC_ENABLED = '_dd.appsec.enabled' - TAG_APM_ENABLED = '_dd.apm.enabled' TAG_DISTRIBUTED_APPSEC_EVENT = '_dd.p.appsec' TELEMETRY_METRICS_NAMESPACE = 'appsec' diff --git a/lib/datadog/tracing/metadata/ext.rb b/lib/datadog/tracing/metadata/ext.rb index 95cf3d4e7f0..9ec9de0f77d 100644 --- a/lib/datadog/tracing/metadata/ext.rb +++ b/lib/datadog/tracing/metadata/ext.rb @@ -31,6 +31,8 @@ module Ext # See Datadog-internal "RFC: Identifying which spans have profiling enabled " for details TAG_PROFILING_ENABLED = '_dd.profiling.enabled' + TAG_APM_ENABLED = '_dd.apm.enabled' + # Defines constants for trace analytics # @public_api module Analytics diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index eb85182af75..fac663b4c58 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -71,13 +71,13 @@ def initialize( sampling_priority: nil, service: nil, profiling_enabled: nil, + apm_tracing_disabled: nil, tags: nil, metrics: nil, trace_state: nil, trace_state_unknown_fields: nil, remote_parent: false, tracer: nil - ) # Attributes @id = id || Tracing::Utils::TraceId.next_id @@ -98,6 +98,7 @@ def initialize( @sampling_priority = sampling_priority @service = service @profiling_enabled = profiling_enabled + @apm_tracing_disabled = apm_tracing_disabled @trace_state = trace_state @trace_state_unknown_fields = trace_state_unknown_fields @tracer = tracer @@ -510,6 +511,7 @@ def build_trace(spans, partial = false) metrics: metrics, root_span_id: !partial ? root_span && root_span.id : nil, profiling_enabled: @profiling_enabled, + apm_tracing_disabled: @apm_tracing_disabled ) end diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 6d0903d9a9d..24cb7affa28 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -34,7 +34,8 @@ class TraceSegment :sampling_decision_maker, :sampling_priority, :service, - :profiling_enabled + :profiling_enabled, + :apm_tracing_disabled # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity @@ -58,7 +59,8 @@ def initialize( service: nil, tags: nil, metrics: nil, - profiling_enabled: nil + profiling_enabled: nil, + apm_tracing_disabled: nil ) @id = id @root_span_id = root_span_id @@ -85,6 +87,7 @@ def initialize( @sampling_priority = sampling_priority || sampling_priority_tag @service = Core::Utils::SafeDup.frozen_or_dup(service || service_tag) @profiling_enabled = profiling_enabled + @apm_tracing_disabled = apm_tracing_disabled end # rubocop:enable Metrics/PerceivedComplexity # rubocop:enable Metrics/CyclomaticComplexity diff --git a/lib/datadog/tracing/tracer.rb b/lib/datadog/tracing/tracer.rb index 5c95b38b2c6..4a70ee85b6c 100644 --- a/lib/datadog/tracing/tracer.rb +++ b/lib/datadog/tracing/tracer.rb @@ -341,6 +341,7 @@ def build_trace(digest = nil) TraceOperation.new( hostname: hostname, profiling_enabled: profiling_enabled, + apm_tracing_disabled: apm_tracing_disabled, id: digest.trace_id, origin: digest.trace_origin, parent_span_id: digest.span_id, @@ -356,6 +357,7 @@ def build_trace(digest = nil) TraceOperation.new( hostname: hostname, profiling_enabled: profiling_enabled, + apm_tracing_disabled: apm_tracing_disabled, remote_parent: false, tracer: self ) @@ -549,6 +551,10 @@ def profiling_enabled @profiling_enabled ||= !!(defined?(Datadog::Profiling) && Datadog::Profiling.respond_to?(:enabled?) && Datadog::Profiling.enabled?) end + + def apm_tracing_disabled + @apm_tracing_disabled ||= Datadog.configuration.appsec.standalone.enabled + end end end end diff --git a/lib/datadog/tracing/transport/trace_formatter.rb b/lib/datadog/tracing/transport/trace_formatter.rb index 2140ccebdd0..5b4a94c29ba 100644 --- a/lib/datadog/tracing/transport/trace_formatter.rb +++ b/lib/datadog/tracing/transport/trace_formatter.rb @@ -59,6 +59,7 @@ def format! tag_high_order_trace_id! tag_sampling_priority! tag_profiling_enabled! + tag_apm_tracing_disabled! if first_span tag_git_repository_url! @@ -196,6 +197,12 @@ def tag_profiling_enabled! ) end + def tag_apm_tracing_disabled! + return unless trace.apm_tracing_disabled + + root_span.set_tag(Tracing::Metadata::Ext::TAG_APM_ENABLED, 0) + end + def tag_git_repository_url! return if git_repository_url.nil? diff --git a/sig/datadog/tracing/metadata/ext.rbs b/sig/datadog/tracing/metadata/ext.rbs index 9820be2b123..7dd6ee3bcf4 100644 --- a/sig/datadog/tracing/metadata/ext.rbs +++ b/sig/datadog/tracing/metadata/ext.rbs @@ -8,6 +8,7 @@ module Datadog TAG_PEER_SERVICE: ::String TAG_KIND: ::String TAG_TOP_LEVEL: ::String + TAG_APM_ENABLED: ::String module Analytics DEFAULT_SAMPLE_RATE: ::Float diff --git a/sig/datadog/tracing/trace_operation.rbs b/sig/datadog/tracing/trace_operation.rbs index 551ce4ccf48..0c804543c33 100644 --- a/sig/datadog/tracing/trace_operation.rbs +++ b/sig/datadog/tracing/trace_operation.rbs @@ -23,7 +23,7 @@ module Datadog attr_writer sampled: untyped attr_writer service: untyped - def initialize: (?agent_sample_rate: untyped?, ?events: untyped?, ?hostname: untyped?, ?id: untyped?, ?max_length: untyped, ?name: untyped?, ?origin: untyped?, ?parent_span_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?rule_sample_rate: untyped?, ?sample_rate: untyped?, ?sampled: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?tags: untyped?, ?metrics: untyped?, ?remote_parent: untyped?) -> void + def initialize: (?agent_sample_rate: untyped?, ?events: untyped?, ?hostname: untyped?, ?id: untyped?, ?max_length: untyped, ?name: untyped?, ?origin: untyped?, ?parent_span_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?rule_sample_rate: untyped?, ?sample_rate: untyped?, ?sampled: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?profiling_enabled: untyped?, ?apm_tracing_enabled: untyped?, ?tags: untyped?, ?metrics: untyped?, ?remote_parent: untyped?) -> void def full?: () -> untyped def finished_span_count: () -> untyped def finished?: () -> untyped diff --git a/sig/datadog/tracing/trace_segment.rbs b/sig/datadog/tracing/trace_segment.rbs index 41f81d3114f..65e307c7f14 100644 --- a/sig/datadog/tracing/trace_segment.rbs +++ b/sig/datadog/tracing/trace_segment.rbs @@ -21,8 +21,10 @@ module Datadog attr_reader sampling_decision_maker: untyped attr_reader sampling_priority: untyped attr_reader service: untyped + attr_reader profiling_enabled: untyped + attr_reader apm_tracing_disabled: untyped - def initialize: (untyped spans, ?agent_sample_rate: untyped?, ?hostname: untyped?, ?id: untyped?, ?lang: untyped?, ?name: untyped?, ?origin: untyped?, ?process_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?root_span_id: untyped?, ?rule_sample_rate: untyped?, ?runtime_id: untyped?, ?sample_rate: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?tags: untyped?, ?metrics: untyped?) -> void + def initialize: (untyped spans, ?agent_sample_rate: untyped?, ?hostname: untyped?, ?id: untyped?, ?lang: untyped?, ?name: untyped?, ?origin: untyped?, ?process_id: untyped?, ?rate_limiter_rate: untyped?, ?resource: untyped?, ?root_span_id: untyped?, ?rule_sample_rate: untyped?, ?runtime_id: untyped?, ?sample_rate: untyped?, ?sampling_priority: untyped?, ?service: untyped?, ?tags: untyped?, ?metrics: untyped?, ?profiling_enabled: untyped?, ?apm_tracing_disabled: untyped?) -> void def any?: () -> untyped def count: () -> untyped def empty?: () -> untyped From 8ed08f3fbdaabc0f26a4c2e6b029a2f76dd6d76c Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Wed, 5 Mar 2025 18:16:57 +0100 Subject: [PATCH 02/21] Add method to skip upstream sampling priority --- lib/datadog/tracing/tracer.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/datadog/tracing/tracer.rb b/lib/datadog/tracing/tracer.rb index 4a70ee85b6c..35e629239b3 100644 --- a/lib/datadog/tracing/tracer.rb +++ b/lib/datadog/tracing/tracer.rb @@ -338,6 +338,9 @@ def build_trace(digest = nil) hostname = hostname && !hostname.empty? ? hostname : nil if digest + sampling_priority = if propagate_sampling_priority?(upstream_tags: digest.trace_distributed_tags) + digest.trace_sampling_priority + end TraceOperation.new( hostname: hostname, profiling_enabled: profiling_enabled, @@ -345,7 +348,7 @@ def build_trace(digest = nil) id: digest.trace_id, origin: digest.trace_origin, parent_span_id: digest.span_id, - sampling_priority: digest.trace_sampling_priority, + sampling_priority: sampling_priority, # Distributed tags are just regular trace tags with special meaning to Datadog tags: digest.trace_distributed_tags, trace_state: digest.trace_state, @@ -547,6 +550,19 @@ def skip_trace(name) end end + # Decide whether upstream sampling priority should be propagated, by taking into account + # the upstream tags and the configuration. + # We should always propagate if there APM is not disabled. + # + # e.g.: upstream tags containing dd.p.appsec, and appsec is enabled, return true. + # DEV: dd.p.appsec will be replaced by dd.p.ts in APM disablement 2.0, which will be a bitmask. + def propagate_sampling_priority?(upstream_tags:) + return true unless apm_tracing_disabled + return Datadog.configuration.appsec.enabled if upstream_tags.key?(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) + + false + end + def profiling_enabled @profiling_enabled ||= !!(defined?(Datadog::Profiling) && Datadog::Profiling.respond_to?(:enabled?) && Datadog::Profiling.enabled?) From ac8c0144bf163bea6f04b65098dd84f555b9617e Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Wed, 5 Mar 2025 18:23:10 +0100 Subject: [PATCH 03/21] Add APM disablement integration tests --- sig/datadog/tracing/tracer.rbs | 2 + .../contrib/rack/integration_test_spec.rb | 276 +++++++++++++++++- .../support/integration/shared_examples.rb | 239 +-------------- .../contrib/rack/integration_test_spec.rb | 179 +++++++++++- .../support/integration/shared_examples.rb | 49 ++++ 5 files changed, 503 insertions(+), 242 deletions(-) create mode 100644 spec/datadog/tracing/contrib/support/integration/shared_examples.rb diff --git a/sig/datadog/tracing/tracer.rbs b/sig/datadog/tracing/tracer.rbs index a642e3cb7ab..6ca9b390a14 100644 --- a/sig/datadog/tracing/tracer.rbs +++ b/sig/datadog/tracing/tracer.rbs @@ -6,6 +6,8 @@ module Datadog def active_span: (?untyped? key) -> Datadog::Tracing::Span def active_correlation: (?untyped? key) -> Struct[untyped] # Datadog::Correlation::Identifier + + def propagate_sampling_priority?: (upstream_tags: Hash[String, String]) -> bool end end end diff --git a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb index 97dc04e0b12..0b24a27e4ae 100644 --- a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb @@ -46,7 +46,9 @@ let(:tracing_enabled) { true } let(:appsec_enabled) { true } - let(:appsec_standalone_enabled) { false } + let(:instrument_http) { false } + + let(:apm_tracing_disabled) { false } let(:remote_enabled) { false } let(:appsec_ip_passlist) { [] } let(:appsec_ip_denylist) { [] } @@ -188,13 +190,13 @@ c.tracing.enabled = tracing_enabled c.tracing.instrument :rack - c.tracing.instrument :http + c.tracing.instrument :http if instrument_http c.appsec.enabled = appsec_enabled c.appsec.instrument :rack - c.appsec.standalone.enabled = appsec_standalone_enabled + c.appsec.standalone.enabled = apm_tracing_disabled c.appsec.waf_timeout = 10_000_000 # in us c.appsec.ip_passlist = appsec_ip_passlist c.appsec.ip_denylist = appsec_ip_denylist @@ -1023,7 +1025,273 @@ end end - it_behaves_like 'appsec standalone billing' + # it_behaves_like 'appsec standalone billing' + + describe 'ASM Standalone billing' do + let(:url) { '/requestdownstream' } + let(:params) { {} } + let(:headers) do + { + 'HTTP_X_DATADOG_TRACE_ID' => headers_trace_id, + 'HTTP_X_DATADOG_PARENT_ID' => headers_parent_id, + 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => headers_sampling_priority, + 'HTTP_X_DATADOG_ORIGIN' => headers_origin, + 'HTTP_X_DATADOG_TAGS' => headers_tags, + 'HTTP_USER_AGENT' => user_agent + } + end + let(:env) { headers } + + # Default values for headers + let(:headers_trace_id) { '1212121212121212121' } + let(:headers_parent_id) { '34343434' } + let(:headers_origin) { 'rum' } + let(:headers_sampling_priority) { '-1' } + let(:headers_tags) { '_dd.p.other=1' } + let(:user_agent) { nil } + + # Overwrite tracer_helpers span method as in our case we also instrument http + let(:span) do + Datadog::Tracing::Transport::TraceFormatter.format!(traces.last) + spans.find { |s| s.name == 'rack.request' && s.get_tag('http.url') == '/requestdownstream' } + end + + let(:apm_tracing_disabled) { true } + let(:instrument_http) { true } + + context 'without appsec upstream without attack and trace is kept with priority 1' do + subject(:response) do + clear_traces! + get '/success/' + # get url, params, env + end + + context 'from -1 sampling priority' do + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request with propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 0 sampling priority' do + let(:headers_sampling_priority) { '0' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request with propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 1 sampling priority' do + let(:headers_sampling_priority) { '1' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request with propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 2 sampling priority' do + let(:headers_sampling_priority) { '2' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request with propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + end + + context 'without upstream appsec propagation with attack and trace is kept with priority 2' do + subject(:response) do + clear_traces! + get '/success/' + get url, params, env + end + + let(:user_agent) { 'Arachni/v1' } + + context 'from -1 sampling priority' do + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.other=1', '_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 0 sampling priority' do + let(:headers_sampling_priority) { '0' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.other=1', '_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + end + + context 'with upstream appsec propagation without attack and trace is propagated as is' do + subject(:response) do + clear_traces! + get '/success/' + get url, params, env + end + + let(:headers_tags) { '_dd.p.appsec=1' } + + context 'from 0 sampling priority' do + let(:headers_sampling_priority) { '0' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { [0, 2].include?(x) } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { ['0', '2'].include?(x) }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 1 sampling priority' do + let(:headers_sampling_priority) { '1' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { [1, 2].include?(x) } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { ['1', '2'].include?(x) }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 2 sampling priority' do + let(:headers_sampling_priority) { '2' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + end + + context 'with any upstream propagation with attack and raises trace priority to 2' do + subject(:response) do + clear_traces! + get '/success/' + get url, params, env + end + + let(:user_agent) { 'Arachni/v1' } + let(:headers_tags) { nil } + + context 'from -1 sampling priority' do + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 0 sampling priority' do + let(:headers_sampling_priority) { '0' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 1 sampling priority' do + let(:headers_sampling_priority) { '1' } + + it_behaves_like 'a trace with ASM Standalone tags', + { + tag_appsec_propagation: '1', + tag_sampling_priority_condition: ->(x) { x == 2 } + } + it_behaves_like 'a request with propagated headers', + { + res_origin: 'rum', + res_parent_id_not_equal: '34343434', + res_tags: ['_dd.p.appsec=1'], + res_sampling_priority_condition: ->(x) { x == '2' }, + res_trace_id: '1212121212121212121' + } + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + end + end end end end diff --git a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb index bcf82aaa8aa..cf84889ee71 100644 --- a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb @@ -1,3 +1,5 @@ +require 'datadog/tracing/contrib/support/integration/shared_examples' + RSpec.shared_examples 'normal with tracing disable' do let(:tracing_enabled) { false } @@ -216,240 +218,3 @@ expect(agent_return.first.ok?).to be true end end - -RSpec.shared_examples 'appsec standalone billing' do - subject(:response) { get url, params, env } - - let(:appsec_standalone_enabled) { true } - - let(:url) { '/requestdownstream' } - let(:params) { {} } - let(:headers) do - { - 'HTTP_X_DATADOG_TRACE_ID' => headers_trace_id, - 'HTTP_X_DATADOG_PARENT_ID' => headers_parent_id, - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => headers_sampling_priority, - 'HTTP_X_DATADOG_ORIGIN' => headers_origin, - 'HTTP_X_DATADOG_TAGS' => headers_tags, - 'HTTP_USER_AGENT' => user_agent - } - end - let(:env) { headers } - - # Default values for headers - let(:headers_trace_id) { '1212121212121212121' } - let(:headers_parent_id) { '34343434' } - let(:headers_origin) { 'rum' } - let(:headers_sampling_priority) { '-1' } - let(:headers_tags) { '_dd.p.other=1' } - let(:user_agent) { nil } - - context 'without appsec upstream without attack and trace is kept with priority 1' do - context 'from -1 sampling priority' do - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } - } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 0 sampling priority' do - let(:headers_sampling_priority) { '0' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } - } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 1 sampling priority' do - let(:headers_sampling_priority) { '1' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } - } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 2 sampling priority' do - let(:headers_sampling_priority) { '2' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } - } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - end - - context 'without upstream appsec propagation with attack and trace is kept with priority 2' do - let(:user_agent) { 'Arachni/v1' } - - context 'from -1 sampling priority' do - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.other=1', '_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 0 sampling priority' do - let(:headers_sampling_priority) { '0' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.other=1', '_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - end - - context 'with upstream appsec propagation without attack and trace is propagated as is' do - let(:headers_tags) { '_dd.p.appsec=1' } - - context 'from 0 sampling priority' do - let(:headers_sampling_priority) { '0' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { [0, 2].include?(x) } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { ['0', '2'].include?(x) }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 1 sampling priority' do - let(:headers_sampling_priority) { '1' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { [1, 2].include?(x) } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { ['1', '2'].include?(x) }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 2 sampling priority' do - let(:headers_sampling_priority) { '2' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - end - - context 'with any upstream propagation with attack and raises trace priority to 2' do - let(:user_agent) { 'Arachni/v1' } - let(:headers_tags) { nil } - - context 'from -1 sampling priority' do - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 0 sampling priority' do - let(:headers_sampling_priority) { '0' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - - context 'from 1 sampling priority' do - let(:headers_sampling_priority) { '1' } - - it_behaves_like 'a trace with ASM Standalone tags', - { - tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { x == 2 } - } - it_behaves_like 'a request with propagated headers', - { - res_origin: 'rum', - res_parent_id_not_equal: '34343434', - res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { x == '2' }, - res_trace_id: '1212121212121212121' - } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' - end - end -end diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index d135556027d..85d58b35263 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -4,24 +4,80 @@ require 'rack' require 'datadog' require 'datadog/tracing/contrib/rack/middlewares' +require 'datadog/tracing/contrib/support/integration/shared_examples' require_relative '../support/http' +begin + require 'rack/contrib/json_body_parser' +rescue LoadError + # fallback for old rack-contrib + require 'rack/contrib/post_body_content_type_parser' +end + RSpec.describe 'Rack integration tests' do include Rack::Test::Methods let(:rack_options) { {} } + let(:instrument_http) { false } + let(:http_instrumentation_options) { {} } let(:remote_enabled) { false } + let(:apm_tracing_disabled) { false } + + # We send the trace to a mocked agent to verify that the trace includes the headers that we want + # In the future, it might be a good idea to use the traces that the mocked agent + # receives in the tests/shared examples + let(:agent_http_client) do + Datadog::Tracing::Transport::HTTP.default do |t| + t.adapter agent_http_adapter + end + end + let(:agent_http_adapter) { Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) } + let(:agent_settings) do + Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( + adapter: nil, + ssl: false, + uds_path: nil, + hostname: 'localhost', + port: 6218, + timeout_seconds: 30, + ) + end + let(:agent_tested_headers) { {} } before do + WebMock.enable! + stub_request(:get, 'http://localhost:3000/returnheaders') + .to_return do |request| + { + status: 200, + body: request.headers.to_json, + headers: { 'Content-Type' => 'application/json' } + } + end + + # Mocked agent with correct headers + stub_request(:post, 'http://localhost:6218/v0.4/traces') + .with do |request| + agent_tested_headers <= request.headers + end + .to_return(status: 200) + unless remote_enabled Datadog.configure do |c| c.remote.enabled = false c.tracing.instrument :rack, rack_options + # Required for APM disablement tests as rack can extract but not inject headers + c.tracing.instrument :http, http_instrumentation_options if instrument_http + + c.appsec.standalone.enabled = apm_tracing_disabled end end end after do + WebMock.reset! + WebMock.disable! + Datadog.registry[:rack].reset_configuration! end @@ -373,12 +429,32 @@ map '/success/' do run(proc { |_env| [200, { 'Content-Type' => 'text/html' }, ['OK']] }) end + + map '/requestdownstream' do + run( + proc do |_env| + uri = URI('http://localhost:3000/returnheaders') + ext_request = nil + ext_response = nil + + Net::HTTP.start(uri.host, uri.port) do |http| + ext_request = Net::HTTP::Get.new(uri) + + ext_response = http.request(ext_request) + end + + [200, { 'Content-Type' => 'application/json' }, [ext_response.body]] + end + ) + end end end before do is_expected.to be_ok - expect(spans).to have(1).items + expected_spans_number = instrument_http ? 2 : 1 + agent_http_client.send_traces(traces) + # expect(spans).to have(expected_spans_number).items end describe 'GET request' do @@ -577,6 +653,107 @@ end end end + + describe 'APM disablement' do + let(:url) { '/requestdownstream' } + let(:params) { {} } + let(:headers) do + { + 'HTTP_X_DATADOG_TRACE_ID' => headers_trace_id, + 'HTTP_X_DATADOG_PARENT_ID' => headers_parent_id, + 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => headers_sampling_priority, + 'HTTP_X_DATADOG_ORIGIN' => headers_origin, + 'HTTP_X_DATADOG_TAGS' => headers_tags, + 'HTTP_USER_AGENT' => user_agent + } + end + let(:env) { headers } + + # Default values for headers + let(:headers_trace_id) { '1212121212121212121' } + let(:headers_parent_id) { '34343434' } + let(:headers_origin) { 'rum' } + let(:headers_sampling_priority) { '-1' } + let(:headers_tags) { '_dd.p.other=1' } + let(:user_agent) { nil } + + # Overwrite tracer_helpers span method as in our case we also instrument http + let(:span) do + Datadog::Tracing::Transport::TraceFormatter.format!(traces.last) + spans.find { |s| s.name == 'rack.request' && s.get_tag('http.url') == '/requestdownstream' } + end + + let(:apm_tracing_disabled) { true } + let(:instrument_http) { true } + + context 'heartbeat trace that received propagated tags sent without sampling priority set to FORCE_KEEP' do + subject(:response) do + clear_traces! + get '/success/' + get url, params, env + end + + context 'from -1 sampling priority' do + it_behaves_like 'a trace with APM disablement tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request sent without propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 0 sampling priority' do + let(:headers_sampling_priority) { '0' } + + it_behaves_like 'a trace with APM disablement tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request sent without propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 1 sampling priority' do + let(:headers_sampling_priority) { '1' } + + it_behaves_like 'a trace with APM disablement tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request sent without propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + + context 'from 2 sampling priority' do + let(:headers_sampling_priority) { '2' } + + it_behaves_like 'a trace with APM disablement tags', + { + tag_other_propagation: '1', + tag_sampling_priority_condition: ->(x) { x < 2 } + } + it_behaves_like 'a request sent without propagated headers' + it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + end + end + + context 'no propagated tags received, two request sent' do + subject(:response) do + get url, params, env + get url, params, env + end + + let (:env) { {} } + + it do + # expect(traces[0].sampling_priority).to eq(2) + expect(traces[1].sampling_priority).to eq(-1) + end + end + end end context 'when `request_queuing` enabled' do diff --git a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb new file mode 100644 index 00000000000..6de7b53d19a --- /dev/null +++ b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb @@ -0,0 +1,49 @@ +RSpec.shared_examples 'a trace with APM disablement tags' do |params = {}| + let(:tag_apm_enabled) { params[:tag_apm_enabled] || 0 } + let(:tag_other_propagation) { params[:tag_other_propagation] || :any } + # We use a lambda as we may change the comparison type + let(:tag_sampling_priority_condition) { params[:tag_sampling_priority_condition] || ->(x) { x == 0 } } + let(:tag_trace_id) { params[:tag_trace_id] || headers_trace_id.to_i } + + it do + expect(span.send(:metrics)['_dd.apm.enabled']).to eq(tag_apm_enabled) + expect(span.send(:metrics)['_sampling_priority_v1']).to(satisfy { |x| tag_sampling_priority_condition.call(x) }) + + expect(span.send(:meta)['_dd.p.other']).to eq(tag_other_propagation) unless tag_other_propagation == :any + + expect(span.send(:trace_id)).to eq(tag_trace_id) + expect(traces.last.send(:spans)[0].send(:trace_id)).to eq(tag_trace_id) + end +end + +RSpec.shared_examples 'a request sent with propagated headers' do |params = {}| + let(:res_origin) { params[:res_origin] } + let(:res_parent_id_not_equal) { params[:res_parent_id_not_equal] } + let(:res_tags) { params[:res_tags] } + let(:res_sampling_priority_condition) { params[:res_sampling_priority_condition] || ->(x) { x.nil? } } + let(:res_trace_id) { params[:res_trace_id] } + + let(:res_headers) { JSON.parse(response.body) } + + it do + expect(res_headers['X-Datadog-Origin']).to eq(res_origin) + expect(res_headers['X-Datadog-Parent']).to_not eq(res_parent_id_not_equal) if res_parent_id_not_equal + expect(res_headers['X-Datadog-Sampling-Priority']).to(satisfy { |x| res_sampling_priority_condition.call(x) }) + expect(res_headers['X-Datadog-Trace-Id']).to eq(res_trace_id) + expect(res_headers['X-Datadog-Tags'].split(',')).to include(*res_tags) if res_tags + end +end + +RSpec.shared_examples 'a request sent without propagated headers' do + + it_behaves_like 'a request sent with propagated headers', {} +end + +RSpec.shared_examples 'a trace sent to agent with Datadog-Client-Computed-Stats header' do + let(:agent_tested_headers) { { 'Datadog-Client-Computed-Stats' => 'yes' } } + + it do + agent_return = agent_http_client.send_traces(traces) + expect(agent_return.first.ok?).to be true + end +end From 411d42811eb7b5c1f10214e7d5fdd59f49d0593c Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Thu, 6 Mar 2025 15:19:46 +0100 Subject: [PATCH 04/21] Fix propagate_sampling_priority with no upstream_tags --- lib/datadog/tracing/tracer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datadog/tracing/tracer.rb b/lib/datadog/tracing/tracer.rb index 35e629239b3..229e438aead 100644 --- a/lib/datadog/tracing/tracer.rb +++ b/lib/datadog/tracing/tracer.rb @@ -558,7 +558,7 @@ def skip_trace(name) # DEV: dd.p.appsec will be replaced by dd.p.ts in APM disablement 2.0, which will be a bitmask. def propagate_sampling_priority?(upstream_tags:) return true unless apm_tracing_disabled - return Datadog.configuration.appsec.enabled if upstream_tags.key?(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) + return Datadog.configuration.appsec.enabled if upstream_tags&.key?(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) false end From 5d55d8a4394df6a54295f2c7968ec1bfba3afea6 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Thu, 6 Mar 2025 15:22:39 +0100 Subject: [PATCH 05/21] Fix asm standalone integration tests --- .../contrib/rack/integration_test_spec.rb | 18 +++++++++++++++++- .../support/integration/shared_examples.rb | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb index 0b24a27e4ae..691b2e64b05 100644 --- a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb @@ -1062,8 +1062,12 @@ context 'without appsec upstream without attack and trace is kept with priority 1' do subject(:response) do clear_traces! + # First trace to send appsec oneshot_tags ('_dd.appsec.event_rules.loaded'...) get '/success/' - # get url, params, env + # Second trace is a heartbeat trace + get '/success/' + # Third trace should be sampled or force_kept if there is an appsec event + get url, params, env end context 'from -1 sampling priority' do @@ -1116,7 +1120,11 @@ context 'without upstream appsec propagation with attack and trace is kept with priority 2' do subject(:response) do clear_traces! + # First trace to send appsec oneshot_tags ('_dd.appsec.event_rules.loaded'...) + get '/success/' + # Second trace is a heartbeat trace get '/success/' + # Third trace should be sampled or force_kept if there is an appsec event get url, params, env end @@ -1162,7 +1170,11 @@ context 'with upstream appsec propagation without attack and trace is propagated as is' do subject(:response) do clear_traces! + # First trace to send appsec oneshot_tags ('_dd.appsec.event_rules.loaded'...) get '/success/' + # Second trace is a heartbeat trace + get '/success/' + # Third trace should be sampled or force_kept if there is an appsec event get url, params, env end @@ -1229,7 +1241,11 @@ context 'with any upstream propagation with attack and raises trace priority to 2' do subject(:response) do clear_traces! + # First trace to send appsec oneshot_tags ('_dd.appsec.event_rules.loaded'...) + get '/success/' + # Second trace is a heartbeat trace get '/success/' + # Third trace should be sampled or force_kept if there is an appsec event get url, params, env end diff --git a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb index cf84889ee71..63d8d0e769a 100644 --- a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb @@ -188,7 +188,7 @@ expect(span.send(:meta)['_dd.p.other']).to eq(tag_other_propagation) unless tag_other_propagation == :any expect(span.send(:trace_id)).to eq(tag_trace_id) - expect(trace.send(:spans)[0].send(:trace_id)).to eq(tag_trace_id) + expect(traces.last.send(:spans)[0].send(:trace_id)).to eq(tag_trace_id) end end From e491c6f06a097dcb4cba12564fc4b941c0fe263c Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Thu, 6 Mar 2025 15:34:23 +0100 Subject: [PATCH 06/21] Remove appsec_standalone_reject method --- lib/datadog/appsec/utils.rb | 2 - lib/datadog/appsec/utils/trace_operation.rb | 15 ------- .../tracing/contrib/ethon/easy_patch.rb | 4 -- .../tracing/contrib/excon/middleware.rb | 3 -- .../tracing/contrib/faraday/middleware.rb | 3 -- .../tracing/contrib/http/instrumentation.rb | 4 -- .../contrib/httpclient/instrumentation.rb | 4 -- .../tracing/contrib/httprb/instrumentation.rb | 4 -- .../contrib/rest_client/request_patch.rb | 3 -- sig/datadog/appsec/utils/trace_operation.rbs | 9 ----- .../appsec/utils/trace_operation_spec.rb | 40 ------------------- 11 files changed, 91 deletions(-) delete mode 100644 lib/datadog/appsec/utils/trace_operation.rb delete mode 100644 sig/datadog/appsec/utils/trace_operation.rbs delete mode 100644 spec/datadog/appsec/utils/trace_operation_spec.rb diff --git a/lib/datadog/appsec/utils.rb b/lib/datadog/appsec/utils.rb index b38ec5b96f5..8e4083533db 100644 --- a/lib/datadog/appsec/utils.rb +++ b/lib/datadog/appsec/utils.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require_relative 'utils/trace_operation' - module Datadog module AppSec # Utilities for AppSec diff --git a/lib/datadog/appsec/utils/trace_operation.rb b/lib/datadog/appsec/utils/trace_operation.rb deleted file mode 100644 index 19f2b0b2187..00000000000 --- a/lib/datadog/appsec/utils/trace_operation.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Datadog - module AppSec - module Utils - # Utility class to to AppSec-specific trace operations - class TraceOperation - def self.appsec_standalone_reject?(trace) - Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - end - end - end - end -end diff --git a/lib/datadog/tracing/contrib/ethon/easy_patch.rb b/lib/datadog/tracing/contrib/ethon/easy_patch.rb index 72e2c104f6b..af661e86b0c 100644 --- a/lib/datadog/tracing/contrib/ethon/easy_patch.rb +++ b/lib/datadog/tracing/contrib/ethon/easy_patch.rb @@ -110,10 +110,6 @@ def datadog_before_request(continue_from: nil) datadog_tag_request - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(datadog_trace) - datadog_trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end - if datadog_configuration[:distributed_tracing] @datadog_original_headers ||= {} Contrib::HTTP.inject(datadog_trace, @datadog_original_headers) diff --git a/lib/datadog/tracing/contrib/excon/middleware.rb b/lib/datadog/tracing/contrib/excon/middleware.rb index 9bb3f8629ef..e25ee1941cd 100644 --- a/lib/datadog/tracing/contrib/excon/middleware.rb +++ b/lib/datadog/tracing/contrib/excon/middleware.rb @@ -30,9 +30,6 @@ def request_call(datum) trace = Tracing.active_trace datum[:datadog_span] = span annotate!(span, datum) - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end propagate!(trace, span, datum) if distributed_tracing? span diff --git a/lib/datadog/tracing/contrib/faraday/middleware.rb b/lib/datadog/tracing/contrib/faraday/middleware.rb index 0795ba29b2c..9bdea9c364b 100644 --- a/lib/datadog/tracing/contrib/faraday/middleware.rb +++ b/lib/datadog/tracing/contrib/faraday/middleware.rb @@ -29,9 +29,6 @@ def call(env) Tracing.trace(Ext::SPAN_REQUEST, on_error: request_options[:on_error]) do |span, trace| annotate!(span, env, request_options) - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end propagate!(trace, span, env) if request_options[:distributed_tracing] && Tracing.enabled? app.call(env).on_complete { |resp| handle_response(span, resp, request_options) } end diff --git a/lib/datadog/tracing/contrib/http/instrumentation.rb b/lib/datadog/tracing/contrib/http/instrumentation.rb index b913a7d41dd..cd59f93fccd 100644 --- a/lib/datadog/tracing/contrib/http/instrumentation.rb +++ b/lib/datadog/tracing/contrib/http/instrumentation.rb @@ -35,10 +35,6 @@ def request(req, body = nil, &block) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND span.resource = req.method - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end - if Tracing.enabled? && !Contrib::HTTP.should_skip_distributed_tracing?(client_config) Contrib::HTTP.inject(trace, req) end diff --git a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb index 15c218bf333..5cbc784bf59 100644 --- a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb @@ -30,10 +30,6 @@ def do_get_block(req, proxy, conn, &block) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end - if Tracing.enabled? && !should_skip_distributed_tracing?(client_config) Contrib::HTTP.inject(trace, req.header) end diff --git a/lib/datadog/tracing/contrib/httprb/instrumentation.rb b/lib/datadog/tracing/contrib/httprb/instrumentation.rb index 7d0a11e7ff9..c39916ebb4a 100644 --- a/lib/datadog/tracing/contrib/httprb/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httprb/instrumentation.rb @@ -30,10 +30,6 @@ def perform(req, options) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end - Contrib::HTTP.inject(trace, req) if Tracing.enabled? && !should_skip_distributed_tracing?(client_config) # Add additional request specific tags to the span. diff --git a/lib/datadog/tracing/contrib/rest_client/request_patch.rb b/lib/datadog/tracing/contrib/rest_client/request_patch.rb index 4f340c5356d..cabb1b73161 100644 --- a/lib/datadog/tracing/contrib/rest_client/request_patch.rb +++ b/lib/datadog/tracing/contrib/rest_client/request_patch.rb @@ -25,9 +25,6 @@ def execute(&block) return super(&block) unless Tracing.enabled? datadog_trace_request(uri) do |_span, trace| - if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(trace) - trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT - end Contrib::HTTP.inject(trace, processed_headers) if datadog_configuration[:distributed_tracing] super(&block) diff --git a/sig/datadog/appsec/utils/trace_operation.rbs b/sig/datadog/appsec/utils/trace_operation.rbs deleted file mode 100644 index cd7a86680a1..00000000000 --- a/sig/datadog/appsec/utils/trace_operation.rbs +++ /dev/null @@ -1,9 +0,0 @@ -module Datadog - module AppSec - module Utils - class TraceOperation - def self.appsec_standalone_reject?: (Datadog::Tracing::TraceOperation trace) -> bool - end - end - end -end diff --git a/spec/datadog/appsec/utils/trace_operation_spec.rb b/spec/datadog/appsec/utils/trace_operation_spec.rb deleted file mode 100644 index 15692054506..00000000000 --- a/spec/datadog/appsec/utils/trace_operation_spec.rb +++ /dev/null @@ -1,40 +0,0 @@ -require 'datadog/appsec/spec_helper' -require 'datadog/appsec/utils/trace_operation' - -RSpec.describe Datadog::AppSec::Utils::TraceOperation do - describe '#appsec_standalone_reject?' do - subject(:appsec_standalone_reject?) do - described_class.appsec_standalone_reject?(trace_op) - end - - let(:trace_op) { Datadog::Tracing::TraceOperation.new(**options) } - let(:options) { {} } - let(:appsec_standalone) { false } - let(:distributed_appsec_event) { '0' } - - before do - allow(Datadog.configuration.appsec.standalone).to receive(:enabled).and_return(appsec_standalone) - trace_op.set_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT, distributed_appsec_event) if trace_op - end - - it { is_expected.to be false } - - context 'when AppSec standalone is enabled' do - let(:appsec_standalone) { true } - - it { is_expected.to be true } - - context 'without a trace' do - let(:trace_op) { nil } - - it { is_expected.to be true } - end - - context 'with a distributed AppSec event' do - let(:distributed_appsec_event) { '1' } - - it { is_expected.to be false } - end - end - end -end From 0b261d2a1c5afc9bb8bb4128102322f56be0c5c0 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Fri, 7 Mar 2025 11:17:43 +0100 Subject: [PATCH 07/21] Skip distributed tracing if APM is disabled on all HTTP/GRPC/Sidekiq frameworks --- lib/datadog/tracing/contrib/ethon/easy_patch.rb | 12 +++++++++++- lib/datadog/tracing/contrib/excon/middleware.rb | 12 +++++++++++- lib/datadog/tracing/contrib/faraday/middleware.rb | 12 +++++++++++- .../contrib/grpc/datadog_interceptor/client.rb | 12 +++++++++++- lib/datadog/tracing/contrib/http/circuit_breaker.rb | 11 ++++------- lib/datadog/tracing/contrib/http/instrumentation.rb | 2 +- .../tracing/contrib/httpclient/instrumentation.rb | 10 ++++++++-- .../tracing/contrib/httprb/instrumentation.rb | 12 ++++++++++-- .../tracing/contrib/rest_client/request_patch.rb | 12 +++++++++++- lib/datadog/tracing/contrib/sidekiq/client_tracer.rb | 12 +++++++++++- .../tracing/contrib/http/circuit_breaker_spec.rb | 10 +++++----- 11 files changed, 94 insertions(+), 23 deletions(-) diff --git a/lib/datadog/tracing/contrib/ethon/easy_patch.rb b/lib/datadog/tracing/contrib/ethon/easy_patch.rb index af661e86b0c..423e47491a4 100644 --- a/lib/datadog/tracing/contrib/ethon/easy_patch.rb +++ b/lib/datadog/tracing/contrib/ethon/easy_patch.rb @@ -110,7 +110,7 @@ def datadog_before_request(continue_from: nil) datadog_tag_request - if datadog_configuration[:distributed_tracing] + unless should_skip_distributed_tracing?(datadog_trace) @datadog_original_headers ||= {} Contrib::HTTP.inject(datadog_trace, @datadog_original_headers) self.headers = @datadog_original_headers @@ -215,6 +215,16 @@ def parse_response_headers header.size != 2 ? nil : header end.compact.to_h end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !datadog_configuration[:distributed_tracing] + end end end end diff --git a/lib/datadog/tracing/contrib/excon/middleware.rb b/lib/datadog/tracing/contrib/excon/middleware.rb index e25ee1941cd..465ecf6dbce 100644 --- a/lib/datadog/tracing/contrib/excon/middleware.rb +++ b/lib/datadog/tracing/contrib/excon/middleware.rb @@ -30,7 +30,7 @@ def request_call(datum) trace = Tracing.active_trace datum[:datadog_span] = span annotate!(span, datum) - propagate!(trace, span, datum) if distributed_tracing? + propagate!(trace, span, datum) unless should_skip_distributed_tracing?(trace) span end @@ -189,6 +189,16 @@ def build_request_options!(datum) def datadog_configuration(host = :default) Datadog.configuration.tracing[:excon, host] end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !distributed_tracing? + end end end end diff --git a/lib/datadog/tracing/contrib/faraday/middleware.rb b/lib/datadog/tracing/contrib/faraday/middleware.rb index 9bdea9c364b..9f1b214d109 100644 --- a/lib/datadog/tracing/contrib/faraday/middleware.rb +++ b/lib/datadog/tracing/contrib/faraday/middleware.rb @@ -29,7 +29,7 @@ def call(env) Tracing.trace(Ext::SPAN_REQUEST, on_error: request_options[:on_error]) do |span, trace| annotate!(span, env, request_options) - propagate!(trace, span, env) if request_options[:distributed_tracing] && Tracing.enabled? + propagate!(trace, span, env) if Tracing.enabled? && !should_skip_distributed_tracing?(request_options, trace) app.call(env).on_complete { |resp| handle_response(span, resp, request_options) } end end @@ -114,6 +114,16 @@ def build_request_options!(env) def datadog_configuration(host = :default) Datadog.configuration.tracing[:faraday, host] end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(client_config, trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !client_config[:distributed_tracing] + end end end end diff --git a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb index eac66872a2b..ab69d364c1f 100644 --- a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb +++ b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb @@ -81,7 +81,7 @@ def annotate!(trace, span, keywords, formatter) # Set analytics sample rate Contrib::Analytics.set_sample_rate(span, analytics_sample_rate) if analytics_enabled? - GRPC.inject(trace, metadata) if distributed_tracing? + GRPC.inject(trace, metadata) unless should_skip_distributed_tracing?(trace) Contrib::SpanAttributeSchema.set_peer_service!(span, Ext::PEER_SERVICE_SOURCES) rescue StandardError => e Datadog.logger.debug("GRPC client trace failed: #{e}") @@ -109,6 +109,16 @@ def find_host_port(call) Datadog.logger.debug { "Could not parse host:port from #{call}: #{e}" } nil end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !distributed_tracing? + end end end end diff --git a/lib/datadog/tracing/contrib/http/circuit_breaker.rb b/lib/datadog/tracing/contrib/http/circuit_breaker.rb index d35ceda5a3e..0598e708b11 100644 --- a/lib/datadog/tracing/contrib/http/circuit_breaker.rb +++ b/lib/datadog/tracing/contrib/http/circuit_breaker.rb @@ -28,14 +28,11 @@ def internal_request?(request) request[Datadog::Core::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST]) end - def should_skip_distributed_tracing?(client_config) + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(client_config, trace) if Datadog.configuration.appsec.standalone.enabled - # Skip distributed tracing so that we don't bill distributed traces in case of absence of - # upstream ASM event (_dd.p.appsec:1) and no local security event (which sets _dd.p.appsec:1 locally). - # If there is an ASM event, we still have to check if distributed tracing is enabled or not - return true unless Tracing.active_trace - - return true if Tracing.active_trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1' + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' end return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) diff --git a/lib/datadog/tracing/contrib/http/instrumentation.rb b/lib/datadog/tracing/contrib/http/instrumentation.rb index cd59f93fccd..cb0c0c15aba 100644 --- a/lib/datadog/tracing/contrib/http/instrumentation.rb +++ b/lib/datadog/tracing/contrib/http/instrumentation.rb @@ -35,7 +35,7 @@ def request(req, body = nil, &block) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND span.resource = req.method - if Tracing.enabled? && !Contrib::HTTP.should_skip_distributed_tracing?(client_config) + if Tracing.enabled? && !Contrib::HTTP.should_skip_distributed_tracing?(client_config, trace) Contrib::HTTP.inject(trace, req) end diff --git a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb index 5cbc784bf59..6dc9718c5ad 100644 --- a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb @@ -30,7 +30,7 @@ def do_get_block(req, proxy, conn, &block) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - if Tracing.enabled? && !should_skip_distributed_tracing?(client_config) + if Tracing.enabled? && !should_skip_distributed_tracing?(client_config, trace) Contrib::HTTP.inject(trace, req.header) end @@ -119,7 +119,13 @@ def analytics_enabled?(request_options) Contrib::Analytics.enabled?(request_options[:analytics_enabled]) end - def should_skip_distributed_tracing?(client_config) + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(client_config, trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) !Datadog.configuration.tracing[:httpclient][:distributed_tracing] diff --git a/lib/datadog/tracing/contrib/httprb/instrumentation.rb b/lib/datadog/tracing/contrib/httprb/instrumentation.rb index c39916ebb4a..22ccf99e01f 100644 --- a/lib/datadog/tracing/contrib/httprb/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httprb/instrumentation.rb @@ -30,7 +30,9 @@ def perform(req, options) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - Contrib::HTTP.inject(trace, req) if Tracing.enabled? && !should_skip_distributed_tracing?(client_config) + if Tracing.enabled? && !should_skip_distributed_tracing?(client_config, trace) + Contrib::HTTP.inject(trace, req) + end # Add additional request specific tags to the span. annotate_span_with_request!(span, req, request_options) @@ -131,7 +133,13 @@ def logger Datadog.logger end - def should_skip_distributed_tracing?(client_config) + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(client_config, trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) !Datadog.configuration.tracing[:httprb][:distributed_tracing] diff --git a/lib/datadog/tracing/contrib/rest_client/request_patch.rb b/lib/datadog/tracing/contrib/rest_client/request_patch.rb index cabb1b73161..def7327a022 100644 --- a/lib/datadog/tracing/contrib/rest_client/request_patch.rb +++ b/lib/datadog/tracing/contrib/rest_client/request_patch.rb @@ -25,7 +25,7 @@ def execute(&block) return super(&block) unless Tracing.enabled? datadog_trace_request(uri) do |_span, trace| - Contrib::HTTP.inject(trace, processed_headers) if datadog_configuration[:distributed_tracing] + Contrib::HTTP.inject(trace, processed_headers) unless should_skip_distributed_tracing?(trace) super(&block) end @@ -121,6 +121,16 @@ def analytics_enabled? def analytics_sample_rate datadog_configuration[:analytics_sample_rate] end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !datadog_configuration[:distributed_tracing] + end end end end diff --git a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb index f57fde8a101..380747ff992 100644 --- a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb +++ b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb @@ -24,7 +24,7 @@ def call(worker_class, job, queue, redis_pool) resource = job_resource(job) Datadog::Tracing.trace(Ext::SPAN_PUSH, service: @sidekiq_service) do |span, trace_op| - Sidekiq.inject(trace_op, job) if configuration[:distributed_tracing] + Sidekiq.inject(trace_op, job) unless should_skip_distributed_tracing?(trace_op) span.resource = resource @@ -55,6 +55,16 @@ def call(worker_class, job, queue, redis_pool) def configuration Datadog.configuration.tracing[:sidekiq] end + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(trace) + if Datadog.configuration.appsec.standalone.enabled + return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + end + + !configuration[:distributed_tracing] + end end end end diff --git a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb index 851ccddb86d..783cb983daa 100644 --- a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb +++ b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb @@ -93,19 +93,19 @@ end describe '#should_skip_distributed_tracing?' do - subject(:should_skip_distributed_tracing?) { circuit_breaker.should_skip_distributed_tracing?(client_config) } + subject(:should_skip_distributed_tracing?) { circuit_breaker.should_skip_distributed_tracing?(client_config, trace) } let(:client_config) { nil } let(:distributed_tracing) { true } let(:appsec_standalone) { false } - let(:active_trace) { nil } + let(:trace) { nil } let(:distributed_appsec_event) { nil } before do allow(Datadog.configuration.tracing[:http]).to receive(:[]).with(:distributed_tracing).and_return(distributed_tracing) allow(Datadog.configuration.appsec.standalone).to receive(:enabled).and_return(appsec_standalone) - allow(Datadog::Tracing).to receive(:active_trace).and_return(active_trace) - allow(active_trace).to receive(:get_tag).with('_dd.p.appsec').and_return(distributed_appsec_event) if active_trace + allow(Datadog::Tracing).to receive(:trace).and_return(trace) + allow(trace).to receive(:get_tag).with('_dd.p.appsec').and_return(distributed_appsec_event) if trace end context 'when distributed tracing is enabled' do @@ -126,7 +126,7 @@ end context 'when there is an active trace' do - let(:active_trace) { instance_double(Datadog::Tracing::TraceOperation) } + let(:trace) { instance_double(Datadog::Tracing::TraceOperation) } context 'when the active trace has no distributed appsec event' do it { is_expected.to be true } From f8acd8f22833d9015ef4e75c526b3848c1d462a7 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Fri, 7 Mar 2025 15:06:51 +0100 Subject: [PATCH 08/21] Delete unnecessary default sample rate (as we match all traces against the rate limiter) --- lib/datadog/tracing/component.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index ea0d9b0fcc1..9e20badeddd 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -82,8 +82,7 @@ def build_sampler(settings) if settings.appsec.standalone.enabled post_sampler = Tracing::Sampling::RuleSampler.new( [Tracing::Sampling::SimpleRule.new(sample_rate: 1.0)], - rate_limiter: Datadog::Core::TokenBucket.new(1.0 / 60, 1.0), - default_sample_rate: 1.0 / 60 + rate_limiter: Datadog::Core::TokenBucket.new(1.0 / 60, 1.0) ) end From 360761a9665d5f39d7abbad5c2183387ae7100da Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Fri, 7 Mar 2025 15:24:04 +0100 Subject: [PATCH 09/21] Add test to check heartbeat traces --- lib/datadog/tracing/component.rb | 2 ++ .../contrib/rack/integration_test_spec.rb | 34 +++++++++++++++---- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index 9e20badeddd..eeb77e62c5c 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -79,6 +79,8 @@ def build_sampler(settings) # so we want to send the minimum amount of traces possible (idealy only traces that contains security events), # but for features such as API Security, we need to send at least one trace per minute, # to keep the service alive on the backend side. + # + # DEV: (APM Disablement V2) Use a separate method to generate the post_sampler if settings.appsec.standalone.enabled post_sampler = Tracing::Sampling::RuleSampler.new( [Tracing::Sampling::SimpleRule.new(sample_rate: 1.0)], diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index 85d58b35263..faa6242d776 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -27,7 +27,7 @@ # In the future, it might be a good idea to use the traces that the mocked agent # receives in the tests/shared examples let(:agent_http_client) do - Datadog::Tracing::Transport::HTTP.default do |t| + Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| t.adapter agent_http_adapter end end @@ -44,6 +44,7 @@ end let(:agent_tested_headers) { {} } + # This before block is executed before booting the tracer, which is why we add mocks here before do WebMock.enable! stub_request(:get, 'http://localhost:3000/returnheaders') @@ -62,6 +63,22 @@ end .to_return(status: 200) + # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 + # + # DEV: (APM Disablement V2) Mock the APM disablement sampler generator instead + if apm_tracing_disabled + post_sampler = Datadog::Tracing::Sampling::RuleSampler.new( + [Datadog::Tracing::Sampling::SimpleRule.new(sample_rate: 1.0)], + rate_limiter: Datadog::Core::TokenBucket.new(1.0 / 4, 1.0), + default_sample_rate: 1.0 / 4 + ) + sampler = Datadog::Tracing::Sampling::PrioritySampler.new( + base_sampler: Datadog::Tracing::Sampling::AllSampler.new, + post_sampler: post_sampler + ) + allow(Datadog::Core::Configuration::Components).to receive(:build_sampler).and_return(sampler) + end + unless remote_enabled Datadog.configure do |c| c.remote.enabled = false @@ -686,7 +703,7 @@ let(:apm_tracing_disabled) { true } let(:instrument_http) { true } - context 'heartbeat trace that received propagated tags sent without sampling priority set to FORCE_KEEP' do + context 'request contains propagated tags, trace sent without sampling priority set to FORCE_KEEP' do subject(:response) do clear_traces! get '/success/' @@ -740,17 +757,22 @@ end end - context 'no propagated tags received, two request sent' do + context '2 heartbeat traces and 1 dropped trace' do subject(:response) do - get url, params, env - get url, params, env + clear_traces! + get '/success/' + sleep(2) + get '/success/' + sleep(2) + get '/success/' end let (:env) { {} } it do - # expect(traces[0].sampling_priority).to eq(2) + expect(traces[0].sampling_priority).to eq(2) expect(traces[1].sampling_priority).to eq(-1) + expect(traces[2].sampling_priority).to eq(2) end end end From 09646d13f5c70e0ed869129b48e67c7a7d8002a2 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Fri, 7 Mar 2025 17:25:18 +0100 Subject: [PATCH 10/21] Fix Datadog-Computed-Client-Stats tests --- .../contrib/rack/integration_test_spec.rb | 21 ++++++++++++------- .../support/integration/shared_examples.rb | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index faa6242d776..044ef7c0ad2 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -56,12 +56,14 @@ } end - # Mocked agent with correct headers - stub_request(:post, 'http://localhost:6218/v0.4/traces') - .with do |request| - agent_tested_headers <= request.headers - end - .to_return(status: 200) + # Mocked agent that returns the headers sent + stub_request(:post, 'http://localhost:6218/v0.4/traces').to_return do |request| + { + status: 200, + body: request.headers.to_json, + headers: { 'Content-Type' => 'application/json' } + } + end # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 # @@ -83,7 +85,7 @@ Datadog.configure do |c| c.remote.enabled = false c.tracing.instrument :rack, rack_options - # Required for APM disablement tests as rack can extract but not inject headers + # Required for APM disablement tests with distributed tracing as rack can extract but not inject headers c.tracing.instrument :http, http_instrumentation_options if instrument_http c.appsec.standalone.enabled = apm_tracing_disabled @@ -470,7 +472,6 @@ before do is_expected.to be_ok expected_spans_number = instrument_http ? 2 : 1 - agent_http_client.send_traces(traces) # expect(spans).to have(expected_spans_number).items end @@ -672,6 +673,10 @@ end describe 'APM disablement' do + before do + agent_http_client.send_traces(traces) + end + let(:url) { '/requestdownstream' } let(:params) { {} } let(:headers) do diff --git a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb index 6de7b53d19a..1f2c55fa15b 100644 --- a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb @@ -44,6 +44,6 @@ it do agent_return = agent_http_client.send_traces(traces) - expect(agent_return.first.ok?).to be true + expect(JSON.parse(agent_return.first.payload)["Datadog-Client-Computed-Stats"]).to eq('yes') end end From 6e82099865865ee5526ed5478a8de4df811bb9af Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Fri, 7 Mar 2025 18:09:40 +0100 Subject: [PATCH 11/21] Simplify Datadog-Client-Computed-Stats test --- .../contrib/rack/integration_test_spec.rb | 62 +++++++++---------- .../support/integration/shared_examples.rb | 9 --- 2 files changed, 31 insertions(+), 40 deletions(-) diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index 044ef7c0ad2..3c490818945 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -19,30 +19,12 @@ let(:rack_options) { {} } let(:instrument_http) { false } - let(:http_instrumentation_options) { {} } let(:remote_enabled) { false } let(:apm_tracing_disabled) { false } # We send the trace to a mocked agent to verify that the trace includes the headers that we want # In the future, it might be a good idea to use the traces that the mocked agent # receives in the tests/shared examples - let(:agent_http_client) do - Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| - t.adapter agent_http_adapter - end - end - let(:agent_http_adapter) { Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) } - let(:agent_settings) do - Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( - adapter: nil, - ssl: false, - uds_path: nil, - hostname: 'localhost', - port: 6218, - timeout_seconds: 30, - ) - end - let(:agent_tested_headers) { {} } # This before block is executed before booting the tracer, which is why we add mocks here before do @@ -86,7 +68,7 @@ c.remote.enabled = false c.tracing.instrument :rack, rack_options # Required for APM disablement tests with distributed tracing as rack can extract but not inject headers - c.tracing.instrument :http, http_instrumentation_options if instrument_http + c.tracing.instrument :http if instrument_http c.appsec.standalone.enabled = apm_tracing_disabled end @@ -673,10 +655,6 @@ end describe 'APM disablement' do - before do - agent_http_client.send_traces(traces) - end - let(:url) { '/requestdownstream' } let(:params) { {} } let(:headers) do @@ -708,6 +686,32 @@ let(:apm_tracing_disabled) { true } let(:instrument_http) { true } + context 'trace sent to agent with Datadog-Client-Computed-Stats header' do + # Agent mocked in top before block + subject(:response) do + clear_traces! + get '/success/' + end + + it do + agent_settings = Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( + adapter: nil, + ssl: false, + uds_path: nil, + hostname: 'localhost', + port: 6218, + timeout_seconds: 30 + ) + agent_http_adapter = Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) + agent_http_client = Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| + t.adapter agent_http_adapter + end + agent_return = agent_http_client.send_traces(traces) + + expect(JSON.parse(agent_return.first.payload)["Datadog-Client-Computed-Stats"]).to eq('yes') + end + end + context 'request contains propagated tags, trace sent without sampling priority set to FORCE_KEEP' do subject(:response) do clear_traces! @@ -719,10 +723,9 @@ it_behaves_like 'a trace with APM disablement tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } it_behaves_like 'a request sent without propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 0 sampling priority' do @@ -731,10 +734,9 @@ it_behaves_like 'a trace with APM disablement tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } it_behaves_like 'a request sent without propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 1 sampling priority' do @@ -743,10 +745,9 @@ it_behaves_like 'a trace with APM disablement tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } it_behaves_like 'a request sent without propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 2 sampling priority' do @@ -755,10 +756,9 @@ it_behaves_like 'a trace with APM disablement tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } it_behaves_like 'a request sent without propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end end diff --git a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb index 1f2c55fa15b..dc7c43bc1fa 100644 --- a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb @@ -38,12 +38,3 @@ it_behaves_like 'a request sent with propagated headers', {} end - -RSpec.shared_examples 'a trace sent to agent with Datadog-Client-Computed-Stats header' do - let(:agent_tested_headers) { { 'Datadog-Client-Computed-Stats' => 'yes' } } - - it do - agent_return = agent_http_client.send_traces(traces) - expect(JSON.parse(agent_return.first.payload)["Datadog-Client-Computed-Stats"]).to eq('yes') - end -end From 5c73ff8a3e47825f4c19631a6b69f16e255dd570 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 10 Mar 2025 14:37:13 +0100 Subject: [PATCH 12/21] Add separate method to generate APM disablement post_sampler --- lib/datadog/tracing/component.rb | 16 ++++++++-------- sig/datadog/tracing/component.rbs | 2 ++ .../contrib/rack/integration_test_spec.rb | 16 +++------------- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index eeb77e62c5c..2ba362bd9b2 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -79,14 +79,7 @@ def build_sampler(settings) # so we want to send the minimum amount of traces possible (idealy only traces that contains security events), # but for features such as API Security, we need to send at least one trace per minute, # to keep the service alive on the backend side. - # - # DEV: (APM Disablement V2) Use a separate method to generate the post_sampler - if settings.appsec.standalone.enabled - post_sampler = Tracing::Sampling::RuleSampler.new( - [Tracing::Sampling::SimpleRule.new(sample_rate: 1.0)], - rate_limiter: Datadog::Core::TokenBucket.new(1.0 / 60, 1.0) - ) - end + post_sampler = build_above_second_post_sampler(60) if settings.appsec.standalone.enabled # Sampling rules are provided if (rules = settings.tracing.sampling.rules) @@ -198,6 +191,13 @@ def build_tracer_tags(settings) end end + def build_above_second_post_sampler(seconds) + Tracing::Sampling::RuleSampler.new( + rate_limiter: Datadog::Core::TokenBucket.new(1.0 / seconds, 1.0), + default_sample_rate: 1.0 + ) + end + def build_test_mode_trace_flush(settings) # If context flush behavior is provided, use it instead. settings.tracing.test_mode.trace_flush || build_trace_flush(settings) diff --git a/sig/datadog/tracing/component.rbs b/sig/datadog/tracing/component.rbs index d600a7559c1..dcf2324b8c6 100644 --- a/sig/datadog/tracing/component.rbs +++ b/sig/datadog/tracing/component.rbs @@ -22,6 +22,8 @@ module Datadog def build_tracer_tags: (untyped settings) -> untyped + def build_above_second_post_sampler: (Integer seconds) -> Datadog::Tracing::Sampling::RuleSampler + def build_test_mode_trace_flush: (untyped settings) -> untyped def build_test_mode_sampler: () -> untyped diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index 3c490818945..152a24291ed 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -47,20 +47,10 @@ } end - # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 - # - # DEV: (APM Disablement V2) Mock the APM disablement sampler generator instead + # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 so tests are faster if apm_tracing_disabled - post_sampler = Datadog::Tracing::Sampling::RuleSampler.new( - [Datadog::Tracing::Sampling::SimpleRule.new(sample_rate: 1.0)], - rate_limiter: Datadog::Core::TokenBucket.new(1.0 / 4, 1.0), - default_sample_rate: 1.0 / 4 - ) - sampler = Datadog::Tracing::Sampling::PrioritySampler.new( - base_sampler: Datadog::Tracing::Sampling::AllSampler.new, - post_sampler: post_sampler - ) - allow(Datadog::Core::Configuration::Components).to receive(:build_sampler).and_return(sampler) + post_sampler = Datadog::Core::Configuration::Components.send(:build_above_second_post_sampler, 4) + allow(Datadog::Core::Configuration::Components).to receive(:build_above_second_post_sampler).and_return(post_sampler) end unless remote_enabled From 0389f6bcb5ab55d9c37b59a02bc11f6d8ee98d9b Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 10 Mar 2025 17:43:24 +0100 Subject: [PATCH 13/21] Cleaned appsec standalone tests for rack --- sig/datadog/appsec/ext.rbs | 2 - .../contrib/rack/integration_test_spec.rb | 81 +++++-------------- .../support/integration/shared_examples.rb | 41 +--------- 3 files changed, 20 insertions(+), 104 deletions(-) diff --git a/sig/datadog/appsec/ext.rbs b/sig/datadog/appsec/ext.rbs index 58bb2277da3..aa9efd59156 100644 --- a/sig/datadog/appsec/ext.rbs +++ b/sig/datadog/appsec/ext.rbs @@ -17,8 +17,6 @@ module Datadog TAG_APPSEC_ENABLED: ::String - TAG_APM_ENABLED: ::String - TAG_DISTRIBUTED_APPSEC_EVENT: ::String TELEMETRY_METRICS_NAMESPACE: ::String diff --git a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb index 691b2e64b05..554bb679ee9 100644 --- a/spec/datadog/appsec/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/rack/integration_test_spec.rb @@ -19,30 +19,6 @@ RSpec.describe 'Rack integration tests' do include Rack::Test::Methods - # We send the trace to a mocked agent to verify that the trace includes the headers that we want - # In the future, it might be a good idea to use the traces that the mocked agent - # receives in the tests/shared examples - let(:agent_http_client) do - Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| - t.adapter agent_http_adapter - end - end - - let(:agent_http_adapter) { Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) } - - let(:agent_settings) do - Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( - adapter: nil, - ssl: false, - uds_path: nil, - hostname: 'localhost', - port: 6218, - timeout_seconds: 30, - ) - end - - let(:agent_tested_headers) { {} } - let(:tracing_enabled) { true } let(:appsec_enabled) { true } @@ -169,13 +145,6 @@ } end - # Mocked agent with correct headers - stub_request(:post, 'http://localhost:6218/v0.4/traces') - .with do |request| - agent_tested_headers <= request.headers - end - .to_return(status: 200) - # DEV: Would it be faster to do another stub for requests that don't match the headers # rather than waiting for the TCP connection to fail? @@ -1025,8 +994,6 @@ end end - # it_behaves_like 'appsec standalone billing' - describe 'ASM Standalone billing' do let(:url) { '/requestdownstream' } let(:params) { {} } @@ -1074,10 +1041,9 @@ it_behaves_like 'a trace with ASM Standalone tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + it_behaves_like 'a request sent without propagated headers' end context 'from 0 sampling priority' do @@ -1086,10 +1052,9 @@ it_behaves_like 'a trace with ASM Standalone tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + it_behaves_like 'a request sent without propagated headers' end context 'from 1 sampling priority' do @@ -1098,10 +1063,9 @@ it_behaves_like 'a trace with ASM Standalone tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + it_behaves_like 'a request sent without propagated headers' end context 'from 2 sampling priority' do @@ -1110,10 +1074,9 @@ it_behaves_like 'a trace with ASM Standalone tags', { tag_other_propagation: '1', - tag_sampling_priority_condition: ->(x) { x < 2 } + tag_sampling_priority_condition: ->(x) { x <= 0 } } - it_behaves_like 'a request with propagated headers' - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' + it_behaves_like 'a request sent without propagated headers' end end @@ -1136,7 +1099,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1144,7 +1107,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 0 sampling priority' do @@ -1155,7 +1117,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1163,7 +1125,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end end @@ -1186,17 +1147,16 @@ it_behaves_like 'a trace with ASM Standalone tags', { tag_appsec_propagation: '1', - tag_sampling_priority_condition: ->(x) { [0, 2].include?(x) } + tag_sampling_priority_condition: ->(x) { x == 0 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', res_tags: ['_dd.p.appsec=1'], - res_sampling_priority_condition: ->(x) { ['0', '2'].include?(x) }, + res_sampling_priority_condition: ->(x) { x == '0' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 1 sampling priority' do @@ -1207,7 +1167,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { [1, 2].include?(x) } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1215,7 +1175,6 @@ res_sampling_priority_condition: ->(x) { ['1', '2'].include?(x) }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 2 sampling priority' do @@ -1226,7 +1185,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1234,7 +1193,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end end @@ -1258,7 +1216,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1266,7 +1224,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 0 sampling priority' do @@ -1277,7 +1234,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1285,7 +1242,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end context 'from 1 sampling priority' do @@ -1296,7 +1252,7 @@ tag_appsec_propagation: '1', tag_sampling_priority_condition: ->(x) { x == 2 } } - it_behaves_like 'a request with propagated headers', + it_behaves_like 'a request sent with propagated headers', { res_origin: 'rum', res_parent_id_not_equal: '34343434', @@ -1304,7 +1260,6 @@ res_sampling_priority_condition: ->(x) { x == '2' }, res_trace_id: '1212121212121212121' } - it_behaves_like 'a trace sent to agent with Datadog-Client-Computed-Stats header' end end end diff --git a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb index 63d8d0e769a..50e530f724d 100644 --- a/spec/datadog/appsec/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/appsec/contrib/support/integration/shared_examples.rb @@ -171,50 +171,13 @@ end RSpec.shared_examples 'a trace with ASM Standalone tags' do |params = {}| - let(:tag_apm_enabled) { params[:tag_apm_enabled] || 0 } + # Located in tracing shared examples + it_behaves_like 'a trace with APM disablement tags', params let(:tag_appsec_enabled) { params[:tag_appsec_enabled] || 1.0 } let(:tag_appsec_propagation) { params[:tag_appsec_propagation] } - let(:tag_other_propagation) { params[:tag_other_propagation] || :any } - # We use a lambda as we may change the comparison type - let(:tag_sampling_priority_condition) { params[:tag_sampling_priority_condition] || ->(x) { x == 0 } } - let(:tag_trace_id) { params[:tag_trace_id] || headers_trace_id.to_i } it do - expect(span.send(:metrics)['_dd.apm.enabled']).to eq(tag_apm_enabled) expect(span.send(:metrics)['_dd.appsec.enabled']).to eq(tag_appsec_enabled) - expect(span.send(:metrics)['_sampling_priority_v1']).to(satisfy { |x| tag_sampling_priority_condition.call(x) }) - expect(span.send(:meta)['_dd.p.appsec']).to eq(tag_appsec_propagation) - expect(span.send(:meta)['_dd.p.other']).to eq(tag_other_propagation) unless tag_other_propagation == :any - - expect(span.send(:trace_id)).to eq(tag_trace_id) - expect(traces.last.send(:spans)[0].send(:trace_id)).to eq(tag_trace_id) - end -end - -RSpec.shared_examples 'a request with propagated headers' do |params = {}| - let(:res_origin) { params[:res_origin] } - let(:res_parent_id_not_equal) { params[:res_parent_id_not_equal] } - let(:res_tags) { params[:res_tags] } - let(:res_sampling_priority_condition) { params[:res_sampling_priority_condition] || ->(x) { x.nil? } } - let(:res_trace_id) { params[:res_trace_id] } - - let(:res_headers) { JSON.parse(response.body) } - - it do - expect(res_headers['X-Datadog-Origin']).to eq(res_origin) - expect(res_headers['X-Datadog-Parent']).to_not eq(res_parent_id_not_equal) if res_parent_id_not_equal - expect(res_headers['X-Datadog-Sampling-Priority']).to(satisfy { |x| res_sampling_priority_condition.call(x) }) - expect(res_headers['X-Datadog-Trace-Id']).to eq(res_trace_id) - expect(res_headers['X-Datadog-Tags'].split(',')).to include(*res_tags) if res_tags - end -end - -RSpec.shared_examples 'a trace sent to agent with Datadog-Client-Computed-Stats header' do - let(:agent_tested_headers) { { 'Datadog-Client-Computed-Stats' => 'yes' } } - - it do - agent_return = agent_http_client.send_traces(traces) - expect(agent_return.first.ok?).to be true end end From 3ec6ff95bf4f405b1f8eb176a9c921c6a68e230d Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 10 Mar 2025 18:06:48 +0100 Subject: [PATCH 14/21] Delete ASM Standalone tests on Rails and Sinatra (ASM Standalone has no effect on them) --- .../contrib/rails/integration_test_spec.rb | 78 ------------------- .../contrib/sinatra/integration_test_spec.rb | 78 ------------------- 2 files changed, 156 deletions(-) diff --git a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb b/spec/datadog/appsec/contrib/rails/integration_test_spec.rb index 8933ac47d98..453ac9408db 100644 --- a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/rails/integration_test_spec.rb @@ -2,7 +2,6 @@ require 'datadog/tracing/contrib/rails/rails_helper' require 'datadog/appsec/contrib/support/integration/shared_examples' -require 'datadog/appsec/spec_helper' require 'rack/test' require 'datadog/tracing' @@ -11,33 +10,7 @@ RSpec.describe 'Rails integration tests' do include Rack::Test::Methods - # We send the trace to a mocked agent to verify that the trace includes the headers that we want - # In the future, it might be a good idea to use the traces that the mocked agent - # receives in the tests/shared examples - let(:agent_http_client) do - Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| - t.adapter agent_http_adapter - end - end - - let(:agent_http_adapter) { Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) } - - let(:agent_settings) do - Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( - adapter: nil, - ssl: false, - uds_path: nil, - hostname: 'localhost', - port: 6218, - timeout_seconds: 30, - ) - end - let(:sorted_spans) do - # We must format the trace to have the same result as the agent - # This is especially important for _sampling_priority_v1 metric - Datadog::Tracing::Transport::TraceFormatter.format!(trace) - chain = lambda do |start| loop.with_object([start]) do |_, o| # root reached (default) @@ -55,8 +28,6 @@ sort.call(spans) end - let(:agent_tested_headers) { {} } - let(:rack_span) { sorted_spans.reverse.find { |x| x.name == Datadog::Tracing::Contrib::Rack::Ext::SPAN_REQUEST } } let(:tracing_enabled) { true } @@ -64,7 +35,6 @@ let(:appsec_instrument_rack) { false } - let(:appsec_standalone_enabled) { false } let(:appsec_ip_denylist) { [] } let(:appsec_user_id_denylist) { [] } let(:appsec_ruleset) { :recommended } @@ -119,44 +89,16 @@ end before do - # It may have been better to add this endpoint to the Rails app, - # but I couldn't figure out how to call the Rails app from itself using Net::HTTP. - # Creating a WebMock and stubbing it was easier. - WebMock.enable! - stub_request(:get, 'http://localhost:3000/returnheaders') - .to_return do |request| - { - status: 200, - body: request.headers.to_json, - headers: { 'Content-Type' => 'application/json' } - } - end - - # Mocked agent with correct headers - stub_request(:post, 'http://localhost:6218/v0.4/traces') - .with do |request| - agent_tested_headers <= request.headers - end - .to_return(status: 200) - - # DEV: Would it be faster to do another stub for requests that don't match the headers - # rather than waiting for the TCP connection to fail? - - # TODO: Mocked agent that matches a given body, then use it in the shared examples, - # That way it would be real integration tests - Datadog.configure do |c| c.tracing.enabled = tracing_enabled c.tracing.instrument :rails - c.tracing.instrument :http c.appsec.enabled = appsec_enabled c.appsec.instrument :rails c.appsec.instrument :rack if appsec_instrument_rack - c.appsec.standalone.enabled = appsec_standalone_enabled c.appsec.waf_timeout = 10_000_000 # in us c.appsec.ip_denylist = appsec_ip_denylist c.appsec.user_id_denylist = appsec_user_id_denylist @@ -167,9 +109,6 @@ end after do - WebMock.reset! - WebMock.disable! - Datadog.configuration.reset! Datadog.registry[:rails].reset_configuration! end @@ -202,20 +141,6 @@ def set_user Datadog::Kit::Identity.set_user(Datadog::Tracing.active_trace, id: 'blocked-user-id') head :ok end - - def request_downstream - uri = URI('http://localhost:3000/returnheaders') - ext_request = nil - ext_response = nil - - Net::HTTP.start(uri.host, uri.port) do |http| - ext_request = Net::HTTP::Get.new('/returnheaders') - - ext_response = http.request(ext_request) - end - - render json: ext_response.body, content_type: 'application/json' - end end ) end @@ -241,7 +166,6 @@ def request_downstream '/success' => 'test#success', [:post, '/success'] => 'test#success', '/set_user' => 'test#set_user', - '/requestdownstream' => 'test#request_downstream', } end @@ -559,8 +483,6 @@ def request_downstream end end end - - it_behaves_like 'appsec standalone billing' end end end diff --git a/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb b/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb index 238c49e7296..df327460fdc 100644 --- a/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb @@ -1,6 +1,5 @@ require 'datadog/tracing/contrib/support/spec_helper' require 'datadog/appsec/contrib/support/integration/shared_examples' -require 'datadog/appsec/spec_helper' require 'rack/test' require 'securerandom' @@ -19,33 +18,7 @@ RSpec.describe 'Sinatra integration tests' do include Rack::Test::Methods - # We send the trace to a mocked agent to verify that the trace includes the headers that we want - # In the future, it might be a good idea to use the traces that the mocked agent - # receives in the tests/shared examples - let(:agent_http_client) do - Datadog::Tracing::Transport::HTTP.default(agent_settings: test_agent_settings) do |t| - t.adapter agent_http_adapter - end - end - - let(:agent_http_adapter) { Datadog::Core::Transport::HTTP::Adapters::Net.new(agent_settings) } - - let(:agent_settings) do - Datadog::Core::Configuration::AgentSettingsResolver::AgentSettings.new( - adapter: nil, - ssl: false, - uds_path: nil, - hostname: 'localhost', - port: 6218, - timeout_seconds: 30, - ) - end - let(:sorted_spans) do - # We must format the trace to have the same result as the agent - # This is especially important for _sampling_priority_v1 metric - Datadog::Tracing::Transport::TraceFormatter.format!(trace) - chain = lambda do |start| loop.with_object([start]) do |_, o| # root reached (default) @@ -62,9 +35,6 @@ sort = ->(list) { list.sort_by { |e| chain.call(e).count } } sort.call(spans) end - - let(:agent_tested_headers) { {} } - let(:sinatra_span) { sorted_spans.reverse.find { |x| x.name == Datadog::Tracing::Contrib::Sinatra::Ext::SPAN_REQUEST } } let(:route_span) { sorted_spans.find { |x| x.name == Datadog::Tracing::Contrib::Sinatra::Ext::SPAN_ROUTE } } let(:rack_span) { sorted_spans.reverse.find { |x| x.name == Datadog::Tracing::Contrib::Rack::Ext::SPAN_REQUEST } } @@ -72,7 +42,6 @@ let(:tracing_enabled) { true } let(:appsec_enabled) { true } - let(:appsec_standalone_enabled) { false } let(:appsec_ip_denylist) { [] } let(:appsec_user_id_denylist) { [] } let(:appsec_ruleset) { :recommended } @@ -127,41 +96,15 @@ end before do - WebMock.enable! - stub_request(:get, 'http://localhost:3000/returnheaders') - .to_return do |request| - { - status: 200, - body: request.headers.to_json, - headers: { 'Content-Type' => 'application/json' } - } - end - - # Mocked agent with correct headers - stub_request(:post, 'http://localhost:6218/v0.4/traces') - .with do |request| - agent_tested_headers <= request.headers - end - .to_return(status: 200) - - # DEV: Would it be faster to do another stub for requests that don't match the headers - # rather than waiting for the TCP connection to fail? - - # TODO: Mocked agent that matches a given body, then use it in the shared examples, - # That way it would be real integration tests - Datadog.configure do |c| c.tracing.enabled = tracing_enabled c.tracing.instrument :sinatra - c.tracing.instrument :http c.appsec.enabled = appsec_enabled c.appsec.instrument :sinatra - # TODO: test with c.appsec.instrument :rack - c.appsec.standalone.enabled = appsec_standalone_enabled c.appsec.waf_timeout = 10_000_000 # in us c.appsec.ip_denylist = appsec_ip_denylist c.appsec.user_id_denylist = appsec_user_id_denylist @@ -172,9 +115,6 @@ end after do - WebMock.reset! - WebMock.disable! - Datadog.configuration.reset! Datadog.registry[:rack].reset_configuration! Datadog.registry[:sinatra].reset_configuration! @@ -225,22 +165,6 @@ Datadog::Kit::Identity.set_user(Datadog::Tracing.active_trace, id: 'blocked-user-id') 'ok' end - - get '/requestdownstream' do - content_type :json - - uri = URI('http://localhost:3000/returnheaders') - ext_request = nil - ext_response = nil - - Net::HTTP.start(uri.host, uri.port) do |http| - ext_request = Net::HTTP::Get.new(uri) - - ext_response = http.request(ext_request) - end - - ext_response.body - end end end @@ -472,8 +396,6 @@ it_behaves_like 'a trace with AppSec api security tags' end end - - it_behaves_like 'appsec standalone billing' end end end From 6c863cd8e4c6b1d4d9b1a16132ff2a37a8b91834 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 10 Mar 2025 18:10:06 +0100 Subject: [PATCH 15/21] Remove forced-tests from git conflict resolution --- .github/forced-tests-list.json | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/.github/forced-tests-list.json b/.github/forced-tests-list.json index 098be4f2815..951b0f89926 100644 --- a/.github/forced-tests-list.json +++ b/.github/forced-tests-list.json @@ -1,15 +1,4 @@ { - "AGENT_NOT_SUPPORTING_SPAN_EVENTS": - [ - "tests/test_span_events.py" - ], - "PARAMETRIC": - [ - "tests/parametric/test_span_events.py" - ], - "DEFAULT": [ - "tests/test_graphql.py" - ], "SCA_STANDALONE": [ "tests/appsec/test_asm_standalone.py::Test_SCAStandalone_Telemetry" ] From ce8a3638c2ff83b6da9ded1d99ff77f6cd6b520c Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Tue, 11 Mar 2025 11:12:12 +0100 Subject: [PATCH 16/21] Rename build_above_second_post_sampler to build_rate_limit_post_sampler --- lib/datadog/tracing/component.rb | 4 ++-- sig/datadog/tracing/component.rbs | 2 +- spec/datadog/tracing/contrib/rack/integration_test_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index 2ba362bd9b2..8511490d10b 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -79,7 +79,7 @@ def build_sampler(settings) # so we want to send the minimum amount of traces possible (idealy only traces that contains security events), # but for features such as API Security, we need to send at least one trace per minute, # to keep the service alive on the backend side. - post_sampler = build_above_second_post_sampler(60) if settings.appsec.standalone.enabled + post_sampler = build_rate_limit_post_sampler(seconds: 60) if settings.appsec.standalone.enabled # Sampling rules are provided if (rules = settings.tracing.sampling.rules) @@ -191,7 +191,7 @@ def build_tracer_tags(settings) end end - def build_above_second_post_sampler(seconds) + def build_rate_limit_post_sampler(seconds:) Tracing::Sampling::RuleSampler.new( rate_limiter: Datadog::Core::TokenBucket.new(1.0 / seconds, 1.0), default_sample_rate: 1.0 diff --git a/sig/datadog/tracing/component.rbs b/sig/datadog/tracing/component.rbs index dcf2324b8c6..07004bbb398 100644 --- a/sig/datadog/tracing/component.rbs +++ b/sig/datadog/tracing/component.rbs @@ -22,7 +22,7 @@ module Datadog def build_tracer_tags: (untyped settings) -> untyped - def build_above_second_post_sampler: (Integer seconds) -> Datadog::Tracing::Sampling::RuleSampler + def build_rate_limit_post_sampler: (seconds: Integer) -> Datadog::Tracing::Sampling::RuleSampler def build_test_mode_trace_flush: (untyped settings) -> untyped diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index 152a24291ed..8ef38c53338 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -49,8 +49,8 @@ # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 so tests are faster if apm_tracing_disabled - post_sampler = Datadog::Core::Configuration::Components.send(:build_above_second_post_sampler, 4) - allow(Datadog::Core::Configuration::Components).to receive(:build_above_second_post_sampler).and_return(post_sampler) + post_sampler = Datadog::Core::Configuration::Components.send(:build_rate_limit_post_sampler, **{seconds: 4}) + allow(Datadog::Core::Configuration::Components).to receive(:build_rate_limit_post_sampler).and_return(post_sampler) end unless remote_enabled From 3df2d615791f868e42084416a6a423539a79f7a9 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Tue, 11 Mar 2025 13:45:53 +0100 Subject: [PATCH 17/21] Move before block in context --- .../contrib/rack/integration_test_spec.rb | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index 8ef38c53338..a26c19703fe 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -441,15 +441,14 @@ end end - before do - is_expected.to be_ok - expected_spans_number = instrument_http ? 2 : 1 - # expect(spans).to have(expected_spans_number).items - end - describe 'GET request' do subject(:response) { get route } + before do + is_expected.to be_ok + expect(spans).to have(1).items + end + context 'without parameters' do let(:route) { '/success/' } @@ -620,6 +619,11 @@ describe 'POST request' do subject(:response) { post route } + before do + is_expected.to be_ok + expect(spans).to have(1).items + end + context 'without parameters' do let(:route) { '/success/' } @@ -645,6 +649,10 @@ end describe 'APM disablement' do + before do + is_expected.to be_ok + end + let(:url) { '/requestdownstream' } let(:params) { {} } let(:headers) do From c2ff1b698a29fbc3305a015b1623d1e228143cd2 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Tue, 11 Mar 2025 14:09:45 +0100 Subject: [PATCH 18/21] Rubocop --- lib/datadog/tracing/contrib/ethon/easy_patch.rb | 5 +++-- lib/datadog/tracing/contrib/excon/middleware.rb | 5 +++-- lib/datadog/tracing/contrib/faraday/middleware.rb | 5 +++-- .../tracing/contrib/grpc/datadog_interceptor/client.rb | 5 +++-- lib/datadog/tracing/contrib/http/circuit_breaker.rb | 5 +++-- lib/datadog/tracing/contrib/httpclient/instrumentation.rb | 5 +++-- lib/datadog/tracing/contrib/httprb/instrumentation.rb | 5 +++-- lib/datadog/tracing/contrib/rest_client/request_patch.rb | 5 +++-- lib/datadog/tracing/contrib/sidekiq/client_tracer.rb | 5 +++-- lib/datadog/tracing/tracer.rb | 5 ++++- spec/datadog/tracing/contrib/rack/integration_test_spec.rb | 6 +++--- .../tracing/contrib/support/integration/shared_examples.rb | 1 - 12 files changed, 34 insertions(+), 23 deletions(-) diff --git a/lib/datadog/tracing/contrib/ethon/easy_patch.rb b/lib/datadog/tracing/contrib/ethon/easy_patch.rb index 423e47491a4..d120311e72e 100644 --- a/lib/datadog/tracing/contrib/ethon/easy_patch.rb +++ b/lib/datadog/tracing/contrib/ethon/easy_patch.rb @@ -219,8 +219,9 @@ def parse_response_headers # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !datadog_configuration[:distributed_tracing] diff --git a/lib/datadog/tracing/contrib/excon/middleware.rb b/lib/datadog/tracing/contrib/excon/middleware.rb index 465ecf6dbce..c96d7c04001 100644 --- a/lib/datadog/tracing/contrib/excon/middleware.rb +++ b/lib/datadog/tracing/contrib/excon/middleware.rb @@ -193,8 +193,9 @@ def datadog_configuration(host = :default) # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !distributed_tracing? diff --git a/lib/datadog/tracing/contrib/faraday/middleware.rb b/lib/datadog/tracing/contrib/faraday/middleware.rb index 9f1b214d109..cb9d591f94e 100644 --- a/lib/datadog/tracing/contrib/faraday/middleware.rb +++ b/lib/datadog/tracing/contrib/faraday/middleware.rb @@ -118,8 +118,9 @@ def datadog_configuration(host = :default) # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !client_config[:distributed_tracing] diff --git a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb index ab69d364c1f..2aff6f833e6 100644 --- a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb +++ b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb @@ -113,8 +113,9 @@ def find_host_port(call) # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !distributed_tracing? diff --git a/lib/datadog/tracing/contrib/http/circuit_breaker.rb b/lib/datadog/tracing/contrib/http/circuit_breaker.rb index 0598e708b11..5563b6e31ba 100644 --- a/lib/datadog/tracing/contrib/http/circuit_breaker.rb +++ b/lib/datadog/tracing/contrib/http/circuit_breaker.rb @@ -31,8 +31,9 @@ def internal_request?(request) # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) diff --git a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb index 6dc9718c5ad..37607aab322 100644 --- a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb @@ -122,8 +122,9 @@ def analytics_enabled?(request_options) # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) diff --git a/lib/datadog/tracing/contrib/httprb/instrumentation.rb b/lib/datadog/tracing/contrib/httprb/instrumentation.rb index 22ccf99e01f..452bf038941 100644 --- a/lib/datadog/tracing/contrib/httprb/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httprb/instrumentation.rb @@ -136,8 +136,9 @@ def logger # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) diff --git a/lib/datadog/tracing/contrib/rest_client/request_patch.rb b/lib/datadog/tracing/contrib/rest_client/request_patch.rb index def7327a022..1af493c45af 100644 --- a/lib/datadog/tracing/contrib/rest_client/request_patch.rb +++ b/lib/datadog/tracing/contrib/rest_client/request_patch.rb @@ -125,8 +125,9 @@ def analytics_sample_rate # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !datadog_configuration[:distributed_tracing] diff --git a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb index 380747ff992..69bfbe58fa8 100644 --- a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb +++ b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb @@ -59,8 +59,9 @@ def configuration # Skips distributed tracing if disabled for this instrumentation # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled - return true unless trace && trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) == '1' + if Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true end !configuration[:distributed_tracing] diff --git a/lib/datadog/tracing/tracer.rb b/lib/datadog/tracing/tracer.rb index 229e438aead..418f0ea74b3 100644 --- a/lib/datadog/tracing/tracer.rb +++ b/lib/datadog/tracing/tracer.rb @@ -558,7 +558,10 @@ def skip_trace(name) # DEV: dd.p.appsec will be replaced by dd.p.ts in APM disablement 2.0, which will be a bitmask. def propagate_sampling_priority?(upstream_tags:) return true unless apm_tracing_disabled - return Datadog.configuration.appsec.enabled if upstream_tags&.key?(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) + + if upstream_tags&.key?(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) + return Datadog.configuration.appsec.enabled + end false end diff --git a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb index a26c19703fe..60008d99947 100644 --- a/spec/datadog/tracing/contrib/rack/integration_test_spec.rb +++ b/spec/datadog/tracing/contrib/rack/integration_test_spec.rb @@ -49,7 +49,7 @@ # Sampler with the same settings as APM disabled one, except it is 4 seconds instead of 60 so tests are faster if apm_tracing_disabled - post_sampler = Datadog::Core::Configuration::Components.send(:build_rate_limit_post_sampler, **{seconds: 4}) + post_sampler = Datadog::Core::Configuration::Components.send(:build_rate_limit_post_sampler, **{ seconds: 4 }) allow(Datadog::Core::Configuration::Components).to receive(:build_rate_limit_post_sampler).and_return(post_sampler) end @@ -706,7 +706,7 @@ end agent_return = agent_http_client.send_traces(traces) - expect(JSON.parse(agent_return.first.payload)["Datadog-Client-Computed-Stats"]).to eq('yes') + expect(JSON.parse(agent_return.first.payload)['Datadog-Client-Computed-Stats']).to eq('yes') end end @@ -770,7 +770,7 @@ get '/success/' end - let (:env) { {} } + let(:env) { {} } it do expect(traces[0].sampling_priority).to eq(2) diff --git a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb index dc7c43bc1fa..30e250935f1 100644 --- a/spec/datadog/tracing/contrib/support/integration/shared_examples.rb +++ b/spec/datadog/tracing/contrib/support/integration/shared_examples.rb @@ -35,6 +35,5 @@ end RSpec.shared_examples 'a request sent without propagated headers' do - it_behaves_like 'a request sent with propagated headers', {} end From a9473aaf42904061fdd58f11b5f0a00b33c921fe Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Tue, 11 Mar 2025 17:44:33 +0100 Subject: [PATCH 19/21] Changed system-tests scenarios to Experimental mode --- .github/forced-tests-list.json | 2 +- .github/workflows/system-tests.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/forced-tests-list.json b/.github/forced-tests-list.json index 951b0f89926..b3ed615518b 100644 --- a/.github/forced-tests-list.json +++ b/.github/forced-tests-list.json @@ -1,5 +1,5 @@ { - "SCA_STANDALONE": [ + "SCA_STANDALONE_EXPERIMENTAL": [ "tests/appsec/test_asm_standalone.py::Test_SCAStandalone_Telemetry" ] } diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 93fe0aad31e..c74a9c3f038 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -263,7 +263,7 @@ jobs: - APPSEC_DISABLED - APPSEC_BLOCKING_FULL_DENYLIST - APPSEC_REQUEST_BLOCKING - - APPSEC_STANDALONE + - APPSEC_STANDALONE_EXPERIMENTAL - APPSEC_BLOCKING include: - library: ruby @@ -322,7 +322,7 @@ jobs: scenario: DEBUGGER_PII_REDACTION - library: ruby app: rails70 - scenario: SCA_STANDALONE + scenario: SCA_STANDALONE_EXPERIMENTAL - library: ruby app: rack scenario: SAMPLING From 91f2f68de528681a94a02e45a89024f4fb8c4423 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Wed, 12 Mar 2025 13:32:30 +0100 Subject: [PATCH 20/21] Add should_skip_distributed_tracing? method --- .../tracing/contrib/ethon/easy_patch.rb | 16 +--- .../tracing/contrib/excon/middleware.rb | 18 ++--- .../tracing/contrib/faraday/middleware.rb | 18 ++--- .../grpc/datadog_interceptor/client.rb | 18 ++--- .../contrib/grpc/distributed/propagation.rb | 1 + .../tracing/contrib/http/circuit_breaker.rb | 13 ---- .../contrib/http/distributed/propagation.rb | 1 + .../tracing/contrib/http/instrumentation.rb | 6 +- .../contrib/httpclient/instrumentation.rb | 19 ++--- .../tracing/contrib/httprb/instrumentation.rb | 19 ++--- .../contrib/rest_client/request_patch.rb | 18 ++--- .../tracing/contrib/sidekiq/client_tracer.rb | 18 ++--- .../sidekiq/distributed/propagation.rb | 1 + .../tracing/distributed/circuit_breaker.rb | 26 +++++++ .../tracing/contrib/http/circuit_breaker.rbs | 2 - .../contrib/httpclient/instrumentation.rbs | 2 - .../contrib/httprb/instrumentation.rbs | 2 - .../tracing/distributed/circuit_breaker.rbs | 9 +++ .../contrib/http/circuit_breaker_spec.rb | 64 --------------- .../distributed/circuit_breaker_spec.rb | 77 +++++++++++++++++++ 20 files changed, 164 insertions(+), 184 deletions(-) create mode 100644 lib/datadog/tracing/distributed/circuit_breaker.rb create mode 100644 sig/datadog/tracing/distributed/circuit_breaker.rbs create mode 100644 spec/datadog/tracing/distributed/circuit_breaker_spec.rb diff --git a/lib/datadog/tracing/contrib/ethon/easy_patch.rb b/lib/datadog/tracing/contrib/ethon/easy_patch.rb index d120311e72e..bdff647c328 100644 --- a/lib/datadog/tracing/contrib/ethon/easy_patch.rb +++ b/lib/datadog/tracing/contrib/ethon/easy_patch.rb @@ -110,7 +110,10 @@ def datadog_before_request(continue_from: nil) datadog_tag_request - unless should_skip_distributed_tracing?(datadog_trace) + unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: datadog_configuration, + trace: datadog_trace + ) @datadog_original_headers ||= {} Contrib::HTTP.inject(datadog_trace, @datadog_original_headers) self.headers = @datadog_original_headers @@ -215,17 +218,6 @@ def parse_response_headers header.size != 2 ? nil : header end.compact.to_h end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !datadog_configuration[:distributed_tracing] - end end end end diff --git a/lib/datadog/tracing/contrib/excon/middleware.rb b/lib/datadog/tracing/contrib/excon/middleware.rb index c96d7c04001..982fd5b25d5 100644 --- a/lib/datadog/tracing/contrib/excon/middleware.rb +++ b/lib/datadog/tracing/contrib/excon/middleware.rb @@ -30,7 +30,12 @@ def request_call(datum) trace = Tracing.active_trace datum[:datadog_span] = span annotate!(span, datum) - propagate!(trace, span, datum) unless should_skip_distributed_tracing?(trace) + if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: @options, + trace: trace + ) + propagate!(trace, span, datum) + end span end @@ -189,17 +194,6 @@ def build_request_options!(datum) def datadog_configuration(host = :default) Datadog.configuration.tracing[:excon, host] end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !distributed_tracing? - end end end end diff --git a/lib/datadog/tracing/contrib/faraday/middleware.rb b/lib/datadog/tracing/contrib/faraday/middleware.rb index cb9d591f94e..a402c60781f 100644 --- a/lib/datadog/tracing/contrib/faraday/middleware.rb +++ b/lib/datadog/tracing/contrib/faraday/middleware.rb @@ -29,7 +29,12 @@ def call(env) Tracing.trace(Ext::SPAN_REQUEST, on_error: request_options[:on_error]) do |span, trace| annotate!(span, env, request_options) - propagate!(trace, span, env) if Tracing.enabled? && !should_skip_distributed_tracing?(request_options, trace) + if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: request_options, + trace: trace + ) + propagate!(trace, span, env) + end app.call(env).on_complete { |resp| handle_response(span, resp, request_options) } end end @@ -114,17 +119,6 @@ def build_request_options!(env) def datadog_configuration(host = :default) Datadog.configuration.tracing[:faraday, host] end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !client_config[:distributed_tracing] - end end end end diff --git a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb index 2aff6f833e6..d73053242a1 100644 --- a/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb +++ b/lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb @@ -81,7 +81,12 @@ def annotate!(trace, span, keywords, formatter) # Set analytics sample rate Contrib::Analytics.set_sample_rate(span, analytics_sample_rate) if analytics_enabled? - GRPC.inject(trace, metadata) unless should_skip_distributed_tracing?(trace) + unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: Datadog.configuration_for(self) || datadog_configuration, + trace: trace + ) + GRPC.inject(trace, metadata) + end Contrib::SpanAttributeSchema.set_peer_service!(span, Ext::PEER_SERVICE_SOURCES) rescue StandardError => e Datadog.logger.debug("GRPC client trace failed: #{e}") @@ -109,17 +114,6 @@ def find_host_port(call) Datadog.logger.debug { "Could not parse host:port from #{call}: #{e}" } nil end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !distributed_tracing? - end end end end diff --git a/lib/datadog/tracing/contrib/grpc/distributed/propagation.rb b/lib/datadog/tracing/contrib/grpc/distributed/propagation.rb index c5b96810424..191bfdce087 100644 --- a/lib/datadog/tracing/contrib/grpc/distributed/propagation.rb +++ b/lib/datadog/tracing/contrib/grpc/distributed/propagation.rb @@ -3,6 +3,7 @@ require_relative 'fetcher' require_relative '../../../distributed/b3_multi' require_relative '../../../distributed/b3_single' +require_relative '../../../distributed/circuit_breaker' require_relative '../../../distributed/datadog' require_relative '../../../distributed/none' require_relative '../../../distributed/propagation' diff --git a/lib/datadog/tracing/contrib/http/circuit_breaker.rb b/lib/datadog/tracing/contrib/http/circuit_breaker.rb index 5563b6e31ba..11f1db27d65 100644 --- a/lib/datadog/tracing/contrib/http/circuit_breaker.rb +++ b/lib/datadog/tracing/contrib/http/circuit_breaker.rb @@ -27,19 +27,6 @@ def internal_request?(request) !!(request[Datadog::Core::Transport::Ext::HTTP::HEADER_META_TRACER_VERSION] || request[Datadog::Core::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST]) end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) - - !Datadog.configuration.tracing[:http][:distributed_tracing] - end end end end diff --git a/lib/datadog/tracing/contrib/http/distributed/propagation.rb b/lib/datadog/tracing/contrib/http/distributed/propagation.rb index 0a2ca41dd6a..316655474d3 100644 --- a/lib/datadog/tracing/contrib/http/distributed/propagation.rb +++ b/lib/datadog/tracing/contrib/http/distributed/propagation.rb @@ -2,6 +2,7 @@ require_relative 'fetcher' require_relative '../../../distributed/propagation' +require_relative '../../../distributed/circuit_breaker' require_relative '../../../distributed/b3_multi' require_relative '../../../distributed/b3_single' require_relative '../../../distributed/datadog' diff --git a/lib/datadog/tracing/contrib/http/instrumentation.rb b/lib/datadog/tracing/contrib/http/instrumentation.rb index cb0c0c15aba..bc17f65ab4b 100644 --- a/lib/datadog/tracing/contrib/http/instrumentation.rb +++ b/lib/datadog/tracing/contrib/http/instrumentation.rb @@ -35,7 +35,11 @@ def request(req, body = nil, &block) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND span.resource = req.method - if Tracing.enabled? && !Contrib::HTTP.should_skip_distributed_tracing?(client_config, trace) + if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + client_config: client_config, + datadog_config: Datadog.configuration.tracing[:http], + trace: trace + ) Contrib::HTTP.inject(trace, req) end diff --git a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb index 37607aab322..9b8dda20307 100644 --- a/lib/datadog/tracing/contrib/httpclient/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httpclient/instrumentation.rb @@ -30,7 +30,11 @@ def do_get_block(req, proxy, conn, &block) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - if Tracing.enabled? && !should_skip_distributed_tracing?(client_config, trace) + if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + client_config: client_config, + datadog_config: Datadog.configuration.tracing[:httpclient], + trace: trace + ) Contrib::HTTP.inject(trace, req.header) end @@ -119,19 +123,6 @@ def analytics_enabled?(request_options) Contrib::Analytics.enabled?(request_options[:analytics_enabled]) end - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) - - !Datadog.configuration.tracing[:httpclient][:distributed_tracing] - end - def set_analytics_sample_rate(span, request_options) return unless analytics_enabled?(request_options) diff --git a/lib/datadog/tracing/contrib/httprb/instrumentation.rb b/lib/datadog/tracing/contrib/httprb/instrumentation.rb index 452bf038941..775b027cce6 100644 --- a/lib/datadog/tracing/contrib/httprb/instrumentation.rb +++ b/lib/datadog/tracing/contrib/httprb/instrumentation.rb @@ -30,7 +30,11 @@ def perform(req, options) span.service = service_name(host, request_options, client_config) span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND - if Tracing.enabled? && !should_skip_distributed_tracing?(client_config, trace) + if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + client_config: client_config, + datadog_config: Datadog.configuration.tracing[:httprb], + trace: trace + ) Contrib::HTTP.inject(trace, req) end @@ -133,19 +137,6 @@ def logger Datadog.logger end - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(client_config, trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) - - !Datadog.configuration.tracing[:httprb][:distributed_tracing] - end - def set_analytics_sample_rate(span, request_options) return unless analytics_enabled?(request_options) diff --git a/lib/datadog/tracing/contrib/rest_client/request_patch.rb b/lib/datadog/tracing/contrib/rest_client/request_patch.rb index 1af493c45af..a8a703d664a 100644 --- a/lib/datadog/tracing/contrib/rest_client/request_patch.rb +++ b/lib/datadog/tracing/contrib/rest_client/request_patch.rb @@ -25,7 +25,12 @@ def execute(&block) return super(&block) unless Tracing.enabled? datadog_trace_request(uri) do |_span, trace| - Contrib::HTTP.inject(trace, processed_headers) unless should_skip_distributed_tracing?(trace) + unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: datadog_configuration, + trace: trace + ) + Contrib::HTTP.inject(trace, processed_headers) + end super(&block) end @@ -121,17 +126,6 @@ def analytics_enabled? def analytics_sample_rate datadog_configuration[:analytics_sample_rate] end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !datadog_configuration[:distributed_tracing] - end end end end diff --git a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb index 69bfbe58fa8..87ac65bd087 100644 --- a/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb +++ b/lib/datadog/tracing/contrib/sidekiq/client_tracer.rb @@ -24,7 +24,12 @@ def call(worker_class, job, queue, redis_pool) resource = job_resource(job) Datadog::Tracing.trace(Ext::SPAN_PUSH, service: @sidekiq_service) do |span, trace_op| - Sidekiq.inject(trace_op, job) unless should_skip_distributed_tracing?(trace_op) + unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?( + datadog_config: configuration, + trace: trace_op + ) + Sidekiq.inject(trace_op, job) + end span.resource = resource @@ -55,17 +60,6 @@ def call(worker_class, job, queue, redis_pool) def configuration Datadog.configuration.tracing[:sidekiq] end - - # Skips distributed tracing if disabled for this instrumentation - # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) - def should_skip_distributed_tracing?(trace) - if Datadog.configuration.appsec.standalone.enabled && - (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') - return true - end - - !configuration[:distributed_tracing] - end end end end diff --git a/lib/datadog/tracing/contrib/sidekiq/distributed/propagation.rb b/lib/datadog/tracing/contrib/sidekiq/distributed/propagation.rb index 678c16db4f8..f3dd6a200ce 100644 --- a/lib/datadog/tracing/contrib/sidekiq/distributed/propagation.rb +++ b/lib/datadog/tracing/contrib/sidekiq/distributed/propagation.rb @@ -2,6 +2,7 @@ require_relative '../../../distributed/fetcher' require_relative '../../../distributed/propagation' +require_relative '../../../distributed/circuit_breaker' require_relative '../../../distributed/b3_multi' require_relative '../../../distributed/b3_single' require_relative '../../../distributed/datadog' diff --git a/lib/datadog/tracing/distributed/circuit_breaker.rb b/lib/datadog/tracing/distributed/circuit_breaker.rb new file mode 100644 index 00000000000..ae04ddcd85f --- /dev/null +++ b/lib/datadog/tracing/distributed/circuit_breaker.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Datadog + module Tracing + module Distributed + # Helper method to decide when to skip distributed tracing + module CircuitBreaker + module_function + + # Skips distributed tracing if disabled for this instrumentation + # or if APM is disabled unless there is an AppSec event (from upstream distributed trace or local) + def should_skip_distributed_tracing?(client_config: nil, datadog_config: nil, trace: nil) + if ::Datadog.configuration.appsec.standalone.enabled && + (trace.nil? || trace.get_tag(::Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') + return true + end + + return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing) + return !datadog_config[:distributed_tracing] if datadog_config + + false + end + end + end + end +end diff --git a/sig/datadog/tracing/contrib/http/circuit_breaker.rbs b/sig/datadog/tracing/contrib/http/circuit_breaker.rbs index 62ecc351a8a..bc1a05213ff 100644 --- a/sig/datadog/tracing/contrib/http/circuit_breaker.rbs +++ b/sig/datadog/tracing/contrib/http/circuit_breaker.rbs @@ -5,8 +5,6 @@ module Datadog module CircuitBreaker def should_skip_tracing?: (untyped request) -> (true | false) def internal_request?: (untyped request) -> bool - - def should_skip_distributed_tracing?: (untyped client_config) -> untyped end end end diff --git a/sig/datadog/tracing/contrib/httpclient/instrumentation.rbs b/sig/datadog/tracing/contrib/httpclient/instrumentation.rbs index 44a3a3c40d2..ce0f5210536 100644 --- a/sig/datadog/tracing/contrib/httpclient/instrumentation.rbs +++ b/sig/datadog/tracing/contrib/httpclient/instrumentation.rbs @@ -23,8 +23,6 @@ module Datadog def logger: () -> untyped - def should_skip_distributed_tracing?: (untyped client_config) -> untyped - def set_analytics_sample_rate: (untyped span, untyped request_options) -> (nil | untyped) end end diff --git a/sig/datadog/tracing/contrib/httprb/instrumentation.rbs b/sig/datadog/tracing/contrib/httprb/instrumentation.rbs index 1fad4e2b94e..16a4ba2133f 100644 --- a/sig/datadog/tracing/contrib/httprb/instrumentation.rbs +++ b/sig/datadog/tracing/contrib/httprb/instrumentation.rbs @@ -23,8 +23,6 @@ module Datadog def logger: () -> untyped - def should_skip_distributed_tracing?: (untyped client_config) -> untyped - def set_analytics_sample_rate: (untyped span, untyped request_options) -> (nil | untyped) end end diff --git a/sig/datadog/tracing/distributed/circuit_breaker.rbs b/sig/datadog/tracing/distributed/circuit_breaker.rbs new file mode 100644 index 00000000000..f76149dcb8a --- /dev/null +++ b/sig/datadog/tracing/distributed/circuit_breaker.rbs @@ -0,0 +1,9 @@ +module Datadog + module Tracing + module Distributed + module CircuitBreaker + def self.should_skip_distributed_tracing?: (?client_config: Hash[Symbol, untyped]?, ?datadog_config: Datadog::Tracing::Contrib::Configuration::Settings?, ?trace: Datadog::Tracing::TraceOperation?) -> bool + end + end + end +end diff --git a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb index 783cb983daa..4d656c7ed87 100644 --- a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb +++ b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb @@ -91,68 +91,4 @@ end end end - - describe '#should_skip_distributed_tracing?' do - subject(:should_skip_distributed_tracing?) { circuit_breaker.should_skip_distributed_tracing?(client_config, trace) } - - let(:client_config) { nil } - let(:distributed_tracing) { true } - let(:appsec_standalone) { false } - let(:trace) { nil } - let(:distributed_appsec_event) { nil } - - before do - allow(Datadog.configuration.tracing[:http]).to receive(:[]).with(:distributed_tracing).and_return(distributed_tracing) - allow(Datadog.configuration.appsec.standalone).to receive(:enabled).and_return(appsec_standalone) - allow(Datadog::Tracing).to receive(:trace).and_return(trace) - allow(trace).to receive(:get_tag).with('_dd.p.appsec').and_return(distributed_appsec_event) if trace - end - - context 'when distributed tracing is enabled' do - it { is_expected.to be false } - end - - context 'when distributed tracing is disabled' do - let(:distributed_tracing) { false } - - it { is_expected.to be true } - end - - context 'when appsec standalone is enabled' do - let(:appsec_standalone) { true } - - context 'when there is no active trace' do - it { is_expected.to be true } - end - - context 'when there is an active trace' do - let(:trace) { instance_double(Datadog::Tracing::TraceOperation) } - - context 'when the active trace has no distributed appsec event' do - it { is_expected.to be true } - end - - context 'when the active trace has a distributed appsec event' do - # This should act like standalone appsec is disabled, as it does not return in the - # `if Datadog.configuration.appsec.standalone.enabled` block - # so we're only testing the "no client config, distributed tracing enabled" case here - let(:distributed_appsec_event) { '1' } - - it { is_expected.to be false } - end - end - end - - context 'given a client config with distributed_tracing disabled' do - let(:client_config) { { distributed_tracing: false } } - - it { is_expected.to be true } - end - - context 'given a client config with distributed_tracing enabled' do - let(:client_config) { { distributed_tracing: true } } - - it { is_expected.to be false } - end - end end diff --git a/spec/datadog/tracing/distributed/circuit_breaker_spec.rb b/spec/datadog/tracing/distributed/circuit_breaker_spec.rb new file mode 100644 index 00000000000..a44bc422072 --- /dev/null +++ b/spec/datadog/tracing/distributed/circuit_breaker_spec.rb @@ -0,0 +1,77 @@ +require 'datadog/tracing/contrib/support/spec_helper' + +require 'datadog' +require 'datadog/tracing/distributed/circuit_breaker' + +RSpec.describe Datadog::Tracing::Distributed::CircuitBreaker do + subject(:circuit_breaker) { circuit_breaker_class.new } + + let(:circuit_breaker_class) { Class.new { include Datadog::Tracing::Distributed::CircuitBreaker } } + + describe '#should_skip_distributed_tracing?' do + subject(:should_skip_distributed_tracing?) do + circuit_breaker.send( + :should_skip_distributed_tracing?, + **{ client_config: client_config, datadog_config: datadog_config, trace: trace } + ) + end + + let(:client_config) { nil } + let(:datadog_config) { { distributed_tracing: true } } + let(:appsec_standalone) { false } + let(:trace) { nil } + let(:distributed_appsec_event) { nil } + + before do + allow(Datadog.configuration.appsec.standalone).to receive(:enabled).and_return(appsec_standalone) + allow(trace).to receive(:get_tag).with('_dd.p.appsec').and_return(distributed_appsec_event) if trace + end + + context 'when distributed tracing in datadog_config is enabled' do + it { is_expected.to be false } + end + + context 'when distributed tracing in datadog_config is disabled' do + let(:datadog_config) { { distributed_tracing: false } } + + it { is_expected.to be true } + end + + context 'when appsec standalone is enabled' do + let(:appsec_standalone) { true } + + context 'when there is no active trace' do + it { is_expected.to be true } + end + + context 'when there is an active trace' do + let(:trace) { instance_double(Datadog::Tracing::TraceOperation) } + + context 'when the active trace has no distributed appsec event' do + it { is_expected.to be true } + end + + context 'when the active trace has a distributed appsec event' do + # This should act like standalone appsec is disabled, as it does not return in the + # `if Datadog.configuration.appsec.standalone.enabled` block + # so we're only testing the "no client config, distributed tracing enabled" case here + let(:distributed_appsec_event) { '1' } + + it { is_expected.to be false } + end + end + end + + context 'given a client config with distributed_tracing disabled' do + let(:client_config) { { distributed_tracing: false } } + + it { is_expected.to be true } + end + + context 'given a client config with distributed_tracing enabled' do + let(:client_config) { { distributed_tracing: true } } + + it { is_expected.to be false } + end + end +end From def4da4ff4112aba5aff7fcf8536c6e22d816419 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Wed, 12 Mar 2025 15:22:20 +0100 Subject: [PATCH 21/21] Fix circuit breaker spec --- spec/datadog/tracing/distributed/circuit_breaker_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/datadog/tracing/distributed/circuit_breaker_spec.rb b/spec/datadog/tracing/distributed/circuit_breaker_spec.rb index a44bc422072..6a237fc712e 100644 --- a/spec/datadog/tracing/distributed/circuit_breaker_spec.rb +++ b/spec/datadog/tracing/distributed/circuit_breaker_spec.rb @@ -1,5 +1,3 @@ -require 'datadog/tracing/contrib/support/spec_helper' - require 'datadog' require 'datadog/tracing/distributed/circuit_breaker'