-
Notifications
You must be signed in to change notification settings - Fork 376
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
ASM Standalone Billing #3965
ASM Standalone Billing #3965
Changes from all commits
03106a2
000bf66
fa4540d
fbfd4af
a617a2c
d0b5f33
31d6326
a3a26d0
63ab56b
81d20f2
091a27f
b32f0f8
7549839
c882353
e86c729
3c4de1a
f362662
a7c3eed
ee6dc53
72a1027
efcf8dd
9acdafb
452e7b4
3ffaddd
97d3eba
63c1b87
8575ed2
154f31d
56a6475
5f9a239
22f18a8
4a2bf67
e24310e
73f8af2
a2f973b
8e54f7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,10 @@ module AppSec | |
module Ext | ||
INTERRUPT = :datadog_appsec_interrupt | ||
SCOPE_KEY = 'datadog.appsec.scope' | ||
|
||
TAG_APPSEC_ENABLED = '_dd.appsec.enabled' | ||
TAG_APM_ENABLED = '_dd.apm.enabled' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this constant really belong to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe so as the only time it is added to a trace is when ASM standalone is activated. Usually, we don't want disable APM, this is for now the only case when it is disabled. |
||
TAG_DISTRIBUTED_APPSEC_EVENT = '_dd.p.appsec' | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
|
||
module Datadog | ||
module AppSec | ||
module Utils | ||
# Utility class to to AppSec-specific trace operations | ||
class TraceOperation | ||
def self.appsec_standalone_reject?(trace) | ||
Datadog.configuration.appsec.standalone.enabled && | ||
(trace.nil? || trace.get_tag(Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT) != '1') | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,15 @@ def internal_request?(request) | |
end | ||
|
||
def should_skip_distributed_tracing?(client_config) | ||
if Datadog.configuration.appsec.standalone.enabled | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that we add a bunch of 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
# 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] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we forgot to remove this when merging GraphQL changes? If so, would it be better to create a separate PR with these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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