Skip to content

Commit

Permalink
fix(profiler) do not emit profiles when profiler is disabled (#2393)
Browse files Browse the repository at this point in the history
* add test to ensure the profiler does not emit profiles when inactive

* do not emit profiles if the feature is inactive
  • Loading branch information
realFlowControl committed Nov 28, 2023
1 parent 38b71b9 commit e8b86d1
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 1 deletion.
19 changes: 18 additions & 1 deletion .github/workflows/prof_correctness.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 29 additions & 0 deletions profiling/src/timeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down

0 comments on commit e8b86d1

Please sign in to comment.