Repository navigation
feat!: combined tracer+profiler in ddtrace.so - #4179
morrisonlevi wants to merge 114 commits into
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbed69efb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| " | ||
|
|
||
| DATADOG_EXTENSION_FLAGS="$DATADOG_EXTENSION_FLAGS -DDDTRACE" | ||
| DATADOG_EXTENSION_FLAGS="$DATADOG_EXTENSION_FLAGS -DTRACER -DSIDECAR" |
There was a problem hiding this comment.
Defining TRACER instead of DDTRACE leaves the unchanged #if DDTRACE in ext/sidecar.c:127 false in every tracer build. Consequently, when the sidecar reconnects after a restart or fork, dd_sidecar_post_connect() always passes NULL for agent_config_reader, so its named shared-memory mapping is never reopened and the tracer can continue using stale agent-provided sampling configuration.
Useful? React with 👍 / 👎.
| #ifdef TRACER | ||
| ddtrace_globals ddtrace; | ||
| #endif |
There was a problem hiding this comment.
The AppSec CMake target still compiles all tracer sources with DDTRACE=1 only (appsec/cmake/ddtrace.cmake:231-232), while this change moves the module globals and tracer lifecycle behind TRACER. AppSec extension and xtest builds therefore omit the ddtrace globals field and lifecycle declarations even though tracer sources still use DDTRACE_G(...), causing that build path to fail; its compile definitions need to migrate to TRACER/SIDECAR as the other build systems did.
Useful? React with 👍 / 👎.
| if ($hasCombinedProfiling) { | ||
| // The standalone profiler cannot coexist with combined ddtrace. | ||
| $replacements[$profilingExtensionPattern] = '; $0'; |
There was a problem hiding this comment.
When upgrading a legacy installation where an active extension=datadog-profiling.so line is what enables profiling, installing the combined package without repeating --enable-profiling now comments that line but never writes datadog.profiling.enabled=On. Because combined ddtrace.so defaults this setting to off—and the legacy installer commonly left only a commented datadog.profiling.enabled=1 entry—the upgrade silently disables profiling that was previously active; detect the active legacy line and migrate that state before commenting it.
Useful? React with 👍 / 👎.
cbed69e to
b347854
Compare
2 participants
Description
For the general case, we want to merge the tracer and profiler into one extension e.g. just
ddtrace.sowith nodatadog-profiling.so.Here are our build targets:
ddtrace.so+libdatadog_php.soandddtrace.socontains the tracer and profiler. Thelibdatadog_php.sois per platform and is PHP build agnostic, whereas ddtrace.so is very tied to the PHP build specifics.ddtrace.soonly and again, it contains the tracer and profiler.datadog-profiling.soonly. No sidecar stuff in it at all, not even compiled in, no tracer. This will not be shipped in our CI, or at least that's our plan for now.To make that work and have all the assembler/linker stuff be happy, I've split it into various features:
tracerwhich is used when building ddtrace.so and not when building datadog-profiling.so. (I think actually it can be removed in its current state, I don't think it's doing anything anymore)profilingwhich is generally used in all builds when supported e.g. not for PHP 7.0 nor for Windows.profiling-embeddedwhich is used when embedding profiling in ddtrace.so, depends on profiling.profiling-standalonewhich is used for datadog-profiling.so, depends on profiling.runtimewhich is Rust stuff that gets included by everything because it's used by config, logging. Gets put intodatadog-profiling.so,libdatadog_php.so, and/orddtrace.soas needed.tracer-runtimewhich is basically the tracer/sidecar portions written in Rust which can be compiled intoddtrace.sofor non-SSI but for SSI they go intolibdatadog_php.so.sidecarfor sidecar.otel-contextwhich is kinda likeruntimefor specifically OTel context stuff but IIRC there's a small technical reason it's separate, I forget what. In any case, I think it helps with code clarity e.g.features = "otel-context".And various dependencies are now linked to those features. Features here are additive but you cannot combine every option e.g. only one of profiling-embedded and profiling-standalone can be used.
The
datadog-setup.phpscript tries to understand some of these things and disable the profiler, but it's not guaranteed.TODO
make testshould probably work with the executable built frommake xlang-ltoe.g.make xlang-lto testshould build the combined extension via LTO and then run tests without rebuilding.I anticipate we'll have to run clippy multiple times rather thanDone.--all-targetsor similar.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