From e8b86d131c645f198820176928b4c85b49478938 Mon Sep 17 00:00:00 2001 From: Florian Engelhardt Date: Tue, 28 Nov 2023 17:05:17 +0100 Subject: [PATCH] fix(profiler) do not emit profiles when profiler is disabled (#2393) * add test to ensure the profiler does not emit profiles when inactive * do not emit profiles if the feature is inactive --- .github/workflows/prof_correctness.yml | 19 ++++++++++++++++- profiling/src/timeline.rs | 29 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/.github/workflows/prof_correctness.yml b/.github/workflows/prof_correctness.yml index 2b07a6c96e..e065d9e6d4 100644 --- a/.github/workflows/prof_correctness.yml +++ b/.github/workflows/prof_correctness.yml @@ -55,13 +55,30 @@ jobs: target/ key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} + - name: Run no profile test + run: | + export DD_PROFILING_ENABLED=Off + export DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=1 + export DD_PROFILING_EXPERIMENTAL_EXCEPTION_ENABLED=1 + export DD_PROFILING_EXPERIMENTAL_EXCEPTION_SAMPLING_DISTANCE=1 + php -d extension=target/release/libdatadog_php_profiling.so --ri datadog-profiling + for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions"; do + mkdir -p profiling/tests/correctness/"$test_case"/ + export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof + php -d extension=$PWD/target/release/libdatadog_php_profiling.so profiling/tests/correctness/"$test_case".php + if [ -f "$DD_PROFILING_OUTPUT_PPROF".1.lz4 ]; then + echo "File $DD_PROFILING_OUTPUT_PPROF.1.lz4 should not exist!" + exit 1; + fi + done + - name: Run tests run: | export DD_PROFILING_LOG_LEVEL=trace export DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED=1 export DD_PROFILING_EXPERIMENTAL_EXCEPTION_ENABLED=1 export DD_PROFILING_EXPERIMENTAL_EXCEPTION_SAMPLING_DISTANCE=1 - php -d extension=target/release/libdatadog_php_profiling.so -v + php -d extension=target/release/libdatadog_php_profiling.so --ri datadog-profiling for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions"; do mkdir -p profiling/tests/correctness/"$test_case"/ export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof diff --git a/profiling/src/timeline.rs b/profiling/src/timeline.rs index 27e39cc71d..27f902788d 100644 --- a/profiling/src/timeline.rs +++ b/profiling/src/timeline.rs @@ -37,6 +37,17 @@ fn try_sleeping_fn( execute_data: *mut zend_execute_data, return_value: *mut zval, ) -> anyhow::Result<()> { + let timeline_enabled = REQUEST_LOCALS.with(|cell| { + cell.try_borrow() + .map(|locals| locals.profiling_experimental_timeline_enabled) + .unwrap_or(false) + }); + + if !timeline_enabled { + unsafe { func(execute_data, return_value) }; + return Ok(()); + } + let start = Instant::now(); // SAFETY: simple forwarding to original func with original args. @@ -186,6 +197,10 @@ pub fn timeline_rinit() { return; }; + if !locals.profiling_experimental_timeline_enabled { + return; + } + IDLE_SINCE.with(|cell| { // try to borrow and bail out if not successful let Ok(idle_since) = cell.try_borrow() else { @@ -211,6 +226,16 @@ pub fn timeline_rinit() { /// This function is run during the P-RSHUTDOWN phase and resets the `IDLE_SINCE` thread local to /// "now", indicating the start of a new idle phase pub fn timeline_prshutdown() { + let timeline_enabled = REQUEST_LOCALS.with(|cell| { + cell.try_borrow() + .map(|locals| locals.profiling_experimental_timeline_enabled) + .unwrap_or(false) + }); + + if !timeline_enabled { + return; + } + IDLE_SINCE.with(|cell| { // try to borrow and bail out if not successful let Ok(mut idle_since) = cell.try_borrow_mut() else { @@ -230,6 +255,10 @@ pub(crate) fn timeline_mshutdown() { return; }; + if !locals.profiling_experimental_timeline_enabled { + return; + } + IDLE_SINCE.with(|cell| { // try to borrow and bail out if not successful let Ok(idle_since) = cell.try_borrow() else {