Add appsec benchmarks by estringana · Pull Request #2791 · DataDog/dd-trace-php

estringana · 2024-08-14T14:58:52Z

Description

Integrates PHP AppSec into the Datadog benchmarking platform so that performance regressions are caught automatically before they ship.

Before this PR, there was no automated performance baseline for AppSec overhead. Changes to the RASP instrumentation would have silently impacted performance. This PR adds AppSec-specific benchmark scenarios to the suite and wires them into CI, so every PR against dd-trace-php that touches AppSec gets a performance comparison against the master baseline.

See also: benchmarking-platform#94 (the platform-side changes that enable this).

[APPSEC-54443]

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-08-14T15:07:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.07%. Comparing base (31382ec) to head (95aa3f3).
⚠️ Report is 610 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2791      +/-   ##
============================================
+ Coverage     81.05%   81.07%   +0.01%     
  Complexity     2517     2517              
============================================
  Files           146      146              
  Lines         14654    14654              
============================================
+ Hits          11878    11880       +2     
+ Misses         2776     2774       -2

Flag	Coverage Δ
tracer-extension	`78.18% <ø> (ø)`
tracer-php	`82.27% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31382ec...95aa3f3. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pr-commenter · 2024-09-02T14:10:26Z

Benchmarks [ profiler ]

Benchmark execution time: 2024-09-02 14:10:17

Comparing candidate commit 9db79ba in PR branch estringana/add-appsec-benchmarks with baseline commit 05ae627 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 9 unstable metrics.

pr-commenter · 2024-09-02T14:52:08Z

Benchmarks [ tracer ]

Benchmark execution time: 2024-09-25 09:08:18

Comparing candidate commit 95aa3f3 in PR branch estringana/add-appsec-benchmarks with baseline commit 31382ec in branch master.

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

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

🟥 execution_time [+103.079µs; +235.101µs] or [+3.494%; +7.968%]

scenario:EmptyFileBench/benchEmptyFileOverhead

🟩 execution_time [-185.295µs; -67.985µs] or [-5.855%; -2.148%]

scenario:LaravelBench/benchLaravelBaseline

🟥 execution_time [+7.138ms; +7.310ms] or [+230.113%; +235.670%]

scenario:LaravelBench/benchLaravelBaseline-opcache

🟥 execution_time [+7.287ms; +7.432ms] or [+233.886%; +238.513%]

scenario:LaravelBench/benchLaravelOverhead

🟥 execution_time [+7.781ms; +7.992ms] or [+235.894%; +242.277%]

scenario:LaravelBench/benchLaravelOverhead-opcache

🟥 execution_time [+7.837ms; +8.052ms] or [+231.866%; +238.216%]

scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache

🟩 execution_time [-215.290ns; -166.310ns] or [-12.663%; -9.782%]

scenario:WordPressBench/benchWordPressBaseline

🟥 mem_peak [+512.544KB; +512.544KB] or [+7.068%; +7.068%]

scenario:WordPressBench/benchWordPressBaseline-opcache

🟥 mem_peak [+198.464KB; +198.464KB] or [+4.415%; +4.415%]

scenario:WordPressBench/benchWordPressOverhead

🟥 mem_peak [+2.944MB; +2.944MB] or [+61.087%; +61.087%]

scenario:WordPressBench/benchWordPressOverhead-opcache

🟥 mem_peak [+2.688MB; +2.688MB] or [+134.034%; +134.034%]

pr-commenter · 2024-09-02T15:04:18Z

Benchmarks [ appsec ]

Benchmark execution time: 2024-09-11 09:53:43

Comparing candidate commit ccd86d5 in PR branch estringana/add-appsec-benchmarks with baseline commit 05ae627 in branch master.

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

ddyurchenko · 2024-09-02T15:17:58Z

About - Benchmarks [ tracer ]. Something is completely not right with WordPressBench/benchWordPressOverhead, should not experience such big perf difference. Other 4 scenarios seems to be what usually flaky / not super precise benchmarks look like.

I can increase flakiness threshold for you from 2% to 5%, so the small flaky results are hidden. For most teams we already use 5%, ensuring 2% threshold is possible, but would require quite some engineering time (and in general not sure if it is useful for you -- will you address a regression of size 2% anyhow?).

ddyurchenko · 2024-09-10T15:57:38Z

I keep wondering what is happening with scenario:WordPressBench/benchWordPressOverhead. It is consistently 80% faster across 5 last runs. It can be that we do something wrong in estringana/add-appsec branch in benchmarking-platform repo during candidate or baseline run.

pr-commenter · 2024-09-17T11:24:57Z

Benchmarks

Benchmark execution time: 2024-09-20 08:45:44

Comparing candidate commit 339adfc in PR branch estringana/add-appsec-benchmarks with baseline commit 31382ec in branch master.

Found 1 performance improvements and 8 performance regressions! Performance is the same for 168 metrics, 1 unstable metrics.

scenario:ContextPropagationBench/benchInject64Bit

🟥 execution_time [+158.425ns; +471.575ns] or [+2.014%; +5.994%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

🟩 execution_time [-236.273µs; -105.307µs] or [-7.039%; -3.137%]

scenario:LaravelBench/benchLaravelBaseline

🟥 execution_time [+7.176ms; +7.392ms] or [+226.330%; +233.156%]

scenario:LaravelBench/benchLaravelOverhead

🟥 execution_time [+7.756ms; +7.986ms] or [+230.602%; +237.461%]

scenario:LaravelBench/benchLaravelOverhead-opcache

🟥 execution_time [+7.787ms; +8.009ms] or [+226.349%; +232.819%]

scenario:WordPressBench/benchWordPressBaseline

🟥 mem_peak [+512.896KB; +512.896KB] or [+7.073%; +7.073%]

scenario:WordPressBench/benchWordPressBaseline-opcache

🟥 mem_peak [+198.672KB; +198.672KB] or [+4.419%; +4.419%]

scenario:WordPressBench/benchWordPressOverhead

🟥 mem_peak [+2.945MB; +2.945MB] or [+61.093%; +61.093%]

scenario:WordPressBench/benchWordPressOverhead-opcache

🟥 mem_peak [+2.689MB; +2.689MB] or [+134.039%; +134.039%]

estringana · 2024-09-20T15:35:06Z

I keep wondering what is happening with scenario:WordPressBench/benchWordPressOverhead. It is consistently 80% faster across 5 last runs. It can be that we do something wrong in estringana/add-appsec branch in benchmarking-platform repo during candidate or baseline run.

This is fixed. Basically, there was error connecting to the wordpress database. I think this problem was there before but can't understand why it was not spotted

estringana · 2024-09-20T15:37:24Z

 	$(call run_tests,--testsuite=metrics $(TESTS))

-benchmarks_run_dependencies: global_test_run_dependencies tests/Frameworks/Symfony/Version_5_2/composer.lock-php$(PHP_MAJOR_MINOR) tests/Frameworks/Laravel/Version_8_x/composer.lock-php$(PHP_MAJOR_MINOR) tests/Benchmarks/composer.lock-php$(PHP_MAJOR_MINOR)
+benchmarks_run_dependencies: global_test_run_dependencies tests/Frameworks/Symfony/Version_5_2/composer.lock-php$(PHP_MAJOR_MINOR) tests/Frameworks/Laravel/Version_10_x/composer.lock-php$(PHP_MAJOR_MINOR) tests/Benchmarks/composer.lock-php$(PHP_MAJOR_MINOR)


This is the reason why Laravel performance has increased. Basically, the autoloader didn't exists and all responses were fast because they were 500x

estringana · 2024-09-23T07:52:18Z

  timeout: 1h
  script:
    - export ARTIFACTS_DIR="$(pwd)/reports" && (mkdir "${ARTIFACTS_DIR}" || :)
-    - git clone --branch dd-trace-php https://gitlab-ci-token:${CI_JOB_TOKEN}@gitlab.ddbuild.io/DataDog/benchmarking-platform /platform && cd /platform


I will change this once the other branch is merged and this approved

estringana · 2024-09-23T07:53:18Z

  '8.3-buster': { <<: *linux_php_service, image: 'datadog/dd-trace-ci:php-8.3_buster' }
  'php-master-buster': { <<: *linux_php_service, image: 'datadog/dd-trace-ci:php-master_buster' }
  # --- Bookworm ---
+  '8.2-bookworm': { <<: *linux_php_service, image: 'datadog/dd-trace-ci:php-8.2_bookworm-3' }


Not really needed but I added it here because that's the image I use to test benchmarks. Appsec is not compiling on buster versions

estringana · 2024-09-23T07:54:06Z

        $this->TearDownAfterClass();
    }

-    public function enableWordPressTracing()


I have changed this so now we ensure the database is created before any test

estringana · 2024-09-23T07:54:49Z


 spl_autoload_register('\DDTrace\Tests\missing_ddtrace_class_fatal_autoloader', true);

+$phpunitVersionParts = class_exists('\PHPUnit\Runner\Version')


The next three files related with bootstrap are changed because benchmarks were loading appsec Mock. This changes avoid that happening

Basically I copied/pasted code from one file to another. All the code was already there but in different files

PROFeNoM

LGTM

Reminder: Don't forget to change the branch in .gitlab/benchmarks 👍

estringana force-pushed the estringana/add-appsec-benchmarks branch 2 times, most recently from 7955e0b to 979c3ad Compare August 28, 2024 14:13

estringana force-pushed the estringana/add-appsec-benchmarks branch from 979c3ad to bf6e1ef Compare August 30, 2024 13:49

estringana marked this pull request as ready for review September 2, 2024 12:40

estringana requested review from a team as code owners September 2, 2024 12:40

estringana requested review from PROFeNoM and ddyurchenko September 2, 2024 12:40

ddyurchenko approved these changes Sep 2, 2024

View reviewed changes

estringana force-pushed the estringana/add-appsec-benchmarks branch 2 times, most recently from 0813939 to 19a0904 Compare September 12, 2024 10:00

estringana commented Sep 16, 2024

View reviewed changes

Comment thread .gitlab/benchmarks.yml

estringana and others added 12 commits September 16, 2024 12:41

Add basic benchmarks skeleton

ebd0ab7

Point to my ongoing branch on benchmarks

c0e0ac5

Add some changes so benchmarks can run

24dab61

tweak: Ensure that appsec and tracer benchmarks have separate filenames

a93a97d

Install appsec extension and helper

bd11d00

Check debbuging logging files

be9e053

Output php modules installed

5883ce0

Enable appsec cli on benchmarks

3e88984

Update rules configuration

8ad0e7c

Remove debug mode

e8b8547

Avoid failure when no logs generated

d2bffd1

Add 8.2 bookworm

71c27b9

estringana force-pushed the estringana/add-appsec-benchmarks branch from 46f2665 to 9ecca43 Compare September 16, 2024 10:42

Point to temporary branch

7f15058

estringana force-pushed the estringana/add-appsec-benchmarks branch from f89eed7 to 7f15058 Compare September 16, 2024 15:58

estringana added 2 commits September 17, 2024 09:31

Fix tea tests

89c08fb

Remove log files after printing them

620b5d4

estringana added 4 commits September 18, 2024 09:51

Display php errors if any

9122e08

TEMP

da4f67a

Generate logs

415a3ae

Point benchmarks to the right version of Laravel

f38dddc

estringana force-pushed the estringana/add-appsec-benchmarks branch from 6b533a9 to 8293cfd Compare September 19, 2024 15:10

Fix wordpress database setup in benchmark tests

339adfc

estringana force-pushed the estringana/add-appsec-benchmarks branch from 8293cfd to 339adfc Compare September 20, 2024 08:20

Revert changes to makefile

aaec12b

estringana force-pushed the estringana/add-appsec-benchmarks branch from 5d2d179 to aaec12b Compare September 20, 2024 11:02

Avoid loading appsec mock on benchmarks

0b8f053

estringana commented Sep 20, 2024

View reviewed changes

estringana commented Sep 23, 2024

View reviewed changes

PROFeNoM reviewed Sep 23, 2024

View reviewed changes

Comment thread tests/bootstrap_phpunit.php

PROFeNoM reviewed Sep 23, 2024

View reviewed changes

Comment thread Makefile

Fix namespaces on bootstrap files

ffb0d10

PROFeNoM approved these changes Sep 24, 2024

View reviewed changes

Revert target benchmark branch

95aa3f3

estringana merged commit f117e22 into master Sep 25, 2024

estringana deleted the estringana/add-appsec-benchmarks branch September 25, 2024 09:49

github-actions Bot added this to the 1.4.0 milestone Sep 25, 2024


		spl_autoload_register('\DDTrace\Tests\missing_ddtrace_class_fatal_autoloader', true);

		$phpunitVersionParts = class_exists('\PHPUnit\Runner\Version')

Conversation

estringana commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer checklist

Uh oh!

codecov-commenter commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter Bot commented Sep 2, 2024

Benchmarks [ profiler ]

Uh oh!

pr-commenter Bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ tracer ]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

scenario:EmptyFileBench/benchEmptyFileOverhead

scenario:LaravelBench/benchLaravelBaseline

scenario:LaravelBench/benchLaravelBaseline-opcache

scenario:LaravelBench/benchLaravelOverhead

scenario:LaravelBench/benchLaravelOverhead-opcache

scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache

scenario:WordPressBench/benchWordPressBaseline

scenario:WordPressBench/benchWordPressBaseline-opcache

scenario:WordPressBench/benchWordPressOverhead

scenario:WordPressBench/benchWordPressOverhead-opcache

Uh oh!

pr-commenter Bot commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ appsec ]

Uh oh!

ddyurchenko commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddyurchenko commented Sep 10, 2024

Uh oh!

Uh oh!

pr-commenter Bot commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:ContextPropagationBench/benchInject64Bit

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

scenario:LaravelBench/benchLaravelBaseline

scenario:LaravelBench/benchLaravelOverhead

scenario:LaravelBench/benchLaravelOverhead-opcache

scenario:WordPressBench/benchWordPressBaseline

scenario:WordPressBench/benchWordPressBaseline-opcache

scenario:WordPressBench/benchWordPressOverhead

scenario:WordPressBench/benchWordPressOverhead-opcache

Uh oh!

estringana commented Sep 20, 2024

Uh oh!

estringana Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PROFeNoM left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

estringana commented Aug 14, 2024 •

edited

Loading

codecov-commenter commented Aug 14, 2024 •

edited

Loading

pr-commenter Bot commented Sep 2, 2024 •

edited

Loading

pr-commenter Bot commented Sep 2, 2024 •

edited

Loading

ddyurchenko commented Sep 2, 2024 •

edited

Loading

pr-commenter Bot commented Sep 17, 2024 •

edited

Loading