feat!: combined tracer+profiler in ddtrace.so - #4179
Conversation
# 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.
…P_INCLUDES" This reverts commit 6df11ac.
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.
|
Benchmarks [ profiler ]Benchmark execution time: 2026-09-11 19:16:53 Comparing candidate commit 1f08ff9 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 25 metrics, 9 unstable metrics.
|
Benchmarks [ tracer ]Benchmark execution time: 2026-09-11 20:09:03 Comparing candidate commit 1f08ff9 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 191 metrics, 2 unstable metrics.
|
…on-2 # Conflicts: # .claude/ci/shared-zai-tea-tests.md
This reverts commit 1fc782b.
| // Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
There was a problem hiding this comment.
Btw. we have a default license for the repo and don't add these individually.
There was a problem hiding this comment.
We definitely need to find a way to not do this.
Fine for the initial draft to test it, but this is not something I'm willing to accept. Also, like "preserve its historical ID ordering" wtf :-D
There was a problem hiding this comment.
Okay, I'll await your experiments that you'd said you'd perform for this (from slack conversation).
|
Can we ensure that there's at least one loader test with a proper stack walk producing an usable profile with SSI? |
2 participants
Description
This PR removes
datadog-profiling.soand moves the profiler intoddtrace.so. This can affect customer code if they are checkingextension_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 loadingdatadog-profiling.sowill mean the profiler isn't loaded. Thedatadog-setup.phpscript tries to understand some of these things and disable the profiler, but it's not guaranteed.Motivation
.so.Testing
This adds a task to
Loader test on <arch> libcjobs which ensures that we have awall-timesample with the expected stack for SSI. It tries to avoid dependencies and workflows which can introduce flakiness, so it uses the version of Python built into the image, uses libzstd rather than a Python package for it, the profile is written locally to disk, and so on. It piggy-backs onto the loader test instead of a new job because it takes 2+ minutes to pull images and git repos and just mere seconds to run the test.Reviewer checklist