DataDog / dd-trace-py

Datadog Python APM Client
https://ddtrace.readthedocs.io/
Other
553 stars 416 forks source link

perf(iast): only patch common modules if iast is enabled #11516

Closed mabdinur closed 1 day ago

mabdinur commented 4 days ago

Avoids patching builtins.open, _io.BytesIO.read, _io.StringIO.read and os.system when IAST is disabled.

Checklist

Reviewer Checklist

github-actions[bot] commented 4 days ago

CODEOWNERS have been resolved as:

ddtrace/_monkey.py                                                      @DataDog/apm-core-python
ddtrace/settings/asm.py                                                 @DataDog/asm-python
brettlangdon commented 4 days ago

Regression test? is there a benchmark we'd expect to improve with this change? or should we have a performance regression test to help validate what this improves?

mabdinur commented 4 days ago

Regression test? is there a benchmark we'd expect to improve with this change? or should we have a performance regression test to help validate what this improves?

There isn't a particular benchmark I am trying to improve. I just don't see the value in patching these operations if IAST or ASM is disabled (we double check if iast/ASM is enabled in the wrappers). If folks don't see value in this change I can close this PR too.

pr-commenter[bot] commented 4 days ago

Benchmarks

Benchmark execution time: 2024-11-22 22:18:46

Comparing candidate commit d35e45f89ff7e7f459c2a8f3ad574af15e522c42 in PR branch munir/asm-avoid-unnecessary-patching with baseline commit b2a7d785c2141ad16ec1a2e56f2cce05d81c4159 in branch main.

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

scenario:iast_aspects-format_map_aspect

scenario:iast_aspects-ljust_aspect

scenario:iast_aspects-ospathbasename_aspect

scenario:iast_aspects-ospathdirname_aspect

scenario:iast_aspects-ospathjoin_aspect

scenario:iast_aspects-ospathnormcase_aspect

scenario:iast_aspects-ospathsplit_aspect

scenario:iast_aspects-ospathsplitext_aspect