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 19 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
17 changes: 12 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,7 @@ 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 should_skip_distributed_tracing?(datadog_trace)
@datadog_original_headers ||= {}
Contrib::HTTP.inject(datadog_trace, @datadog_original_headers)
self.headers = @datadog_original_headers
Expand Down Expand Up @@ -219,6 +215,17 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I am seeing this piece of logic repeatedly several times, perhaps it make sense to encapsulate this from an AppSec module and be reused at multiple places?

The method can be stateless or depends on configuration and the contract is basically returning a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will investigate that, although I don't think it should be in an AppSec module, as in the V2, this will not contain any reference to AppSec (appsec.standalone will be replaced by tracing.apm and APPSEC_EVENT will be replaced by TRACE_SOURCE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 91f2f68

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
Expand Down
16 changes: 12 additions & 4 deletions lib/datadog/tracing/contrib/excon/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@ 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?
propagate!(trace, span, datum) unless should_skip_distributed_tracing?(trace)

span
end
Expand Down Expand Up @@ -192,6 +189,17 @@ 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
Expand Down
16 changes: 12 additions & 4 deletions lib/datadog/tracing/contrib/faraday/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ 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?
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
Expand Down Expand Up @@ -117,6 +114,17 @@ 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
Expand Down
13 changes: 12 additions & 1 deletion lib/datadog/tracing/contrib/grpc/datadog_interceptor/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down Expand Up @@ -109,6 +109,17 @@ 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
Expand Down
14 changes: 6 additions & 8 deletions lib/datadog/tracing/contrib/http/circuit_breaker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ def internal_request?(request)
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'
# 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)
Expand Down
6 changes: 1 addition & 5 deletions lib/datadog/tracing/contrib/http/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ 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? && !Contrib::HTTP.should_skip_distributed_tracing?(client_config, trace)
Contrib::HTTP.inject(trace, req)
end

Expand Down
15 changes: 9 additions & 6 deletions lib/datadog/tracing/contrib/httpclient/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +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 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? && !should_skip_distributed_tracing?(client_config, trace)
Contrib::HTTP.inject(trace, req.header)
end

Expand Down Expand Up @@ -123,7 +119,14 @@ 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 &&
(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]
Expand Down
15 changes: 10 additions & 5 deletions lib/datadog/tracing/contrib/httprb/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,10 @@ 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? && !should_skip_distributed_tracing?(client_config, 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,7 +133,14 @@ 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 &&
(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]
Expand Down
16 changes: 12 additions & 4 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,7 @@ 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]
Contrib::HTTP.inject(trace, processed_headers) unless should_skip_distributed_tracing?(trace)

super(&block)
end
Expand Down Expand Up @@ -124,6 +121,17 @@ 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
Expand Down
13 changes: 12 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,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

Expand Down Expand Up @@ -55,6 +55,17 @@ 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
Expand Down
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
Loading
Loading