-
Notifications
You must be signed in to change notification settings - Fork 384
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4479 +/- ##
==========================================
+ Coverage 97.73% 97.76% +0.02%
==========================================
Files 1386 1392 +6
Lines 84166 84891 +725
Branches 4257 4277 +20
==========================================
+ Hits 82262 82995 +733
+ Misses 1904 1896 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 20782 Passed, 1370 Skipped, 3m 13.89s Total Time |
BenchmarksBenchmark execution time: 2025-03-26 10:50:51 Comparing candidate commit 839e483 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. scenario:method instrumentation
scenario:tracing - Propagation - Datadog
|
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.
FYI: Currently, there is a karafka integration under review which contains distributed tracing mechanism
What is the process for this kind of situation ? If I merge to master first, should I myself add a commit on the Karafka integration to add APM Disablement capabilities ? (Of course if the other way around happens I could just rebase and add it here) |
I would suggest not to wait on it. However, it would be much welcomed to encapsulate the logic so that the pattern can be adopted easily. |
…t the rate limiter)
10bc01d
to
cc9fd0c
Compare
4544628
to
fea594d
Compare
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.
LGTM 👍🏼
Great job, lots of effort in it and thanks for keep pushing! I left few non-blocking suggestions and IMHO it's good to go 🚀
I'll review it again with the the rate limiting, instead of disablement in mind.
What does this PR do?
This PR fixes multiple bugs on APM Disablement V1 (aka ASM Standalone) when it was activated:
Motivation:
When I tried to activate SCA Standalone system-tests, I discovered that disabling AppSec and disabling APM Tracing would result in heartbeat traces not sending dd.apm.enabled=0 tag. During my investigation, I discovered more bugs (listed above).
This is also a step towards APM Disablement V2. For now, the variables are still called after appsec standalone, but this will change to APM disablement in the future and lose all reference to asm standalone, as it is in fact an APM feature.
Change log entry
None.
Additional Notes:
This PR should be followed soon by APM Disablement V2, which will replace the env var by DD_APM_TRACING_ENABLED, and every config/variable referencing asm standalone to apm disablement.
I understand that there might be some concerns about AppSec stuff being in APM territory but this is mostly a matter of naming that will be fixed when APM Disablement V2 will be released. In the V2, I will replace any instance of
Datadog.configuration.appsec.standalone.enabled
byDatadog.configuration.tracing.apm.disabled
,Datadog::AppSec::Ext::TAG_DISTRIBUTED_APPSEC_EVENT
by something likeDatadog::Tracing::Ext::TAG_DISTRIBUTED_TRACE_SOURCE
(_dd.p.ts
, which will be a bitmask). The only exception might be in thepropagate_sampling_priority?
method, in tracer.rb, where we check if AppSec is activated.How to test the change?
CI and system-tests