ASM Standalone #2903
ASM Standalone #2903
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2903 +/- ##
=========================================
Coverage 74.82% 74.82%
Complexity 2741 2741
=========================================
Files 110 110
Lines 10863 10863
=========================================
Hits 8128 8128
Misses 2735 2735
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Benchmarks [ tracer ]Benchmark execution time: 2024-11-21 16:34:15 Comparing candidate commit 8095baa in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 176 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing
scenario:PDOBench/benchPDOBaseline-opcache
|
estringana
force-pushed
the
estringana/asm-standalone-2
branch
3 times, most recently
from
5feaef4
to
522e841
Compare
Benchmarks [ appsec ]Benchmark execution time: 2024-11-21 16:43:12 Comparing candidate commit 8095baa in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. |
estringana
force-pushed
the
estringana/asm-standalone-2
branch
3 times, most recently
from
5149b34
to
5d05f77
Compare
estringana
force-pushed
the
estringana/asm-standalone-2
branch
6 times, most recently
from
4b42290
to
b068df8
Compare
estringana
force-pushed
the
estringana/asm-standalone-2
branch
from
56c99bd
to
a0ee5cc
Compare
| if (Z_TYPE_P(rule) != IS_ARRAY) { | ||
| continue; | ||
| } | ||
| if (!get_global_DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED()) { // APPSEC_STANDALONE enabled, override sampling rules to be empty |
There was a problem hiding this comment.
This just wrap the code within if (!get_global_DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED()) {
estringana
force-pushed
the
estringana/asm-standalone-2
branch
2 times, most recently
from
3a73d3b
to
a93f131
Compare
| return false; | ||
| } | ||
|
|
||
| zval *root_span = zend_hash_index_find(Z_ARR_P(trace), 0); |
There was a problem hiding this comment.
I think by the way ddtrace is currently designed, the root span will (almost?) always be the first span.
But that's more by coincidence that the root span just happens to be the last span closed - and spans are serialized in reverse order.
This may or may not change in future via normal refactors. (?)
We should at least note somewhere that we rely on that being the case now.
There was a problem hiding this comment.
Ok. How would you go approach this then? I'm ok to implement it properly if it does not require any major refactoring
There was a problem hiding this comment.
I'm not suggesting to actually change it, but primarily to document that you rely on this now.
estringana
force-pushed
the
estringana/asm-standalone-2
branch
3 times, most recently
from
57ade7e
to
7fc63bc
Compare
| return false; | ||
| } | ||
|
|
||
| atomic_store(&dd_limiter->window.last_hit, timeval); |
There was a problem hiding this comment.
I suppose it doesn't matter if there's a possible race condition where two requests hit this at the same time. The worst which will happen is that rarely two traces are sent.
(Otherwise one could do a cmpxchg here.)
This reverts commit 43eb5af.
Signed-off-by: Bob Weinand <[email protected]>
estringana
force-pushed
the
estringana/asm-standalone-2
branch
from
7fc63bc
to
9917e7a
Compare
Description
Work in progress
Reviewer checklist