WAF telemetry #2735

cataphract · 2024-06-27T10:26:17Z

Description

See individual commits for descriptions of the changes.

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-06-27T10:33:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.97%. Comparing base (7b487bd) to head (b3f6f9f).

❗ There is a different number of reports uploaded between BASE (7b487bd) and HEAD (b3f6f9f). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (7b487bd) HEAD (b3f6f9f)

tracer-php 12 11

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2735      +/-   ##
============================================
- Coverage     82.10%   73.97%   -8.13%     
  Complexity     2527     2527              
============================================
  Files           108      108              
  Lines         10360    10360              
============================================
- Hits           8506     7664     -842     
- Misses         1854     2696     +842

Flag	Coverage Δ
tracer-php	`73.97% <ø> (-8.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files 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 7b487bd...b3f6f9f. Read the comment docs.

pr-commenter · 2024-06-27T10:54:19Z

Benchmarks

Benchmark execution time: 2024-07-13 00:47:21

Comparing candidate commit a225a90 in PR branch glopes/waf-telemetry with baseline commit 4c3832d in branch master.

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

scenario:EmptyFileBench/benchEmptyFileBaseline

🟩 execution_time [-398.670µs; -235.850µs] or [-14.264%; -8.439%]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

🟩 execution_time [-430.977µs; -266.503µs] or [-14.768%; -9.132%]

scenario:EmptyFileBench/benchEmptyFileOverhead

🟩 execution_time [-500.727µs; -340.853µs] or [-15.926%; -10.841%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

🟩 execution_time [-541.737µs; -402.743µs] or [-16.620%; -12.356%]

scenario:LaravelBench/benchLaravelBaseline

🟩 execution_time [-419.144µs; -246.976µs] or [-14.387%; -8.478%]

scenario:LaravelBench/benchLaravelBaseline-opcache

🟩 execution_time [-407.735µs; -267.945µs] or [-13.502%; -8.873%]

scenario:LaravelBench/benchLaravelOverhead

🟩 execution_time [-606.339µs; -426.181µs] or [-18.143%; -12.753%]

scenario:LaravelBench/benchLaravelOverhead-opcache

🟩 execution_time [-565.431µs; -362.429µs] or [-16.594%; -10.636%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

🟩 execution_time [-7.998µs; -5.782µs] or [-3.347%; -2.419%]

scenario:SymfonyBench/benchSymfonyBaseline

🟩 execution_time [-300.154µs; -218.546µs] or [-5.000%; -3.641%]

scenario:SymfonyBench/benchSymfonyBaseline-opcache

🟩 execution_time [-314.024µs; -269.696µs] or [-5.139%; -4.413%]

scenario:SymfonyBench/benchSymfonyOverhead

🟩 execution_time [-734.478µs; -678.602µs] or [-10.406%; -9.615%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

🟩 execution_time [-720.026µs; -639.954µs] or [-10.068%; -8.948%]

scenario:TraceSerializationBench/benchSerializeTrace

🟩 execution_time [-8.473µs; -5.227µs] or [-4.195%; -2.588%]

scenario:TraceSerializationBench/benchSerializeTrace-opcache

🟩 mem_peak [-167.139KB; -62.645KB] or [-7.538%; -2.825%]

scenario:WordPressBench/benchWordPressOverhead

🟩 execution_time [-11.984ms; -9.469ms] or [-5.473%; -4.325%]

scenario:WordPressBench/benchWordPressOverhead-opcache

🟩 execution_time [-10.767ms; -8.338ms] or [-4.933%; -3.821%]

ext/telemetry.c

pr-commenter · 2024-10-10T12:43:08Z

Benchmarks [ tracer ]

Benchmark execution time: 2024-11-11 12:23:45

Comparing candidate commit b3f6f9f in PR branch glopes/waf-telemetry with baseline commit 7b487bd in branch master.

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

scenario:PDOBench/benchPDOBaseline

🟥 execution_time [+15.094µs; +16.101µs] or [+6.754%; +7.205%]

scenario:PDOBench/benchPDOBaseline-opcache

🟥 execution_time [+8.749µs; +16.131µs] or [+3.884%; +7.161%]

pr-commenter · 2024-10-10T12:48:18Z

Benchmarks [ appsec ]

Benchmark execution time: 2024-11-11 12:23:13

Comparing candidate commit b3f6f9f in PR branch glopes/waf-telemetry with baseline commit 7b487bd in branch master.

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

appsec/src/extension/commands_helpers.c

Anilm3

I left a number of comments, everything looks correct albeit there could be some improvements around the helper telemetry API and the naming conventions.

appsec/src/extension/commands_helpers.c

appsec/src/extension/ddtrace.h

appsec/src/helper/client.cpp

appsec/src/helper/metrics.cpp

appsec/src/helper/remote_config/client_handler.cpp

appsec/src/helper/service.hpp

appsec/src/helper/subscriber/waf.cpp

estringana · 2024-11-12T12:05:58Z

appsec/src/extension/commands_helpers.c

@@ -648,6 +678,103 @@ bool dd_command_process_metrics(mpack_node_t root, zend_object *nonnull span)
    return true;
 }

+static void _handle_telemetry_metric(const char *nonnull key_str,


Not sure if it is a good practice or I made it up but, should this early declaration be at the top?

You put these at the top if you use these multiple times at various places across a file. For a single use like here, there's no reason to.

estringana · 2024-11-12T12:17:46Z

appsec/src/extension/ddtrace.c

@@ -123,6 +147,7 @@ void dd_trace_startup()

    if (get_global_DD_APPSEC_TESTING()) {
        _register_testing_objects();
+        _setup_testing_telemetry_functions();


I coudln't get why this is within APPSEC_TESTING. I'm probably missing something

Ok. Makes sense

estringana · 2024-11-12T12:23:21Z

appsec/src/helper/network/proto.hpp

@@ -89,6 +89,9 @@ template <typename T> struct base_response_generic : public base_response {
    }
 };

+using telemetry_metrics =


I think this is not used

estringana · 2024-11-12T12:34:31Z

appsec/src/extension/commands_helpers.c

+        }                                                                      \
+    } while (0)
+
+    HANDLE_METRIC("waf.requests", DDTRACE_METRIC_TYPE_COUNT);


I saw you added some integration tests checking this metrics but I am missing some PHPT checking the extension.

It would also allow you to test unknown telemetry metrics

cataphract requested review from a team as code owners June 27, 2024 10:26

cataphract force-pushed the glopes/waf-telemetry branch 2 times, most recently from 8464095 to d91cd3f Compare June 27, 2024 17:35

iamluc reviewed Jun 28, 2024

View reviewed changes

ext/telemetry.c Outdated Show resolved Hide resolved

cataphract force-pushed the glopes/waf-telemetry branch 5 times, most recently from e777ab6 to 8239978 Compare July 4, 2024 16:53

cataphract closed this Jul 5, 2024

cataphract force-pushed the glopes/waf-telemetry branch from 8239978 to 3cdc077 Compare July 5, 2024 08:21

cataphract reopened this Jul 5, 2024

cataphract force-pushed the glopes/waf-telemetry branch 9 times, most recently from 6b37ec2 to ad72485 Compare July 11, 2024 17:19

cataphract force-pushed the glopes/waf-telemetry branch 3 times, most recently from 1b22395 to a225a90 Compare July 13, 2024 00:19

cataphract force-pushed the glopes/waf-telemetry branch from a225a90 to ff61a19 Compare October 10, 2024 12:13

cataphract force-pushed the glopes/waf-telemetry branch 4 times, most recently from 4e07791 to e00d8b6 Compare October 15, 2024 11:47

Anilm3 reviewed Oct 22, 2024

View reviewed changes

appsec/src/extension/commands_helpers.c Outdated Show resolved Hide resolved

Anilm3 approved these changes Oct 22, 2024

View reviewed changes

cataphract force-pushed the glopes/waf-telemetry branch 2 times, most recently from 9ae5168 to 7f4cb58 Compare November 6, 2024 18:02

cataphract mentioned this pull request Nov 11, 2024

telemetry worker: flush data after stops DataDog/libdatadog#515

Open

cataphract added 3 commits November 11, 2024 11:46

Telemetry in AppSec

eb8b87b

Address PR comments

1c02927

appsec helper: abstract tags format

b3f6f9f

cataphract force-pushed the glopes/waf-telemetry branch from 7f4cb58 to b3f6f9f Compare November 11, 2024 11:47

estringana reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WAF telemetry #2735

WAF telemetry #2735

cataphract commented Jun 27, 2024

codecov-commenter commented Jun 27, 2024 •

edited

Loading

pr-commenter bot commented Jun 27, 2024 •

edited

Loading

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

Anilm3 left a comment

estringana Nov 12, 2024

bwoebi Nov 12, 2024 •

edited

Loading

estringana Nov 12, 2024

estringana Nov 12, 2024

estringana Nov 12, 2024

estringana Nov 12, 2024

estringana Nov 12, 2024

WAF telemetry #2735

Are you sure you want to change the base?

WAF telemetry #2735

Conversation

cataphract commented Jun 27, 2024

Description

Reviewer checklist

codecov-commenter commented Jun 27, 2024 • edited Loading

Codecov Report

pr-commenter bot commented Jun 27, 2024 • edited Loading

Benchmarks

scenario:EmptyFileBench/benchEmptyFileBaseline

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

scenario:EmptyFileBench/benchEmptyFileOverhead

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

scenario:LaravelBench/benchLaravelBaseline

scenario:LaravelBench/benchLaravelBaseline-opcache

scenario:LaravelBench/benchLaravelOverhead

scenario:LaravelBench/benchLaravelOverhead-opcache

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

scenario:SymfonyBench/benchSymfonyBaseline

scenario:SymfonyBench/benchSymfonyBaseline-opcache

scenario:SymfonyBench/benchSymfonyOverhead

scenario:SymfonyBench/benchSymfonyOverhead-opcache

scenario:TraceSerializationBench/benchSerializeTrace

scenario:TraceSerializationBench/benchSerializeTrace-opcache

scenario:WordPressBench/benchWordPressOverhead

scenario:WordPressBench/benchWordPressOverhead-opcache

pr-commenter bot commented Oct 10, 2024 • edited Loading

Benchmarks [ tracer ]

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOBaseline-opcache

pr-commenter bot commented Oct 10, 2024 • edited Loading

Benchmarks [ appsec ]

Anilm3 left a comment

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

bwoebi Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

estringana Nov 12, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jun 27, 2024 •

edited

Loading

pr-commenter bot commented Jun 27, 2024 •

edited

Loading

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

pr-commenter bot commented Oct 10, 2024 •

edited

Loading

bwoebi Nov 12, 2024 •

edited

Loading