Add DD_APPSEC_SCA_ENABLED new configuration variable #2557

estringana · 2024-03-05T15:10:24Z

Description

It is required to create a new configuration variable DD_APPSEC_SCA_ENABLED so customers can enable SCA. This variable is reported to the backend via telemetry and used there.

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

APPSEC-14721

pr-commenter · 2024-03-05T16:24:55Z

Benchmarks

Benchmark execution time: 2024-03-05 16:23:25

Comparing candidate commit fa41d2a in PR branch estringana/add-appsec-sca with baseline commit dc24c31 in branch master.

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

scenario:PDOBench/benchPDOBaseline

🟩 execution_time [-16.340µs; -13.325µs] or [-8.609%; -7.020%]

scenario:PDOBench/benchPDOBaseline-opcache

🟥 execution_time [+15.141µs; +16.403µs] or [+8.668%; +9.391%]

scenario:PDOBench/benchPDOOverhead

🟩 execution_time [-18.074µs; -16.050µs] or [-6.292%; -5.588%]

scenario:PDOBench/benchPDOOverhead-opcache

🟥 execution_time [+14.981µs; +17.044µs] or [+5.321%; +6.054%]

scenario:PDOBench/benchPDOOverheadWithDBM

🟩 execution_time [-18.020µs; -15.859µs] or [-5.828%; -5.129%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

🟥 execution_time [+15.028µs; +18.456µs] or [+4.861%; +5.970%]

Anilm3

Are there perhaps any tests showing that this variable is actually being sent?

codecov-commenter · 2024-03-06T12:11:14Z

Codecov Report

Merging #2557 (887c380) into master (dc24c31) will decrease coverage by 1.17%.
Report is 7 commits behind head on master.
The diff coverage is 80.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2557      +/-   ##
============================================
- Coverage     77.08%   75.91%   -1.17%     
  Complexity     2574     2574              
============================================
  Files           214      240      +26     
  Lines         23057    27033    +3976     
  Branches          0      976     +976     
============================================
+ Hits          17773    20522    +2749     
- Misses         5284     5991     +707     
- Partials          0      520     +520

Flag	Coverage Δ
appsec-extension	`69.13% <ø> (?)`
tracer-extension	`78.70% <80.00%> (-0.01%)`	⬇️
tracer-php	`75.08% <ø> (ø)`

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

Files	Coverage Δ
ext/configuration.h	`100.00% <ø> (ø)`
ext/telemetry.c	`100.00% <100.00%> (ø)`
ext/configuration.c	`78.26% <75.00%> (-0.21%)`	⬇️

... and 26 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 dc24c31...887c380. Read the comment docs.

estringana · 2024-03-06T13:17:31Z

ext/telemetry.c

@@ -53,6 +53,8 @@ void ddtrace_telemetry_finalize(void) {
            ddog_ConfigurationOrigin origin = DDOG_CONFIGURATION_ORIGIN_DEFAULT;
            if (!zend_string_equals_cstr(ini->value, cfg->default_encoded_value.ptr, cfg->default_encoded_value.len)) {
                origin = cfg->name_index >= 0 ? DDOG_CONFIGURATION_ORIGIN_ENV_VAR : DDOG_CONFIGURATION_ORIGIN_CODE;
+            } else {


I found that when a configuration is explicitly defined as a env/ini with the same value as the default value of that config, the value sent to telemetry was default and it should be EnvVar instead

bwoebi · 2024-03-06T14:28:19Z

I think you should test datadog.appsec.sca_enabled=1 in one of the tests instead of using the ENV there. I believe it needs to be handled in configuration.c so that it's parsed as datadog.appsec.sca_enabled instead of datadog.appsec_sca_enabled.

estringana · 2024-03-07T14:04:26Z

I think you should test datadog.appsec.sca_enabled=1 in one of the tests instead of using the ENV there. I believe it needs to be handled in configuration.c so that it's parsed as datadog.appsec.sca_enabled instead of datadog.appsec_sca_enabled.

Thanks for pointing that out @bwoebi . If I understood correctly, I fixed it

estringana · 2024-03-13T16:49:08Z

@bwoebi pr is ready. Can you review when you have a chance please?

bwoebi · 2024-03-13T17:22:47Z

ext/configuration.c

+        if (env_name.ptr == strstr(env_name.ptr, "DD_APPSEC_")) {
+            ini_name->ptr[sizeof("datadog.appsec") - 1] = '.';
+        }
+
        if (env_name.ptr == strstr(env_name.ptr, "DD_TRACE_")) {
            ini_name->ptr[sizeof("datadog.trace") - 1] = '.';
        }


Can we reorder this and put an else if there? Just to avoid eval of strstr when unnecessary.

Makes sense. Done

bwoebi

Overall LGTM :-)

Add DD_APPSEC_SCA_ENABLED new configuration variable

fa41d2a

estringana force-pushed the estringana/add-appsec-sca branch from d812229 to fa41d2a Compare March 5, 2024 15:51

Anilm3 reviewed Mar 6, 2024

View reviewed changes

estringana marked this pull request as ready for review March 6, 2024 13:16

estringana requested a review from a team as a code owner March 6, 2024 13:16

estringana commented Mar 6, 2024

View reviewed changes

estringana requested a review from bwoebi March 6, 2024 14:02

estringana added 2 commits March 6, 2024 15:06

Fix bug on origin of config not using default value

b5f08fc

Add sca tests

7f6fc56

estringana force-pushed the estringana/add-appsec-sca branch from 1b1225b to 7f6fc56 Compare March 6, 2024 14:07

Take into consideration appsec envs on the tracer

0f417ed

estringana force-pushed the estringana/add-appsec-sca branch from da51431 to 0f417ed Compare March 7, 2024 12:19

Amend style

5dbc387

bwoebi reviewed Mar 13, 2024

View reviewed changes

bwoebi approved these changes Mar 13, 2024

View reviewed changes

Refactor configuration parser

887c380

estringana merged commit 31e95da into master Mar 14, 2024
605 of 607 checks passed

estringana deleted the estringana/add-appsec-sca branch March 14, 2024 11:12

github-actions bot added this to the 0.99.0 milestone Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DD_APPSEC_SCA_ENABLED new configuration variable #2557

Add DD_APPSEC_SCA_ENABLED new configuration variable #2557

estringana commented Mar 5, 2024

pr-commenter bot commented Mar 5, 2024

Anilm3 left a comment

codecov-commenter commented Mar 6, 2024 •

edited

Loading

estringana Mar 6, 2024 •

edited

Loading

bwoebi commented Mar 6, 2024 •

edited

Loading

estringana commented Mar 7, 2024

estringana commented Mar 13, 2024

bwoebi Mar 13, 2024

estringana Mar 14, 2024

bwoebi left a comment

Add DD_APPSEC_SCA_ENABLED new configuration variable #2557

Add DD_APPSEC_SCA_ENABLED new configuration variable #2557

Conversation

estringana commented Mar 5, 2024

Description

Reviewer checklist

pr-commenter bot commented Mar 5, 2024

Benchmarks

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOBaseline-opcache

scenario:PDOBench/benchPDOOverhead

scenario:PDOBench/benchPDOOverhead-opcache

scenario:PDOBench/benchPDOOverheadWithDBM

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

Anilm3 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2024 • edited Loading

Codecov Report

estringana Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

bwoebi commented Mar 6, 2024 • edited Loading

estringana commented Mar 7, 2024

estringana commented Mar 13, 2024

bwoebi Mar 13, 2024

Choose a reason for hiding this comment

estringana Mar 14, 2024

Choose a reason for hiding this comment

bwoebi left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 6, 2024 •

edited

Loading

estringana Mar 6, 2024 •

edited

Loading

bwoebi commented Mar 6, 2024 •

edited

Loading