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

Move _dd.apm.enabled tag in tracing root span #4141

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
51585fc
Move `_dd.apm.enabled` tag in Tracing Rack Middleware instead of AppS…
vpellan Nov 21, 2024
7e1d748
Add SCA_STANDALONE scenario and force-execute SCA_STANDALONE tests
vpellan Nov 21, 2024
6433166
Changed c.appsec.standalone.enabled to c.tracing.apm.enabled
vpellan Nov 26, 2024
dd79a68
Reverse config while we are still using DD_EXPERIMENTAL_APPSEC_STANDA…
vpellan Nov 26, 2024
85e116f
Use TraceOperation method in should_skip_distributed_tracing
vpellan Nov 26, 2024
cc4be80
Add _dd.apm.enabled: 0 to root spans instead of rack spans when using…
vpellan Nov 27, 2024
cc72ea5
Renamed appsec_standalone_reject? to non_billing_reject?
vpellan Nov 27, 2024
c001f58
Lint
vpellan Nov 27, 2024
8e7f016
Fix should_skip_distributed_tracing with non-billing mode when no act…
vpellan Nov 28, 2024
e25c644
Un-factorized shoukd_skip_distributed_tracing for better code readabi…
vpellan Nov 29, 2024
bf8190b
fix should_skip_distributed_tracing? methods to use trace
vpellan Nov 29, 2024
f702fe9
Fix "should_skip_distributed_tracing?" in contrib
vpellan Dec 2, 2024
fc68c33
non-billing mode spec
vpellan Dec 2, 2024
3c39598
switch tests values for apm enabled
vpellan Dec 2, 2024
c7efe5c
Fix error on should_skip_distributed_tracing? on ethon instrumentation
vpellan Dec 2, 2024
3d54594
Fix gRPC non-billing mode test
vpellan Dec 2, 2024
7e8cf81
Fix gRPC specs
vpellan Dec 3, 2024
3142efb
Remove older should_skip_distributed_tracing?
vpellan Dec 3, 2024
22a9b40
Replace tracing_apm_enabled by non_billing_enabled in non_billing_rej…
vpellan Dec 10, 2024
fd7b84c
Replaced c.tracing.apm.enabled to c.tracing.non_billing.enabled
vpellan Dec 10, 2024
eeffb42
Lint
vpellan Dec 10, 2024
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
5 changes: 4 additions & 1 deletion .github/forced-tests-list.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
{

"SCA_STANDALONE":
[
"tests/appsec/test_asm_standalone.py::Test_SCAStandalone_Telemetry"
]
}
3 changes: 3 additions & 0 deletions .github/workflows/system-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ jobs:
- library: ruby
app: rails70
scenario: DEBUGGER_PII_REDACTION
- library: ruby
app: rails70
scenario: SCA_STANDALONE
- library: ruby
app: rack
scenario: SAMPLING
Expand Down
8 changes: 0 additions & 8 deletions lib/datadog/appsec/configuration/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -197,14 +197,6 @@ def self.add_settings!(base)
o.type :bool, nilable: true
o.env 'DD_APPSEC_SCA_ENABLED'
end

settings :standalone do
option :enabled do |o|
o.type :bool
o.env 'DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED'
o.default false
end
end
end
end
end
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 @@ -151,8 +151,6 @@ def add_appsec_tags(processor, scope)
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 @@ -7,7 +7,6 @@ module Ext
SCOPE_KEY = 'datadog.appsec.scope'

TAG_APPSEC_ENABLED = '_dd.appsec.enabled'
TAG_APM_ENABLED = '_dd.apm.enabled'
TAG_DISTRIBUTED_APPSEC_EVENT = '_dd.p.appsec'
end
end
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.

2 changes: 1 addition & 1 deletion lib/datadog/core/remote/transport/http.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def default_headers
headers[Datadog::Core::Transport::Ext::HTTP::HEADER_CONTAINER_ID] = container_id unless container_id.nil?
# Sending this header to the agent will disable metrics computation (and billing) on the agent side
# by pretending it has already been done on the library side.
if Datadog.configuration.appsec.standalone.enabled
if Datadog.configuration.tracing.non_billing.enabled
headers[Datadog::Core::Transport::Ext::HTTP::HEADER_CLIENT_COMPUTED_STATS] = 'yes'
end
end
Expand Down
9 changes: 4 additions & 5 deletions lib/datadog/tracing/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,11 @@ def build_sampler(settings)
return sampler
end

# AppSec events are sent to the backend using traces.
# Standalone ASM billing means that we don't want to charge clients for APM traces,
# 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,
# This sampler sends the minimum amount of traces possible,
# but to show the service on the service catalog, we need to send at least one trace per minute,
# to keep the service alive on the backend side.
if settings.appsec.standalone.enabled
# Traces can still be force-kept (e.g. those containing AppSec events).
if settings.tracing.non_billing.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),
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/tracing/configuration/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ module Ext
ENV_OTEL_TRACES_EXPORTER = 'OTEL_TRACES_EXPORTER'
ENV_HEADER_TAGS = 'DD_TRACE_HEADER_TAGS'
ENV_TRACE_ID_128_BIT_GENERATION_ENABLED = 'DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED'
ENV_APM_ENABLED = 'DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED' # Temporary until RFC decides final name
Copy link
Member

Choose a reason for hiding this comment

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

Could we highlight that "temporary" nature in the name? It's a big ask to name it ENV_APM_ENABLED, but actually check for standalone feature

P.S let's do it only if it makes sense (merged before RFC is in final state)


# @public_api
module SpanAttributeSchema
Expand Down
39 changes: 39 additions & 0 deletions lib/datadog/tracing/configuration/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,45 @@ def self.extended(base)
end
end

# Configures tracing non-billing mode configuration
#
# This rate-limit traces to 1 per minute, to keep the service alive without billing it.
# The RFC defines the environment variable as DD_TRACE_APM_ENABLED, we are following a similar name.
# As of late November, the RFC is not approved yet but should be soon, hence why we are still
# using DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED. DD_TRACE_APM_ENABLED will most likely change.
# As we already have a way to completely disable traces, we are using 'non_billing' terminology to
# make it clear that this is a non-billing mode, which does NOT completely disables tracing.
#
# Currently, the env var is DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED
# which enables non-billing mode when set to `true`. But the future env var will
# enables non-billing mode when set to `false`. so we will have to invert the values
# returned by the env_parser block.
#
# @public_api
settings :non_billing do
# When set to true (or env var set to false)
# sets the rate limit to 1 trace per minute, and prevents billing by adding tags
#
# @default `DD_TRACE_APM_ENABLED` environment variable, otherwise `true`
# @return [Boolean]
option :enabled do |o|
o.env Tracing::Configuration::Ext::ENV_APM_ENABLED
o.default false
o.type :bool
o.env_parser do |value|
value = value&.downcase
if ['false', '0'].include?(value)
false
elsif ['true', '1'].include?(value)
true
else
Datadog.logger.warn("Unsupported value for 'non-billing' mode: #{value}. Traces will be sent to Datadog.")
nil
end
end
end
end

# Comma-separated, case-insensitive list of header names that are reported in incoming and outgoing HTTP requests.
#
# Each header in the list can either be:
Expand Down
7 changes: 5 additions & 2 deletions lib/datadog/tracing/contrib/ethon/easy_patch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,14 @@ def datadog_before_request(continue_from: nil)

datadog_tag_request

if Datadog::AppSec::Utils::TraceOperation.appsec_standalone_reject?(datadog_trace)
if datadog_trace.non_billing_reject?
datadog_trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT
end

if datadog_configuration[:distributed_tracing]
unless Tracing::Distributed::Helpers.should_skip_distributed_tracing?(
datadog_configuration,
trace: datadog_trace
)
@datadog_original_headers ||= {}
Contrib::HTTP.inject(datadog_trace, @datadog_original_headers)
self.headers = @datadog_original_headers
Expand Down
7 changes: 4 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,11 @@ 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
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?
if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(@options, trace: trace)
propagate!(trace, span, datum)
end
propagate!(trace, span, datum) if distributed_tracing?

span
end
Expand Down
7 changes: 4 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,11 @@ 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
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?
if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(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
4 changes: 0 additions & 4 deletions lib/datadog/tracing/contrib/grpc/datadog_interceptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ def analytics_enabled?
Contrib::Analytics.enabled?(datadog_configuration[:analytics_enabled])
end

def distributed_tracing?
Datadog.configuration_for(self, :distributed_tracing) || datadog_configuration[:distributed_tracing]
end

def analytics_sample_rate
datadog_configuration[:analytics_sample_rate]
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,14 @@ 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?
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?

unless Tracing::Distributed::Helpers.should_skip_distributed_tracing?(
datadog_configuration,
client_config: Datadog.configuration_for(self)
)
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
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
14 changes: 9 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,15 @@ 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)
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?

app_config = Datadog.configuration.tracing[:http]
if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(
app_config,
client_config: client_config,
trace: trace
)
Contrib::HTTP.inject(trace, req)
end

Expand Down
20 changes: 9 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,15 @@ 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)
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?

app_config = Datadog.configuration.tracing[:httpclient]
if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(
app_config,
client_config: client_config,
trace: trace
)
Contrib::HTTP.inject(trace, req.header)
end

Expand Down Expand Up @@ -123,12 +127,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
20 changes: 10 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,18 @@ 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
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?

app_config = Datadog.configuration.tracing[:httprb]
if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(
app_config,
client_config: client_config,
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 +141,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
7 changes: 4 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,11 @@ 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
trace.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace.non_billing_reject?

unless Tracing::Distributed::Helpers.should_skip_distributed_tracing?(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]
trace_op.sampling_priority = Tracing::Sampling::Ext::Priority::AUTO_REJECT if trace_op.non_billing_reject?

if Tracing.enabled? &&
!Tracing::Distributed::Helpers.should_skip_distributed_tracing?(configuration, trace: trace_op)
Sidekiq.inject(trace_op, job)
end

span.resource = resource

Expand Down
11 changes: 11 additions & 0 deletions lib/datadog/tracing/distributed/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ def self.parse_hex_id(value)

num
end

def self.should_skip_distributed_tracing?(app_config, client_config: nil, trace: nil)
if ::Datadog.configuration.tracing.non_billing.enabled
return true unless trace
return true if trace.non_billing_reject?
end

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

!app_config[:distributed_tracing]
end
end
end
end
Expand Down
3 changes: 3 additions & 0 deletions lib/datadog/tracing/metadata/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ module Ext
# See Datadog-internal "RFC: Identifying which spans have profiling enabled " for details
TAG_PROFILING_ENABLED = '_dd.profiling.enabled'

# Set to '0' if 'non-billing' mode is enabled, not present otherwise.
TAG_APM_ENABLED = '_dd.apm.enabled'

# Defines constants for trace analytics
# @public_api
module Analytics
Expand Down
Loading
Loading