Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix APM Disablement V1 (ASM Standalone) #4479

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4046576
Fix apm disablement tag when appsec is disabled
vpellan Feb 24, 2025
8ed08f3
Add method to skip upstream sampling priority
vpellan Mar 5, 2025
ac8c014
Add APM disablement integration tests
vpellan Mar 5, 2025
411d428
Fix propagate_sampling_priority with no upstream_tags
vpellan Mar 6, 2025
5d55d8a
Fix asm standalone integration tests
vpellan Mar 6, 2025
e491c6f
Remove appsec_standalone_reject method
vpellan Mar 6, 2025
0b261d2
Skip distributed tracing if APM is disabled on all HTTP/GRPC/Sidekiq …
vpellan Mar 7, 2025
f8acd8f
Delete unnecessary default sample rate (as we match all traces agains…
vpellan Mar 7, 2025
360761a
Add test to check heartbeat traces
vpellan Mar 7, 2025
09646d1
Fix Datadog-Computed-Client-Stats tests
vpellan Mar 7, 2025
6e82099
Simplify Datadog-Client-Computed-Stats test
vpellan Mar 7, 2025
5c73ff8
Add separate method to generate APM disablement post_sampler
vpellan Mar 10, 2025
0389f6b
Cleaned appsec standalone tests for rack
vpellan Mar 10, 2025
3ec6ff9
Delete ASM Standalone tests on Rails and Sinatra (ASM Standalone has …
vpellan Mar 10, 2025
6c863cd
Remove forced-tests from git conflict resolution
vpellan Mar 10, 2025
ce8a363
Rename build_above_second_post_sampler to build_rate_limit_post_sampler
vpellan Mar 11, 2025
3df2d61
Move before block in context
vpellan Mar 11, 2025
c2ff1b6
Rubocop
vpellan Mar 11, 2025
a9473aa
Changed system-tests scenarios to Experimental mode
vpellan Mar 11, 2025
91f2f68
Add should_skip_distributed_tracing? method
vpellan Mar 12, 2025
def4da4
Fix circuit breaker spec
vpellan Mar 12, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .github/forced-tests-list.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
{

}
"SCA_STANDALONE_EXPERIMENTAL": [
"tests/appsec/test_asm_standalone.py::Test_SCAStandalone_Telemetry"
]
}
5 changes: 4 additions & 1 deletion .github/workflows/system-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ jobs:
- APPSEC_DISABLED
- APPSEC_BLOCKING_FULL_DENYLIST
- APPSEC_REQUEST_BLOCKING
- APPSEC_STANDALONE
- APPSEC_STANDALONE_EXPERIMENTAL
- APPSEC_BLOCKING
include:
- library: ruby
Expand Down Expand Up @@ -320,6 +320,9 @@ jobs:
- library: ruby
app: rails70
scenario: DEBUGGER_PII_REDACTION
- library: ruby
app: rails70
scenario: SCA_STANDALONE_EXPERIMENTAL
- library: ruby
app: rack
scenario: SAMPLING
Expand Down
2 changes: 0 additions & 2 deletions lib/datadog/appsec/contrib/rack/request_middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 0 additions & 1 deletion lib/datadog/appsec/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
2 changes: 0 additions & 2 deletions lib/datadog/appsec/utils.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
# frozen_string_literal: true

require_relative 'utils/trace_operation'

module Datadog
module AppSec
# Utilities for AppSec
Expand Down
15 changes: 0 additions & 15 deletions lib/datadog/appsec/utils/trace_operation.rb

This file was deleted.

15 changes: 8 additions & 7 deletions lib/datadog/tracing/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +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.
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
)
end
post_sampler = build_rate_limit_post_sampler(seconds: 60) if settings.appsec.standalone.enabled

# Sampling rules are provided
if (rules = settings.tracing.sampling.rules)
Expand Down Expand Up @@ -197,6 +191,13 @@ def build_tracer_tags(settings)
end
end

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
)
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)
Expand Down
9 changes: 4 additions & 5 deletions lib/datadog/tracing/contrib/ethon/easy_patch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,10 @@ 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]
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
Expand Down
8 changes: 5 additions & 3 deletions lib/datadog/tracing/contrib/excon/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ 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
if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?(
datadog_config: @options,
trace: trace
)
propagate!(trace, span, datum)
end
propagate!(trace, span, datum) if distributed_tracing?

span
end
Expand Down
8 changes: 5 additions & 3 deletions lib/datadog/tracing/contrib/faraday/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@ 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
if Tracing.enabled? && !Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?(
datadog_config: request_options,
trace: trace
)
propagate!(trace, span, env)
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
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) if distributed_tracing?
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}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
15 changes: 0 additions & 15 deletions lib/datadog/tracing/contrib/http/circuit_breaker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +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

def should_skip_distributed_tracing?(client_config)
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'
end

return !client_config[:distributed_tracing] if client_config && client_config.key?(:distributed_tracing)

!Datadog.configuration.tracing[:http][:distributed_tracing]
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
10 changes: 5 additions & 5 deletions lib/datadog/tracing/contrib/http/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ 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)
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

Expand Down
16 changes: 5 additions & 11 deletions lib/datadog/tracing/contrib/httpclient/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +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 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)
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

Expand Down Expand Up @@ -123,12 +123,6 @@ def analytics_enabled?(request_options)
Contrib::Analytics.enabled?(request_options[:analytics_enabled])
end

def should_skip_distributed_tracing?(client_config)
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)

Expand Down
16 changes: 6 additions & 10 deletions lib/datadog/tracing/contrib/httprb/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ 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
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

Contrib::HTTP.inject(trace, req) if Tracing.enabled? && !should_skip_distributed_tracing?(client_config)

# Add additional request specific tags to the span.
annotate_span_with_request!(span, req, request_options)
rescue StandardError => e
Expand Down Expand Up @@ -135,12 +137,6 @@ def logger
Datadog.logger
end

def should_skip_distributed_tracing?(client_config)
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)

Expand Down
8 changes: 5 additions & 3 deletions lib/datadog/tracing/contrib/rest_client/request_patch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ 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
unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?(
datadog_config: datadog_configuration,
trace: trace
)
Contrib::HTTP.inject(trace, processed_headers)
end
Contrib::HTTP.inject(trace, processed_headers) if datadog_configuration[:distributed_tracing]

super(&block)
end
Expand Down
7 changes: 6 additions & 1 deletion lib/datadog/tracing/contrib/sidekiq/client_tracer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) if configuration[:distributed_tracing]
unless Tracing::Distributed::CircuitBreaker.should_skip_distributed_tracing?(
datadog_config: configuration,
trace: trace_op
)
Sidekiq.inject(trace_op, job)
end

span.resource = resource

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
26 changes: 26 additions & 0 deletions lib/datadog/tracing/distributed/circuit_breaker.rb
Original file line number Diff line number Diff line change
@@ -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 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the method takes configuration as arguments, why is this line reading global config?

The method would be much more understandable if the types of parameters were listed.

(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
2 changes: 2 additions & 0 deletions lib/datadog/tracing/metadata/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion lib/datadog/tracing/trace_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand Down
Loading
Loading