Skip to content

feat!: combined tracer+profiler in ddtrace.so - #4179

Draft
morrisonlevi wants to merge 50 commits into
masterfrom
levi/common-extension-2
Draft

feat!: combined tracer+profiler in ddtrace.so#4179
morrisonlevi wants to merge 50 commits into
masterfrom
levi/common-extension-2

Conversation

@morrisonlevi

Copy link
Copy Markdown
Collaborator

Description

This PR removes datadog-profiling.so and moves the profiler into ddtrace.so. This can affect customer code if they are checking extension_loaded("datadog-profiling") or similar. It can also affect build systems and management if it moves or alters INI entries, such as making assumptions that not loading datadog-profiling.so will mean the profiler isn't loaded. The datadog-setup.php script tries to understand some of these things and disable the profiler, but it's not guaranteed.

Motivation

  • Theoretically, there's a size benefit. This has not been realized yet, and actually things have gotten bigger. Personally, I am not sure if that should block merging this; this is a large PR that will have a lot of conflicts, so we may wish to merge it even if we regress, and work on size later. But honestly this was one of the drivers, if we fail to meet this, then we've failed, and maybe we shouldn't ship this!
  • Less cross-object dependencies, as the tracer and profiler are now in the same .so.
  • TODO

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

# Conflicts:
#	.github/workflows/prof_correctness.yml
- loader: enable profiling in the JIT force-injection functional test
  cases so their assertions about profiler notification output match
  what the combined ddtrace.so actually does when DD_PROFILING_ENABLED
  is set.

- profiling: gc_mem_caches_01.phpt assumed a fixed amount of incidental
  garbage is always reclaimable right after RINIT. That's not true once
  the tracer is active in the same process (the combined build), since
  the tracer's own request-lifetime allocations (e.g. the root span)
  can consume the small amount of cached/free memory the test relied
  on, making gc_mem_caches() legitimately return 0 for reasons
  unrelated to allocation profiling. The test now generates and frees
  its own garbage so the assertion is robust in both standalone and
  combined builds.

- CI: the "profiling tests" job only ever built and exercised the
  standalone datadog-profiling.so, so gaps like the above were never
  caught. Added NTS and ZTS combined-mode (tracer + profiling in one
  ddtrace.so) build-and-test runs alongside the existing standalone
  runs.

Verified locally against registry.ddbuild.io/ci/dd-trace-php/dd-trace-ci:php-8.5_bookworm-10
for both NTS and ZTS: built the real combined ddtrace.so and standalone
datadog-profiling.so, reproduced the exact loader-test scenario
(including the .ddtrace.profiling marker convention), and ran the full
profiling/tests/phpt suite against both artifacts.
Add the generated Makefile as a prerequisite of the Rust archive rules so make rebuilds when php-config (NTS vs ZTS) changes, instead of relinking a stale ABI-incompatible archive. Also drop the standalone profiler build phases from CI's profiling tests job since we only ship combined.
config::minit() now runs immediately after the module-conflict/config-count checks, since it's what installs the log crate's logger (gated by datadog.profiling.log_level). Previously it ran after the tracing-subscriber setup and PHP_VERSION detection, so any log/warn/error calls there were silent no-ops regardless of configured level, and debug builds separately hardcoded an early Trace-level logger that unconditionally leaked a MINIT trace line into every phpt test using the debug combined ddtrace.so (e.g. CI's min install tests).
Temporarily point at DataDog/system-tests@levi/common-extension-2 to
pick up the install_ddtrace.sh profiling-marker detection fix until
it's merged upstream.
The gradle SSI task and the CMake SSI build both invoke cargo/compile_rust.sh
directly instead of going through the generated top-level Makefile, so they
never got Make's INCLUDES. components-rs/config_codegen.rs now requires
DDTRACE_PHP_INCLUDES to preprocess ext/configuration.h for the Rust config-ID
codegen, so both call sites need to export it via php-config themselves.
Fixes local-repro docs that ran phpize/configure/make directly in the
repo root, which overwrites the tracked top-level Makefile. Add
compile_combined/install_combined next to the existing
compile_profiler/install_profiler targets so combined tracer+profiler
builds (what CI actually ships) go through an isolated tmp/build_combined/
copy instead, and point docs at these targets.
CI data (create-multiarch-lib-injection-image) shows 306M/292M vs the
210M ratchet set in April. Confirmed via generate-ssi-package.sh history
that combining tracer+profiling into one ddtrace.so reduces per-API-version
size (removes a second linked copy of the shared Rust runtime) rather than
increasing it -- the growth is from unrelated upstream libdatadog additions
since April (crashtracker, stats computation, FFE, multi-config). Raise the
threshold instead of chasing a regression this branch didn't cause.
…on-2

# Conflicts:
#	.claude/ci/appsec-native-tests.md
#	.claude/ci/building-locally.md
#	.claude/ci/compile-artifacts.md
#	.claude/ci/index.md
#	.claude/ci/meta-job-group-doc.md
#	.claude/ci/packaging-oci.md
#	.gitlab/generate-package.php
#	.gitlab/generate-tracer.php
#	appsec/tests/integration/build.gradle
#	components-rs/build.rs
#	config.m4
#	loader/bin/test_apache_reload.sh
#	profiling/src/config.rs
#	tooling/bin/pecl-build
#	tooling/generate-supported-configurations.sh
config_codegen.rs (added by the combined-extension work) unconditionally
preprocesses ext/configuration.h and needs real PHP headers, but this
task's php-buildonly-rust image has no PHP under test to point at --
it builds libdatadog_php.so once and shares it across every PHP version
in the integration matrix.

Verified the macro list config_codegen.rs extracts doesn't vary by PHP
version, so it doesn't matter which SDK's headers are used. Rather than
pin a specific PHP version from php-buildonly-rust's image (which would
silently go stale once that version drops out of testMatrix), discover
whichever php-config happens to be present under /opt/php/*/bin, with an
explicit failure if none exist.

Fixes the failures in 'appsec integration tests (ssi): [test8.3-release-ssi]'
and the PHP 8.5 variant: both failed with the same
'DDTRACE_PHP_INCLUDES is required' panic from config_codegen.rs.
ConfigId/CONFIG_COUNT generation only serves profiling/src/config.rs, so
relocate config_codegen.rs and its generated config.rs (now config_id.rs)
into profiling/, and call config_codegen::build() from profiling/build.rs
instead of unconditionally from components-rs/build.rs.

Tracer-only builds (e.g. the SSI/portable-lib libdatadog_php.so, built
once and shared across every PHP version) no longer even reference this
codegen, so they never preprocess ext/configuration.h or need PHP headers
-- not just gated off, but structurally absent from that build path.

Also note in Cargo.toml that the "tracer" feature name is misleading.
config_codegen::build() now only ever runs from profiling::build(), which
already requires and validates DDTRACE_PHP_INCLUDES before calling it. So
the stub-header fallback (added when this codegen could still run from a
tracer-only build with no real PHP headers available) is now both
unnecessary and actively wrong: it silently shadowed real PHP headers
even in combined builds, and broke real PHP headers whose macros expect
__has_builtin to be defined by zend_portability.h (which the stub headers
left empty), causing the CentOS 7 combined-extension CI job to fail with
"missing binary operator before token '('" while preprocessing
zai_assert.h.

Pass profiling::build()'s already-validated DDTRACE_PHP_INCLUDES straight
into config_codegen::build() instead, and delete the now-dead stub
headers and configure_php_build().
This test is about the profiler's own helper threads (ddprof_time/
ddprof_upload), not the tracer/sidecar/telemetry. Since CI only builds
and tests the combined ddtrace.so, all of that machinery now starts up
here too as a side effect, adding background threads and a sidecar
connection attempt that add CPU contention unrelated to what this test
verifies, making its timing-sensitive SIGCHLD-reaping loop flakier on
constrained CI runners (e.g. "profiling tests: [7.3, amd64, ..._centos-7]"
failing with "leftover children").

Disable every flag that gates datadog_sidecar_should_enable() so no
sidecar connection is attempted, keeping the test's timing budget
focused on the profiler. Verified locally that a real ddtrace-sidecar
subprocess (datadog-ipc-helper) is spawned without these env vars and is
not spawned with them.
@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pipelines  Tests

Unblock PR with BitsAI

⚠️ Warnings

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 25 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | appsec integration tests: [test8.4-release-zts] — ❌ 51 tests failed · 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

❌ POST request sets content type and length() from com.datadog.appsec.php.integration.FrankenphpClassicTests
Assertion failed: 

assert span.meta['http.request.headers.content-type'] == 'application/json'
       |    |   |                                     |
       |    |   null                                  false
       |    ['http.status_code':'200', 'http.endpoint':'/hello.php', '_dd.p.ksr':'1', 'span.kind':'server', '_dd.p.dm':'-1', 'http.url':'http://docker:32773/hello.php', 'runtime-id':'6aab273f-d8f7-424d-9564-2606688566f5', 'env':'integration', 'http.method':'POST', 'http.useragent':'Java-http-client/17.0.20']
       com.datadog.appsec.php.model.Span@5e6522a6

Assertion failed: 

...
❌ [1] file_get_contents, 19 from com.datadog.appsec.php.integration.FrankenphpClassicTests
Assertion failed: 

assert span.metrics."_dd.appsec.enabled" == 1.0d
       |    |       |                    |
       |    |       null                 false
       |    ['process_id':14.0, 'php.memory.peak_usage_bytes':542936.0, 'php.memory.peak_real_usage_bytes':2097152.0, 'php.compilation.total_time_ms':0.165, '_dd.agent_psr':1.0, '_sampling_priority_v1':1.0]
       com.datadog.appsec.php.model.Span@92fd091

Assertion failed: 

...
❌ [1] file_put_contents, /tmp/dummy, 9 from com.datadog.appsec.php.integration.FrankenphpClassicTests
Assertion failed: 

assert span.metrics."_dd.appsec.enabled" == 1.0d
       |    |       |                    |
       |    |       null                 false
       |    ['process_id':14.0, 'php.memory.peak_usage_bytes':541032.0, 'php.memory.peak_real_usage_bytes':2097152.0, '_dd.agent_psr':1.0, 'php.compilation.total_time_ms':0.144, '_sampling_priority_v1':1.0]
       com.datadog.appsec.php.model.Span@3578a45b

Assertion failed: 

...
❌ [2] fopen, 16 from com.datadog.appsec.php.integration.FrankenphpClassicTests
Assertion failed: 

assert span.metrics."_dd.appsec.enabled" == 1.0d
       |    |       |                    |
       |    |       null                 false
       |    ['process_id':14.0, 'php.memory.peak_usage_bytes':503896.0, 'php.memory.peak_real_usage_bytes':2097152.0, '_dd.agent_psr':1.0, 'php.compilation.total_time_ms':0.0, '_sampling_priority_v1':1.0]
       com.datadog.appsec.php.model.Span@57fce8b

Assertion failed: 

...
❌ [2] readfile, /tmp/dummy, 15 from com.datadog.appsec.php.integration.FrankenphpClassicTests
Assertion failed: 

assert span.metrics."_dd.appsec.enabled" == 1.0d
       |    |       |                    |
       |    |       null                 false
       |    ['process_id':14.0, 'php.memory.peak_usage_bytes':485528.0, '_dd.agent_psr':1.0, 'php.memory.peak_real_usage_bytes':2097152.0, 'php.compilation.total_time_ms':0.0, '_sampling_priority_v1':1.0]
       com.datadog.appsec.php.model.Span@1efcba00

Assertion failed: 

...
↳ and 46 more — View all
DataDog/apm-reliability/dd-trace-php | check libxml2 version — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | min install tests — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

View all 25 failed jobs.

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 55.03% (+0.00%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: ee98168 | Docs | View more details | Give us feedback!

@pr-commenter

pr-commenter Bot commented Sep 10, 2026

Copy link
Copy Markdown

Benchmarks [ profiler ]

Benchmark execution time: 2026-09-10 21:07:34

Comparing candidate commit ee98168 in PR branch levi/common-extension-2 with baseline commit 774e243 in branch master.

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

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:php-profiler-timeline-memory-control

  • 🟥 cpu_user_time [+34.163ms; +43.251ms] or [+5.261%; +6.660%]
  • 🟥 execution_time [+34.774ms; +41.413ms] or [+4.979%; +5.930%]

@pr-commenter

pr-commenter Bot commented Sep 10, 2026

Copy link
Copy Markdown

Benchmarks [ tracer ]

Benchmark execution time: 2026-09-10 21:59:06

Comparing candidate commit ee98168 in PR branch levi/common-extension-2 with baseline commit 774e243 in branch master.

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

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟥 execution_time [+3.361µs; +4.499µs] or [+3.066%; +4.105%]

scenario:SamplingRuleMatchingBench/benchRegexMatching1

  • 🟥 execution_time [+73.803ns; +145.797ns] or [+5.012%; +9.901%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2

  • 🟥 execution_time [+90.863ns; +156.737ns] or [+6.150%; +10.608%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

  • 🟥 execution_time [+83.406ns; +143.994ns] or [+5.621%; +9.704%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4

  • 🟥 execution_time [+55.428ns; +156.172ns] or [+3.749%; +10.563%]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant