Disable helper when appsec is fully disabled.

[cataphract]

Description

Also make datadog.appsec.enabled a system config. Note that the default value for datadog.appsec.enabled in ext/configuration.h (true) is different from that in datadog.appsec.disabled (false). This is intentional. appsec chooses between three states: explicitly enabled, explicitly disabled and controlled by remote config. Though a questionable decision, the config setting is still a boolean, and the third state is detected by a hack that determines whether the value was explicitly set. Because on the ddtrace side we want to suppress the helper only if explicitly disabled, we can set the default value to true, and disable the helper when the value is false (if the value is not the default true, then it was explicitly set to false).

Reviewer checklist

• [ ] Test coverage seems ok.
• [ ] Appropriate labels assigned.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.41%. Comparing base (7b487bd) to head (74be367).

Files with missing lines	Patch %	Lines
appsec/src/extension/helper_process.c	60.00%	3 Missing and 1 partial :warning:

:exclamation: There is a different number of reports uploaded between BASE (7b487bd) and HEAD (74be367). Click for more details.

HEAD has 1 upload less than BASE

| Flag | BASE (7b487bd) | HEAD (74be367) | |------|------|------| |tracer-php|12|11|

Additional details and impacted files

[](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) ```diff @@ Coverage Diff @@ ## master #2935 +/- ## ============================================ - Coverage 82.10% 72.41% -9.70% Complexity 2527 2527 ============================================ Files 108 135 +27 Lines 10360 14402 +4042 Branches 0 991 +991 ============================================ + Hits 8506 10429 +1923 - Misses 1854 3427 +1573 - Partials 0 546 +546 ``` | [Flag](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | Coverage Δ | | |---|---|---| | [appsec-extension](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | `68.40% <82.60%> (?)` | | | [tracer-php](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | `73.97% <ø> (-8.13%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) | Coverage Δ | | |---|---|---| | [appsec/src/extension/commands/request\_shutdown.c](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?src=pr&el=tree&filepath=appsec%2Fsrc%2Fextension%2Fcommands%2Frequest_shutdown.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-YXBwc2VjL3NyYy9leHRlbnNpb24vY29tbWFuZHMvcmVxdWVzdF9zaHV0ZG93bi5j) | `67.82% <ø> (ø)` | | | [appsec/src/extension/configuration.h](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?src=pr&el=tree&filepath=appsec%2Fsrc%2Fextension%2Fconfiguration.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-YXBwc2VjL3NyYy9leHRlbnNpb24vY29uZmlndXJhdGlvbi5o) | `100.00% <ø> (ø)` | | | [appsec/src/extension/ddappsec.c](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?src=pr&el=tree&filepath=appsec%2Fsrc%2Fextension%2Fddappsec.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-YXBwc2VjL3NyYy9leHRlbnNpb24vZGRhcHBzZWMuYw==) | `74.77% <100.00%> (ø)` | | | [appsec/src/extension/helper\_process.c](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?src=pr&el=tree&filepath=appsec%2Fsrc%2Fextension%2Fhelper_process.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog#diff-YXBwc2VjL3NyYy9leHRlbnNpb24vaGVscGVyX3Byb2Nlc3MuYw==) | `62.66% <60.00%> (ø)` | | ... and [31 files with indirect coverage changes](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog). Last update [7b487bd...74be367](https://app.codecov.io/gh/DataDog/dd-trace-php/pull/2935?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DataDog).

[bwoebi]

Note that the default value for datadog.appsec.enabled in ext/configuration.h (true) is different from that in datadog.appsec.disabled (false). This is intentional.

As long as this works properly ... Both extensions will write to the same ini_entry in PHP... I think it would be better to keep them in sync and do the name_index < 0 check in the tracer too. Or is there any reason we cannot do this?

[bwoebi]

I'm confused, dd_appsec_maybe_enable_helper still will have the sidecar sideload the appsec helper (if the sidecar is launched at all) - when the tracer is launched with the sidecar, there's no check in dd_appsec_maybe_enable_helper for appsec disabled?

[pr-commenter[bot]]

Benchmarks [ appsec ]

Benchmark execution time: 2024-11-11 12:24:19

Comparing candidate commit 74be367f87d16c4a0fe4fa6da17dff9bc9db6a7b in PR branch no-helper-with-appsec-disabled with baseline commit 7b487bd16a9a6bf71b38ade558867025c9aebd9f in branch master.

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

[pr-commenter[bot]]

Benchmarks [ tracer ]

Benchmark execution time: 2024-11-11 12:16:52

Comparing candidate commit 74be367f87d16c4a0fe4fa6da17dff9bc9db6a7b in PR branch no-helper-with-appsec-disabled with baseline commit 7b487bd16a9a6bf71b38ade558867025c9aebd9f in branch master.

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

scenario:MessagePackSerializationBench/benchMessagePackSerialization

• 🟥 execution_time [+4.284µs; +8.136µs] or [+2.599%; +4.936%]