feat(profiling) add thread names (#2934)

Author: realFlowControl

.github/workflows/prof_asan.yml

@@ -59,17 +59,11 @@ jobs:
             /tmp/build-cargo/
           key: ${{ runner.os }}-cargo-asan-${{ hashFiles('**/Cargo.lock') }}
 
-      - name: Fix kernel mmap rnd bits
-        # Asan in llvm 14 provided in ubuntu 22.04 is incompatible with
-        # high-entropy ASLR in much newer kernels that GitHub runners are
-        # using leading to random crashes: https://reviews.llvm.org/D148280
-        # https://github.com/actions/runner-images/issues/9491#issuecomment-1989718917
-        run: sysctl vm.mmap_rnd_bits=28
-
       - name: Run phpt tests
         run: |
           set -eux
           switch-php nts-asan
           cd profiling/tests
           cp -v $(php-config --prefix)/lib/php/build/run-tests.php .
+          export DD_PROFILING_OUTPUT_PPROF=/tmp/pprof
           php run-tests.php --show-diff --asan -d extension=datadog-profiling.so phpt

Cargo.lock

@@ -93,18 +93,27 @@ lazy_static! {
 
     /// The Server API the profiler is running under.
     static ref SAPI: Sapi = {
-        // Safety: sapi_module is initialized before minit and there should be
-        // no concurrent threads.
-        let sapi_module = unsafe { zend::sapi_module };
-        if sapi_module.name.is_null() {
-            panic!("the sapi_module's name is a null pointer");
-        }
+        #[cfg(not(test))]
+        {
+            // Safety: sapi_module is initialized before minit and there should be
+            // no concurrent threads.
+            let sapi_module = unsafe { zend::sapi_module };
+            if sapi_module.name.is_null() {
+                panic!("the sapi_module's name is a null pointer");
+            }
 
-        // Safety: value has been checked for NULL; I haven't checked that the
-        // engine ensures its length is less than `isize::MAX`, but it is a
-        // risk I'm willing to take.
-        let sapi_name = unsafe { CStr::from_ptr(sapi_module.name) };
-        Sapi::from_name(sapi_name.to_string_lossy().as_ref())
+            // Safety: value has been checked for NULL; I haven't checked that the
+            // engine ensures its length is less than `isize::MAX`, but it is a
+            // risk I'm willing to take.
+            let sapi_name = unsafe { CStr::from_ptr(sapi_module.name) };
+            Sapi::from_name(sapi_name.to_string_lossy().as_ref())
+        }
+        // When running `cargo test` we do not link against PHP, so `zend::sapi_name` is not
+        // available and we just return `Sapi::Unkown`
+        #[cfg(test)]
+        {
+            Sapi::Unknown
+        }
     };
 
     // Safety: PROFILER_NAME is a byte slice that satisfies the safety requirements.

profiling/src/lib.rs

@@ -7,6 +7,7 @@ mod uploader;
 pub use interrupts::*;
 pub use sample_type_filter::*;
 pub use stack_walking::*;
+use thread_utils::get_current_thread_name;
 use uploader::*;
 
 #[cfg(all(php_has_fibers, not(test)))]
@@ -1145,12 +1146,17 @@ impl Profiler {
     /// * `n_extra_labels` - Reserve room for extra labels, such as when the
     ///                      caller adds gc or exception labels.
     fn common_labels(n_extra_labels: usize) -> Vec<Label> {
-        let mut labels = Vec::with_capacity(4 + n_extra_labels);
+        let mut labels = Vec::with_capacity(5 + n_extra_labels);
         labels.push(Label {
             key: "thread id",
             value: LabelValue::Num(unsafe { libc::pthread_self() as i64 }, "id".into()),
         });
 
+        labels.push(Label {
+            key: "thread name",
+            value: LabelValue::Str(get_current_thread_name().into()),
+        });
+
         // SAFETY: this is set to a noop version if ddtrace wasn't found, and
         // we're getting the profiling context on a PHP thread.
         let context = unsafe { datadog_php_profiling_get_profiling_context.unwrap_unchecked()() };

profiling/src/profiling/mod.rs

@@ -1,5 +1,11 @@
+#[cfg(php_zts)]
+use crate::sapi::Sapi;
+use crate::SAPI;
+#[cfg(php_zts)]
+use libc::c_char;
 use libc::sched_yield;
 use log::warn;
+use once_cell::sync::OnceCell;
 use std::mem::MaybeUninit;
 use std::thread::JoinHandle;
 use std::time::{Duration, Instant};
@@ -69,3 +75,70 @@ pub fn join_timeout(handle: JoinHandle<()>, timeout: Duration, impact: &str) {
         std::panic::resume_unwind(err)
     }
 }
+
+thread_local! {
+    /// This is a cache for the thread name. It will not change after the thread has been
+    /// created, as SAPI's do not change thread names and ext-pthreads / ext-parallel do not
+    /// provide an interface for renaming a thread.
+    static THREAD_NAME: OnceCell<String> = const { OnceCell::new() };
+}
+
+pub fn get_current_thread_name() -> String {
+    THREAD_NAME.with(|name| {
+        name.get_or_init(|| {
+            #[cfg(php_zts)]
+            let mut thread_name = SAPI.to_string();
+            #[cfg(not(php_zts))]
+            let thread_name = SAPI.to_string();
+
+            #[cfg(php_zts)]
+            {
+                // So far, only FrankenPHP sets meaningful thread names
+                if *SAPI == Sapi::FrankenPHP {
+                    let mut name = [0u8; 32];
+
+                    let result = unsafe {
+                        libc::pthread_getname_np(
+                            libc::pthread_self(),
+                            name.as_mut_ptr() as *mut c_char,
+                            name.len(),
+                        )
+                    };
+
+                    if result == 0 {
+                        // If successful, convert the result to a Rust String
+                        let cstr =
+                            unsafe { std::ffi::CStr::from_ptr(name.as_ptr() as *const c_char) };
+                        let str_slice: &str = cstr.to_str().unwrap_or_default();
+                        if !str_slice.is_empty() {
+                            thread_name.push_str(": ");
+                            thread_name.push_str(str_slice);
+                        }
+                    }
+                }
+            }
+
+            thread_name
+        }).clone()
+    })
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use libc::c_char;
+
+    #[test]
+    fn test_get_current_thread_name() {
+        unsafe {
+            // When running `cargo test`, the thread name for this test will be set to
+            // `profiling::thread_utils::tests:` which would interfer with this test
+            libc::pthread_setname_np(
+                #[cfg(target_os = "linux")]
+                libc::pthread_self(),
+                b"\0".as_ptr() as *const c_char,
+            );
+        }
+        assert_eq!(get_current_thread_name(), "unknown");
+    }
+}

profiling/src/profiling/thread_utils.rs

@@ -18,6 +18,12 @@
             {
               "key": "thread id",
               "values_regex": "^[0-9]+$"
+            },
+            {
+              "key": "thread name",
+              "values": [
+                "cli"
+              ]
             }
           ]
         }

profiling/tests/correctness/exceptions.json

@@ -22,6 +22,12 @@
             {
               "key": "thread id",
               "values_regex": "^[0-9]+$"
+            },
+            {
+              "key": "thread name",
+              "values": [
+                "cli"
+              ]
             }
           ]
         }

profiling/tests/correctness/exceptions_zts.json

@@ -42,6 +42,6 @@ echo 'Done.';
 ?>
 --EXPECTREGEX--
 .* Exception profiling initialized with sampling distance: 20
-.* Sent stack sample of 2 frames, 2 labels with Exception RuntimeException to profiler.
+.* Sent stack sample of 2 frames, 3 labels with Exception RuntimeException to profiler.
 .*Done\..*
 .*

profiling/tests/phpt/exceptions_01.phpt

@@ -51,7 +51,7 @@ echo 'Done.';
 ?>
 --EXPECTREGEX--
 .* Exception profiling initialized with sampling distance: 20
-.* Sent stack sample of 1 frames, 2 labels with Exception RuntimeException to profiler.
+.* Sent stack sample of 1 frames, 3 labels with Exception RuntimeException to profiler.
 .*Worker [0-9] exited
 .*Done..*
 .*