ASM Standalone Billing #3965

vpellan · 2024-10-01T11:26:26Z

What does this PR do?

This PR add the DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED feature flag to enable ASM standalone billing (that means disable APM by limiting the number of traces as much as possible, including distributed tracing)

ASM uses traces to send AppSec events/data to the agent. Standalone ASM billing means that we don't want to charge clients for APM traces, but we still want AppSec data (which again, relies on tracing), so we want to send the minimum amount of traces possible (ideally only traces that contains security events and their downstream services if distributed tracing is enabled), 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.

ASM Standalone billing can be resumed that way:

We limit the number of traces sent to the agent to 1 per minute (so the service is still considered alive on the backend side) and all the other traces are dropped
If an appsec event is detected in a service, or in an upstream service through distributed tracing, we force keep the trace
We tell the agent that we've already computed metrics, so the agent don't and it effectively disables metrics billing

How to test the change?

There are system-tests specific to APPSEC_STANDALONE for each weblog (Rack, Rails 4.2-7.1, Sinatra 1.4-4.0)

Integration specs for rack, rails and sinatra and unit specs have been also been added. They are executed by the CI.

APPSEC-54014

pr-commenter · 2024-10-01T11:50:28Z

Benchmarks

Benchmark execution time: 2024-10-21 14:45:02

Comparing candidate commit 8e54f7d in PR branch vpellan/standalone-asm with baseline commit 60febf8 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 2 unstable metrics.

codecov-commenter · 2024-10-01T14:33:02Z

Codecov Report

Attention: Patch coverage is 98.75776% with 6 lines in your changes missing coverage. Please review.

Project coverage is 97.87%. Comparing base (60febf8) to head (8e54f7d).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/datadog/tracing/contrib/ethon/easy_patch.rb	50.00%	1 Missing ⚠️
lib/datadog/tracing/contrib/excon/middleware.rb	50.00%	1 Missing ⚠️
lib/datadog/tracing/contrib/faraday/middleware.rb	50.00%	1 Missing ⚠️
...adog/tracing/contrib/httpclient/instrumentation.rb	50.00%	1 Missing ⚠️
.../datadog/tracing/contrib/httprb/instrumentation.rb	50.00%	1 Missing ⚠️
...tadog/tracing/contrib/rest_client/request_patch.rb	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
- Coverage   97.87%   97.87%   -0.01%     
==========================================
  Files        1314     1317       +3     
  Lines       78635    79072     +437     
  Branches     3895     3923      +28     
==========================================
+ Hits        76963    77389     +426     
- Misses       1672     1683      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

y9v · 2024-10-02T09:23:02Z

lib/datadog/tracing/sampling/rule_sampler.rb

- @rate_limiter = if rate_limiter
+ @rate_limiter = if Datadog.configuration.appsec.standalone.enabled
+ # 0.0167 ~ 1 trace per minute
+ Core::TokenBucket.new(0.0167, 1.0)


for readability I would propose to use 1/60 instead of 0.0167 here

Strech

Great work, I sprinkled a bit of suggestions here and there in attempt to improve this special-case code

Strech · 2024-10-02T15:12:53Z

lib/datadog/appsec/contrib/rack/request_middleware.rb

+ span.set_tag(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled
+ span.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1)


I would suggest to keep the order from required to optional, like this

Suggested change

span.set_tag(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled

span.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1)

span.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1)

span.set_tag(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled

Strech · 2024-10-02T15:56:58Z

lib/datadog/appsec/event.rb

+ if scope.trace
+ scope.trace.keep!
+ scope.trace.set_tag(
+ Datadog::Tracing::Metadata::Ext::Distributed::TAG_DECISION_MAKER,
+ Datadog::Tracing::Sampling::Ext::Decision::ASM
+ )
+ scope.trace.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT, '1')
+ end


We can reduce nesting with a guard-clause here like this:

Suggested change

if scope.trace

scope.trace.keep!

scope.trace.set_tag(

Datadog::Tracing::Metadata::Ext::Distributed::TAG_DECISION_MAKER,

Datadog::Tracing::Sampling::Ext::Decision::ASM

)

scope.trace.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT, '1')

end

return unless scope.trace

scope.trace.keep!

scope.trace.set_tag(

Datadog::Tracing::Metadata::Ext::Distributed::TAG_DECISION_MAKER,

Datadog::Tracing::Sampling::Ext::Decision::ASM

)

scope.trace.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT, '1')

Strech · 2024-10-02T16:00:22Z

lib/datadog/tracing/contrib/http/circuit_breaker.rb

+
+ # return true if absence of upstream ASM event (_dd.p.appsec:1)
+ # and no local security event (which sets _dd.p.appsec:1 locally)
+ return true if active_trace.nil? || active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'


Will it be possible to cut 1 namespace? (not sure)

Suggested change

return true if active_trace.nil? || active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'

return true if active_trace.nil? || active_trace.get_tag(AppSec::Ext::TAG_APPSEC_EVENT) != '1'

It does work but it seems that there's a convention in the codebase to still add the parent namespace (I suppose for readability). You can see another example at l.27-28 of the same file.

Strech · 2024-10-02T16:03:16Z

spec/datadog/appsec/configuration/settings_spec.rb

+ [true, false].each do |value|
+ context "when given #{value}" do
+ let(:appsec_standalone_enabled) { value }
+
+ before { set_appsec_standalone_enabled }
+
+ it { expect(settings.appsec.standalone.enabled).to eq(value) }
+ end
+ end


It might be personal opinion, but I think DRY in tests makes them harder to read. WDYT?

I agree for more complex cases, but I think that boolean are simple enough that it does not lower the readability too much. [true, false].each is used by APM team (e.g. in spec/datadog/core/configuration/settings_spec.rb) so I think it's better to use it for cohesion.

To be honest, this spec is such a massive copy-paste with these boolean checks that I kinda wish we had a behaves_like_a_binary_setting or something like that helper rather than repeating it every time.

Not to say we need to fix it in this PR, but yeah, it's been bugging me for a while and it seemed like a good time to share ;)

Yeah, shared behavior sounds good. Agree that maybe not right now, but it's good to share the pain 😉

what are we actually testing here? If we want to test that the value is being assigned, it is enough to test once with one value. If we want to test that this setting could be only set to a boolean value, we also need a test case when a non-boolean value is being passed to this setting.

We are testing that the value is being assigned. I agree that these tests should be reworked (on another PR)

Strech · 2024-10-02T16:03:53Z

spec/datadog/appsec/contrib/support/integration/shared_examples.rb

+ let(:appsec_enabled) { true }
+ it do
+ expect(service_span.send(:metrics)['_dd.apm.enabled']).to eq(0)
+ expect(service_span.send(:metrics)['_dd.appsec.enabled']).to eq(1.0)
+ end


Suggested change

let(:appsec_enabled) { true }

it do

expect(service_span.send(:metrics)['_dd.apm.enabled']).to eq(0)

expect(service_span.send(:metrics)['_dd.appsec.enabled']).to eq(1.0)

end

let(:appsec_enabled) { true }

it do

expect(service_span.send(:metrics)['_dd.apm.enabled']).to eq(0)

expect(service_span.send(:metrics)['_dd.appsec.enabled']).to eq(1.0)

end

Strech · 2024-10-02T16:04:05Z

spec/datadog/appsec/contrib/support/integration/shared_examples.rb

+ let(:appsec_enabled) { false }
+ it do
+ expect(service_span.send(:metrics)['_dd.apm.enabled']).to be_nil
+ expect(service_span.send(:metrics)['_dd.appsec.enabled']).to be_nil
+ end


Suggested change

let(:appsec_enabled) { false }

it do

expect(service_span.send(:metrics)['_dd.apm.enabled']).to be_nil

expect(service_span.send(:metrics)['_dd.appsec.enabled']).to be_nil

end

let(:appsec_enabled) { false }

it do

expect(service_span.send(:metrics)['_dd.apm.enabled']).to be_nil

expect(service_span.send(:metrics)['_dd.appsec.enabled']).to be_nil

end

Strech · 2024-10-02T16:08:36Z

lib/datadog/tracing/contrib/http/circuit_breaker.rb

+
+ # return true if absence of upstream ASM event (_dd.p.appsec:1)
+ # and no local security event (which sets _dd.p.appsec:1 locally)
+ return true if active_trace.nil? || active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'


Since this is a very-special case and we put some comments, we also might simplify reading with a good naming (not saying this one is good, but would like to show an example)

Suggested change

return true if active_trace.nil? || active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'

absent_trace_or_no_local_appsec_event = active_trace.nil? || active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'

return true if absent_trace_or_no_local_appsec_event

Strech · 2024-10-02T16:09:09Z

lib/datadog/tracing/contrib/http/instrumentation.rb

@@ -35,6 +35,11 @@ def request(req, body = nil, &block)
 span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND
 span.resource = req.method

+ if Datadog.configuration.appsec.standalone.enabled &&
+ (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1')


WDYT about the same approach as ⬆️ ?

spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb

datadog-datadog-prod-us1 · 2024-10-03T15:47:56Z

spec/datadog/appsec/contrib/rack/integration_test_spec.rb

+ map '/requestdownstream' do
+ run(
+ proc do |_env|
+ uri = URI('http://localhost:3000/returnheaders')


🔵 Code Vulnerability

Do not use cleartext protocol http:// (...read more)

This rule is designed to prevent the use of the insecure HTTP protocol in your Ruby code. The HTTP protocol does not encrypt the data that is sent between the client and the server, which can lead to sensitive information being intercepted by malicious parties. This is particularly risky when dealing with sensitive data such as API keys, user credentials, or personal user information.

The importance of this rule lies in the security and integrity of your application. By using an unsecured protocol like HTTP, you expose your application and its users to potential data breaches. A breach can lead to loss of trust, legal liability, and significant remediation costs.

To avoid violating this rule, always use the HTTPS protocol when making network requests. HTTPS encrypts the data sent between the client and server, protecting it from interception. By using libraries like Faraday, HTTPX, HTTParty, RestClient, or Ruby's built-in Net::HTTP, you can specify HTTPS by simply replacing 'http://' with 'https://'. For example, instead of HTTP.get("http://example.org"), use HTTP.get("https://example.org"). Always ensure that any third-party services your application interacts with support HTTPS.

spec/datadog/appsec/contrib/rails/integration_test_spec.rb

y9v · 2024-10-07T13:58:40Z

.github/workflows/system-tests.yml

- FORCE_TESTS: -F tests/appsec/waf/test_addresses.py::Test_GraphQL -F tests/appsec/test_blocking_addresses.py::Test_BlockingGraphqlResolvers
- FORCE_TESTS_SCENARIO: GRAPHQL_APPSEC


did we forgot to remove this when merging GraphQL changes? If so, would it be better to create a separate PR with these changes?

This line must be cleaned after the release of the specified version in the manifest. In the case of GraphQL Appsec, it should have been cleaned after 2.3.0 release, but it does not change anything after the release, as we are forcing the execution of a test that is already executed. Charles specified in the doc that "from time to time" you should removes all the -F from the CI (https://github.com/DataDog/system-tests/blob/main/docs/edit/egg-chicken-changes.md?plain=1#L34), so this is why I did not considered it a priority.
I guess that a clean way to do it would be to add a step in fast_castle.
We can do a separate PR but the way FORCE_TESTS_SCENARIO is implemented does not enable to force tests execution on multiple scenarios, so that would mean that I cannot force execute APPSEC_STANDALONE on this PR

y9v · 2024-10-07T14:04:06Z

lib/datadog/appsec/contrib/rack/request_middleware.rb

@@ -150,7 +150,8 @@ def add_appsec_tags(processor, scope)

 return unless trace && span

- span.set_tag('_dd.appsec.enabled', 1)
+ span.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1)
+ span.set_tag(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled


not sure, but does this need a comment?

I'm not sure either what could be added to give more info that what the code already gives

I would add somethink like "we want to make sure the customer will not get billed for APM when standalone ASM is enabled". But again, it's probably fine to leave it as it is

I read PR description but did not understand that this is what the PR is trying to achieve. So probably would be helpful to include this bit somewhere.

I added a comment, thanks!

y9v · 2024-10-07T14:05:28Z

lib/datadog/appsec/event.rb

@@ -137,6 +137,24 @@ def build_service_entry_tags(event_group)
 end
 # rubocop:enable Metrics/MethodLength

+ def add_event_tags(scope, waf_result)


Since the class is already named Event, should we rename this function to add_tags?

y9v · 2024-10-07T14:06:16Z

lib/datadog/appsec/event.rb

+ # Propagate to downstream services the information that the current distributed trace is
+ # containing at least one ASM security event


I would put this comment before scope.trace.keep!, it's more relevant there

y9v · 2024-10-07T14:08:04Z

lib/datadog/appsec/event.rb

+ scope.trace.keep!
+ scope.trace.set_tag(
+ Datadog::Tracing::Metadata::Ext::Distributed::TAG_DECISION_MAKER,
+ Datadog::Tracing::Sampling::Ext::Decision::ASM
+ )
+ scope.trace.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT, '1')


Do we want to execute this code only if the standalone appsec is enabled?

and I would actually move this code to another function

Do we want to execute this code only if the standalone appsec is enabled?

We want to execute this code each time there is an Appsec event, it is not specific to Appsec standalone

I wrote this method to factorise watch_xxx methods in each integration's Gateway Watcher so that it would comply to our Rubocop spec.

and I would actually move this code to another function

Would you move it to another method because it is acting on scope.trace and not scope.service_entry_span anymore ?

yes, I think it does a bit more then setting tags - if I understood correctly it's more about making sure that the trace is preserved?

You are right, scope.trace.keep! also sets sampled instance variable to true. I'll put it back in the gateway watchers and make a separate method for propagated tags.

y9v · 2024-10-07T14:13:51Z

lib/datadog/core/remote/transport/http.rb

@@ -120,6 +120,10 @@ def default_headers
 # Add container ID, if present.
 container_id = Datadog::Core::Environment::Container.container_id
 headers[Datadog::Core::Transport::Ext::HTTP::HEADER_CONTAINER_ID] = container_id unless container_id.nil?
+ # Pretend that stats computation are already done by the client


I don't have much background here, but I think it would be helpful to extend this comment and add an explanation on what this header is used for, and why do we set it to yes manually.

Thanks, I added more context in the comment

y9v · 2024-10-07T14:18:03Z

lib/datadog/tracing/contrib/http/circuit_breaker.rb

+ active_trace = Tracing.active_trace
+
+ # return true if absence of upstream ASM event (_dd.p.appsec:1)
+ # and no local security event (which sets _dd.p.appsec:1 locally)
+ no_upstream_or_local_event = active_trace.nil? ||
+ active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'
+ return true if no_upstream_or_local_event


how about this?

return true unless Tracing.active_trace Tracing.active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'

I did something similar (that would also satisfy Rubocop) thanks !

y9v · 2024-10-07T14:18:53Z

lib/datadog/tracing/contrib/http/circuit_breaker.rb

+ # return true if absence of upstream ASM event (_dd.p.appsec:1)
+ # and no local security event (which sets _dd.p.appsec:1 locally)


again, I think it makes sense to explain here in which case _dd.p.appsec is being set to 1 and why we want to skip distributed tracing in such case

I also added more context to this one

y9v · 2024-10-07T14:20:35Z

lib/datadog/tracing/contrib/http/instrumentation.rb

@@ -35,6 +35,13 @@ def request(req, body = nil, &block)
 span.type = Tracing::Metadata::Ext::HTTP::TYPE_OUTBOUND
 span.resource = req.method

+ asm_standalone_no_upstream_or_local_event = Datadog.configuration.appsec.standalone.enabled &&
+ (trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1')


should this be a method on Datadog::Trace?

I added it as a class method to Tracing::TraceOperation, thanks!

y9v · 2024-10-07T14:22:30Z

lib/datadog/tracing/sampling/rule_sampler.rb

-
- @default_sampler = if default_sampler
+ @default_sampler = if Datadog.configuration.appsec.standalone.enabled
+ @rules = [SimpleRule.new(sample_rate: 1.0)]


I think it's quite confusing that we are setting @rules when assigning @default_sampler value. Should we move this out from here?

I agree, I separated rules and default_sampler initializers, thanks!

spec/datadog/appsec/contrib/rack/integration_test_spec.rb

spec/datadog/appsec/contrib/support/integration/shared_examples.rb

p-datadog · 2024-10-08T17:26:10Z

lib/datadog/tracing/sampling/rule_sampler.rb

@@ -29,21 +29,29 @@ def initialize(
 default_sample_rate: Datadog.configuration.tracing.sampling.default_rate,
 default_sampler: nil
 )


I think this section of the code will benefit from a comment explaining appsec & tracing interaction. Especially for someone who is new to both appsec and tracing.

Thanks, I added a comment to explain it a bit more, let me know if you think that it is not comprehensive enough !

lib/datadog/appsec/contrib/rack/request_middleware.rb

lib/datadog/tracing/contrib/http/instrumentation.rb

marcotc · 2024-10-16T19:05:13Z

lib/datadog/tracing/sampling/rule_sampler.rb

+ @rules = if Datadog.configuration.appsec.standalone.enabled
+ [SimpleRule.new(sample_rate: 1.0)]
+ elsif default_sample_rate && !default_sampler
+ # Add to the end of the rule list a rule always matches any trace
+ rules << SimpleRule.new(sample_rate: default_sample_rate)
+ else
+ rules
+ end
+ @rate_limiter = if Datadog.configuration.appsec.standalone.enabled
+ # 1 trace per minute
+ Core::TokenBucket.new(1.0 / 60, 1.0)
+ elsif rate_limiter
 rate_limiter
 elsif rate_limit
 Core::TokenBucket.new(rate_limit)
 else
 Core::UnlimitedLimiter.new
 end
-
- @default_sampler = if default_sampler
- default_sampler
- elsif default_sample_rate
- # Add to the end of the rule list a rule always matches any trace
- @rules << SimpleRule.new(sample_rate: default_sample_rate)
+ @default_sampler = if Datadog.configuration.appsec.standalone.enabled ||
+ (default_sample_rate && !default_sampler)


I suggest moving this logic to the initialization of the RuleSampler, instead of inside of it:

dd-trace-rb/lib/datadog/tracing/component.rb

Lines 70 to 96 in d3d7fde

def build_sampler(settings)

# A custom sampler is provided

if (sampler = settings.tracing.sampler)

return sampler

end

# Sampling rules are provided

if (rules = settings.tracing.sampling.rules)

post_sampler = Tracing::Sampling::RuleSampler.parse(

rules,

settings.tracing.sampling.rate_limit,

settings.tracing.sampling.default_rate

)

end

# The default sampler.

# Used if no custom sampler is provided, or if sampling rule parsing fails.

post_sampler ||= Tracing::Sampling::RuleSampler.new(

rate_limit: settings.tracing.sampling.rate_limit,

default_sample_rate: settings.tracing.sampling.default_rate

)

Tracing::Sampling::PrioritySampler.new(

base_sampler: Tracing::Sampling::AllSampler.new,

post_sampler: post_sampler

)

end

It looks like this logic can manipulate the parameters passed to RuleSampler to accomplish the same goal, I think it would be: RuleSampler.new([], rate_limiter: Core::TokenBucket.new(1.0 / 60, 1.0), default_sample_rate: 1.0).

It is also initialised in here:

dd-trace-rb/lib/datadog/tracing/tracer.rb

Lines 50 to 62 in 60febf8

def initialize(

trace_flush: Flush::Finished.new,

context_provider: DefaultContextProvider.new,

default_service: Core::Environment::Ext::FALLBACK_SERVICE_NAME,

enabled: true,

sampler: Sampling::PrioritySampler.new(

base_sampler: Sampling::AllSampler.new,

post_sampler: Sampling::RuleSampler.new

),

span_sampler: Sampling::Span::Sampler.new,

tags: {},

writer: Writer.new

)

Do you think I should also move the logic there ? Imo it would mean duplicated code

You can ignore this one in dd-trace-rb/lib/datadog/tracing/tracer.rb. It's not used anymore, just a relic from when the Tracer class itself was public. You can always assume well go through the Component initialization.

lib/datadog/appsec/event.rb

lib/datadog/appsec/contrib/graphql/gateway/watcher.rb

lib/datadog/tracing/trace_operation.rb

Strech

I don't have much to add to existing suggestions by others, great job 👏🏼

Strech · 2024-10-17T08:10:01Z

sig/datadog/appsec/event.rbs

 private

 def self.compressed_and_base64_encoded: (untyped value) -> untyped

 def self.json_parse: (untyped value) -> untyped

 def self.gzip: (untyped value) -> untyped
+
+ def self.add_distributed_tags: (untyped trace) -> untyped


May I ask to type it? (Not sure about trace type, but I think it could be defined too.

Suggested change

def self.add_distributed_tags: (untyped trace) -> untyped

def self.add_distributed_tags: (untyped trace) -> void

Fixed in 22f18a8

Strech · 2024-10-17T08:10:51Z

sig/datadog/appsec/event.rbs

@@ -13,14 +13,18 @@ module Datadog
 def self.record_via_span: (Datadog::Tracing::SpanOperation, *untyped events) -> untyped

 def self.build_service_entry_tags: (Array[Hash[::Symbol, untyped]] event_group) -> Hash[::String, untyped]
-
+
+ def self.add_tags: (Datadog::AppSec::Scope scope, Datadog::AppSec::WAF::Result waf_result) -> untyped


Suggested change

def self.add_tags: (Datadog::AppSec::Scope scope, Datadog::AppSec::WAF::Result waf_result) -> untyped

def self.add_tags: (Datadog::AppSec::Scope scope, Datadog::AppSec::WAF::Result waf_result) -> void

Fixed in 22f18a8

anmarchenko · 2024-10-17T09:32:20Z

lib/datadog/tracing/contrib/http/circuit_breaker.rb

@@ -29,6 +29,15 @@ def internal_request?(request)
 end

 def should_skip_distributed_tracing?(client_config)
+ if Datadog.configuration.appsec.standalone.enabled


I see that we add a bunch of Datadog.configuration.appsec.standalone.enabled all over the tracing component which imposes a hard dependency between Tracing and AppSec internal details. What's especially worrying, is that in this case we reverse the dependency and now Tracing knows about AppSec internals (even though AppSec is supposed to be a client of Tracing).

So my question is for the Tracing team: do you think it would make sense to introduce some tracing settings to allow client products to tweak tracing behaviour even more? Like we do between CI and Tracing modules: Tracing exposes settings, CI configures them.

@marcotc what do you think?

To add more context: appsec standalone is still experimental and the env var/configuration might change name in the future, and the 'appsec' part of this setting name may disappear. What it do on the tracing part is disabling distributed tracing, adding a header to the payload sent to the agent, creating a 1 trace per minute rate limiter, and setting the sampling priority to 'reject' if there is an appsec event detected. Although the first 3 points aren't directly tied to appsec, the last one is, but it is still acting only on the traces.

…or now)

…d Sinatra integration test spec

…t-Computed-Stats header and _sampling_priority_v1 tag - Reproduced system-tests ASM Standalone tests

…tp_spec

…g and spec

…d_tags

…bled metrics

Set sampling priority to AUTO_REJECT when ASM Standalone is active and no appsec event everywhere there is HTTP.inject

… trace.keep in it

marcotc

Looks good!

…mponent.build_sampler

y9v

👍

vpellan force-pushed the vpellan/standalone-asm branch from b3aceb5 to 02b93e4 Compare October 1, 2024 15:41

Base automatically changed from vpellan/add-weblogs-system-tests to master October 2, 2024 08:55

y9v reviewed Oct 2, 2024

View reviewed changes

Strech reviewed Oct 2, 2024

View reviewed changes

vpellan force-pushed the vpellan/standalone-asm branch from 02b93e4 to 632eb8d Compare October 3, 2024 15:46

datadog-datadog-prod-us1 bot reviewed Oct 3, 2024

View reviewed changes

y9v requested changes Oct 7, 2024

View reviewed changes

datadog-datadog-prod-us1 bot reviewed Oct 7, 2024

View reviewed changes

spec/datadog/appsec/contrib/rack/integration_test_spec.rb Show resolved Hide resolved

spec/datadog/appsec/contrib/support/integration/shared_examples.rb Outdated Show resolved Hide resolved

p-datadog reviewed Oct 8, 2024

View reviewed changes

vpellan force-pushed the vpellan/standalone-asm branch from a1eb0bc to ad75a22 Compare October 10, 2024 15:04

vpellan marked this pull request as ready for review October 10, 2024 15:53

vpellan requested review from a team as code owners October 10, 2024 15:53

marcotc reviewed Oct 16, 2024

View reviewed changes

lib/datadog/appsec/contrib/rack/request_middleware.rb Outdated Show resolved Hide resolved

marcotc reviewed Oct 16, 2024

View reviewed changes

lib/datadog/tracing/contrib/http/instrumentation.rb Show resolved Hide resolved

marcotc reviewed Oct 16, 2024

View reviewed changes

lib/datadog/appsec/event.rb Show resolved Hide resolved

marcotc reviewed Oct 16, 2024

View reviewed changes

lib/datadog/appsec/contrib/graphql/gateway/watcher.rb Outdated Show resolved Hide resolved

marcotc reviewed Oct 16, 2024

View reviewed changes

lib/datadog/tracing/trace_operation.rb Outdated Show resolved Hide resolved

Strech approved these changes Oct 17, 2024

View reviewed changes

anmarchenko reviewed Oct 17, 2024

View reviewed changes

vpellan added 5 commits October 17, 2024 17:36

Add DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED env var (does nothing f…

03106a2

…or now)

Add _dd.apm.enabled tag

000bf66

Add APM rate limiting when ASM standalone is activated

fa4540d

Add Datadog-Client-Computed-Stats header with ASM standalone

fbfd4af

Moved hardcoded _dd.appsec/apm.enabled tags to Ext file

a617a2c

vpellan added 19 commits October 17, 2024 17:36

Apply PR review suggestions

3c4de1a

Add /requestdownstream and /returnheaders endpoints to Rack, Rails an…

f362662

…d Sinatra integration test spec

- Added mocked agent and force trace formatting to test Datadog-Clien…

a7c3eed

…t-Computed-Stats header and _sampling_priority_v1 tag - Reproduced system-tests ASM Standalone tests

Applied code review suggestions

ee6dc53

Renamed TAG_APPSEC_EVENT to TAG_DISTRIBUTED_APPSEC_EVENT

72a1027

Add comment for appsec _dd.apm.enabled=0 tag

efcf8dd

Separated tagging and force_keep of traces in gateway watchers

9acdafb

Add comment to explain ASM standalone rate limiting

452e7b4

Add Datadog::AppSec::Event.add_tags spec and sig

3ffaddd

Add Datadog-Client-Computed-Stats test in Core::Remote::Transport::ht…

97d3eba

…tp_spec

Added circuit_breaker 'should_skip_distributed_tracing' spec

63c1b87

Added rule_sampler spec tests when appsec standalone is activated

8575ed2

Rename asm_standalone_reject? to appsec_standalone_reject? and add si…

154f31d

…g and spec

Fix typo in AppSec::Event.add_tags spec

56a6475

Update Unreleased Changelog

5f9a239

Add correct sig to Datadog::AppSec::Event.add_tags and add_distribute…

22f18a8

…d_tags

Replaced set_tag by set_metric for _dd.appsec.enabled and _dd.apm.ena…

4a2bf67

…bled metrics

Move appsec_standalone_reject? to AppSec namespace

e24310e

Set sampling priority to AUTO_REJECT when ASM Standalone is active and no appsec event everywhere there is HTTP.inject

Rename AppSec::Event.add_tags to AppSec::Event.tag_and_keep! and move…

73f8af2

… trace.keep in it

vpellan force-pushed the vpellan/standalone-asm branch from ad75a22 to 73f8af2 Compare October 17, 2024 15:53

marcotc approved these changes Oct 18, 2024

View reviewed changes

Changed RuleSampler initialization with ASM Standalone to Tracing::Co…

a2f973b

…mponent.build_sampler

vpellan force-pushed the vpellan/standalone-asm branch from e5f552f to a2f973b Compare October 21, 2024 13:46

y9v approved these changes Oct 21, 2024

View reviewed changes

revert system-tests branch to main

8e54f7d

vpellan enabled auto-merge October 21, 2024 14:07

vpellan disabled auto-merge October 21, 2024 14:10

vpellan merged commit df97f48 into master Oct 21, 2024
266 checks passed

vpellan deleted the vpellan/standalone-asm branch October 21, 2024 14:47

github-actions bot added this to the 2.5.0 milestone Oct 21, 2024

		span.set_tag(Datadog::AppSec::Ext::TAG_APM_ENABLED, 0) if Datadog.configuration.appsec.standalone.enabled
		span.set_tag(Datadog::AppSec::Ext::TAG_APPSEC_ENABLED, 1)

	return true if active_trace.nil? \|\| active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'
	return true if active_trace.nil? \|\| active_trace.get_tag(AppSec::Ext::TAG_APPSEC_EVENT) != '1'

	return true if active_trace.nil? \|\| active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'
	absent_trace_or_no_local_appsec_event = active_trace.nil? \|\| active_trace.get_tag(Datadog::AppSec::Ext::TAG_APPSEC_EVENT) != '1'
	return true if absent_trace_or_no_local_appsec_event

		FORCE_TESTS: -F tests/appsec/waf/test_addresses.py::Test_GraphQL -F tests/appsec/test_blocking_addresses.py::Test_BlockingGraphqlResolvers
		FORCE_TESTS_SCENARIO: GRAPHQL_APPSEC

		# Propagate to downstream services the information that the current distributed trace is
		# containing at least one ASM security event

		# return true if absence of upstream ASM event (_dd.p.appsec:1)
		# and no local security event (which sets _dd.p.appsec:1 locally)

	def build_sampler(settings)
	# A custom sampler is provided
	if (sampler = settings.tracing.sampler)
	return sampler
	end

	# Sampling rules are provided
	if (rules = settings.tracing.sampling.rules)
	post_sampler = Tracing::Sampling::RuleSampler.parse(
	rules,
	settings.tracing.sampling.rate_limit,
	settings.tracing.sampling.default_rate
	)
	end

	# The default sampler.
	# Used if no custom sampler is provided, or if sampling rule parsing fails.
	post_sampler \|\|= Tracing::Sampling::RuleSampler.new(
	rate_limit: settings.tracing.sampling.rate_limit,
	default_sample_rate: settings.tracing.sampling.default_rate
	)

	Tracing::Sampling::PrioritySampler.new(
	base_sampler: Tracing::Sampling::AllSampler.new,
	post_sampler: post_sampler
	)
	end

	def initialize(
	trace_flush: Flush::Finished.new,
	context_provider: DefaultContextProvider.new,
	default_service: Core::Environment::Ext::FALLBACK_SERVICE_NAME,
	enabled: true,
	sampler: Sampling::PrioritySampler.new(
	base_sampler: Sampling::AllSampler.new,
	post_sampler: Sampling::RuleSampler.new
	),
	span_sampler: Sampling::Span::Sampler.new,
	tags: {},
	writer: Writer.new
	)

	def self.add_distributed_tags: (untyped trace) -> untyped
	def self.add_distributed_tags: (untyped trace) -> void

	def self.add_tags: (Datadog::AppSec::Scope scope, Datadog::AppSec::WAF::Result waf_result) -> untyped
	def self.add_tags: (Datadog::AppSec::Scope scope, Datadog::AppSec::WAF::Result waf_result) -> void

ASM Standalone Billing #3965

ASM Standalone Billing #3965

Conversation

vpellan commented Oct 1, 2024 • edited by jira bot Loading

pr-commenter bot commented Oct 1, 2024 • edited Loading

Benchmarks

codecov-commenter commented Oct 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot Oct 3, 2024

Choose a reason for hiding this comment

🔵 Code Vulnerability

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpellan Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

marcotc left a comment

Choose a reason for hiding this comment

y9v left a comment

Choose a reason for hiding this comment

vpellan commented Oct 1, 2024 •

edited by jira bot

Loading

pr-commenter bot commented Oct 1, 2024 •

edited

Loading

codecov-commenter commented Oct 1, 2024 •

edited

Loading

vpellan Oct 21, 2024 •

edited

Loading