Add DD_APPSEC_SCA_ENABLED new configuration variable by estringana · Pull Request #2557 · DataDog/dd-trace-php

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

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.91%. Comparing base (dc24c31) to head (887c380).
⚠️ Report is 897 commits behind head on master.

Files with missing lines	Patch %	Lines
ext/configuration.c	75.00%	1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (80.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

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 with missing lines	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.

🚀 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.

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

            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

+        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

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 merged 6 commits intomasterfrom
estringana/add-appsec-sca

estringana commented Mar 5, 2024

Uh oh!

pr-commenter Bot commented Mar 5, 2024

Uh oh!

Anilm3 left a comment

Uh oh!

codecov-commenter commented Mar 6, 2024 •

edited

Loading

Uh oh!

estringana Mar 6, 2024 •

edited

Loading

Uh oh!

bwoebi commented Mar 6, 2024 •

edited

Loading

Uh oh!

estringana commented Mar 7, 2024

Uh oh!

estringana commented Mar 13, 2024

Uh oh!

bwoebi Mar 13, 2024

Uh oh!

estringana Mar 14, 2024

Uh oh!

bwoebi left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

estringana commented Mar 5, 2024

Description

Reviewer checklist

Uh oh!

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

Uh oh!

Anilm3 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estringana commented Mar 7, 2024

Uh oh!

estringana commented Mar 13, 2024

Uh oh!

bwoebi Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

estringana Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

bwoebi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 6, 2024 •

edited

Loading

estringana Mar 6, 2024 •

edited

Loading

bwoebi commented Mar 6, 2024 •

edited

Loading