Standalone ASM configuration and span tags #4291
Conversation
Overall package sizeSelf size: 6.51 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
packages/dd-trace/src/config.js
Outdated
| @@ -515,6 +515,7 @@ class Config { | |||
| this._setValue(defaults, 'traceId128BitGenerationEnabled', true) | |||
| this._setValue(defaults, 'traceId128BitLoggingEnabled', false) | |||
| this._setValue(defaults, 'tracing', true) | |||
| this._setValue(defaults, 'apmTracingEnabled', true) | |||
There was a problem hiding this comment.
I was tempted to name it apmTracing because of the tracing property but as it is a bool i think it's better to add the Enabled suffix
There was a problem hiding this comment.
agreed, but let's sync with APM on that
BenchmarksBenchmark execution time: 2024-05-21 14:10:28 Comparing candidate commit cb73c58 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 262 metrics, 4 unstable metrics. |
iunanua
force-pushed
the
igor/standalone-asm-config-and-tags
branch
2 times, most recently
from
ff3519d
to
2bb1295
Compare
iunanua
force-pushed
the
igor/standalone-asm-config-and-tags
branch
from
2bb1295
to
5d6210d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4291 +/- ##
===========================================
- Coverage 85.23% 63.82% -21.41%
===========================================
Files 252 244 -8
Lines 11042 10315 -727
Branches 33 33
===========================================
- Hits 9412 6584 -2828
- Misses 1630 3731 +2101 ☔ View full report in Codecov by Sentry. |
iunanua
changed the title
There was a problem hiding this comment.
Missing type for experimental.appsec.standalone.enabled in index.d.ts and test.ts
packages/dd-trace/test/appsec/iast/vulnerability-reporter.spec.js
Outdated
Show resolved
Hide resolved
packages/dd-trace/test/appsec/iast/vulnerability-reporter.spec.js
Outdated
Show resolved
Hide resolved
| if (fields.apmTracingEnabled === false) { | ||
| spanContext._trace.tags[APM_TRACING_ENABLED_KEY] = 0 |
There was a problem hiding this comment.
why don't we just check for the singleton here too ? instead of having a whole different variable ?
There was a problem hiding this comment.
al the moment, apm tracing is disabled when appsec standalone is true but in the future there might be other products with the need to disable apm tracing, thats why it is a different variable.
And also if we check here the standalone singleton, opentracing span would have a dependency with appsec...
There was a problem hiding this comment.
naming is horrible tho, the tracing is obviously enabled, it's just changing a tag.
Also I'm not sure how useful it is to futur-proof this right now when we're still in a very experimental phase. Feels a tiny bit over-engineered no ?
And the singleton isn't really appsec, you could literally put it in config.js and it would still work the exact same. The Appsec part is just from an arbitrary path you choose
There was a problem hiding this comment.
I think they are two different things:
-
standalone ASM is configured by
DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED. It "disables" apm tracing, adds a tag when appsec events occurs and also interferes in the priority sampler to change traces priority based on span tags (appsec specific tags). So these requirements are specific to appsec, they are not generic and that's whyappsec/standalonesingleton is in the appsec namespace. -
Other aspect is to disable apm tracing. I know, we only include a tag in the trace root span to indicate or pretend it is disabled (but that's what RFC says). So I see this as something generic and that's why I created
apmTracingEnabledconfig property to isolate the span context creation with appsec configuration.
I should have included an explanation like this in the PR description.
There was a problem hiding this comment.
you say the same thing twice xD
There was a problem hiding this comment.
let's see what APM thinks about this
There was a problem hiding this comment.
actually, the change would be in tracer.js as pointed out by ugaitz to continue using fields argument.
replacing tracer.js, line 37
this._apmTracingEnabled = config.apmTracingEnabled
by
this._apmTracingEnabled = config.appsec.standalone.enabled
There was a problem hiding this comment.
And apmTracingEnabled name is derived from the env var name proposed at the beginning in the RFC.
I'm open to suggestions if you don't like it
There was a problem hiding this comment.
I think overall APM should be the one to decide how they want to do this @rochdev
There was a problem hiding this comment.
or @Qard (as I believe you were talking about this in the guild)
Co-authored-by: Ugaitz Urien <ugaitz.urien@datadoghq.com>
Co-authored-by: simon-id <simon.id@datadoghq.com>
What does this PR do?
DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLEDenv var andexperimental.appsec.standalone.enabledconfig optionapmTracingEnabledconfig property.apmTracingEnabledis false, the_dd.apm.enabled:0tag is included in root spans.appsec.standalone.enabledis true, the_dd.p.appsecspan tag is included whether an appsec or iast event occurs.Motivation
This is the first part to support Standalone ASM billing. We have divided the feature into two PRs in order to facilitate the review.
Note for APM reviewers:
opentracing/tracer.jsmodified to passapmTracingEnabledflagopentracing/span.jsmodified to include_dd.apm.enabledtag,Practically the same as for
traceId128BitGenerationEnabledPlugin Checklist
Additional Notes