Skip to content

Commit

Permalink
metadata: Extract facility to find process and thread names
Browse files Browse the repository at this point in the history
The previous implementation was buggy (see PR for a screenshot of the
profile). This commit fixes this and adds tests to this new logic.

Test Plan
=========

Added new tests + manual tests (see PR for screenshot).
  • Loading branch information
javierhonduco committed Sep 6, 2024
1 parent 4e4a190 commit 753bd59
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 58 deletions.
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
pub mod bpf;
pub mod collector;
pub mod ksym;
pub mod metadata;
pub mod object;
pub mod perf_events;
pub mod profile;
Expand Down
52 changes: 52 additions & 0 deletions src/metadata.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#[derive(Debug, PartialEq, Eq)]
pub struct TaskName {
pub main_thread: String,
pub current_thread: String,
}

impl TaskName {
pub fn errored() -> Self {
TaskName {
main_thread: "<could not fetch process name>".into(),
current_thread: "<could not fetch thread name>".into(),
}
}

pub fn for_task(task_id: i32) -> Result<TaskName, anyhow::Error> {
let task = procfs::process::Process::new(task_id)?.stat()?;
let main_task = procfs::process::Process::new(task.pgrp)?.stat()?;
let thread_name = if task.pid == task.pgrp {
"<main thread>".to_string()
} else {
task.comm
};
Ok(TaskName {
main_thread: main_task.comm,
current_thread: thread_name,
})
}
}

#[cfg(test)]
mod tests {
use super::*;
use nix::unistd;
use std::thread;

#[test]
fn test_thread_name() {
let names = TaskName::for_task(unistd::getpgrp().as_raw()).unwrap();
assert_eq!(names.current_thread, "<main thread>");

let builder = thread::Builder::new().name("funky-thread-name".to_string());

builder
.spawn(|| {
let names = TaskName::for_task(unistd::gettid().as_raw()).unwrap();
assert_eq!(names.current_thread, "funky-thread-na");
})
.unwrap()
.join()
.unwrap();
}
}
63 changes: 5 additions & 58 deletions src/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use tracing::{debug, error, span, Level};

use crate::bpf::profiler_bindings::native_stack_t;
use crate::ksym::KsymIter;
use crate::metadata::TaskName;
use crate::object::ExecutableId;
use crate::profiler::Frame;
use crate::profiler::FrameAddress;
Expand Down Expand Up @@ -144,65 +145,13 @@ pub fn fold_profile(profile: SymbolizedAggregatedProfile) -> String {
let kstack = kstack.join(";");
let count: String = sample.count.to_string();

// Getting the meatadata for the stack. This will be abstracted in the future in a common module.
let (process_name, thread_name) = match procfs::process::Process::new(sample.pid) {
// We successfully looked up the PID in procfs (we don't yet
// know if it's a PID/PGID/main thread or a TID/non-main thread)
Ok(p) => match p.stat() {
// Successfully got the pid/tid stat info
Ok(stat) => {
// Differentiate between PID/PGID/main thread or TID/non-main thread
if stat.pid == stat.pgrp {
// NOTE:
// This is the main thread for the PID/PGID
// If stat.pid() == stat.pgrp() for this process,
// this is a stack for the main thread
// of the pid, and stat.comm is the name of the
// process binary file, so use:
// process_name = stat.comm, and thread_name = "main_thread"
(stat.comm, "main_thread".to_string())
} else {
// NOTE:
// This is a non-main thread (TID) of a PID, so we
// have to look up the actual PID/PGID to get the
// process binary name
// As in, stat.comm is the name of the thread, and
// you have to look up the process binary name, so
// use:
// process_name = <derive from stat.pgrp>, and thread_name = stat.comm
//
let process_name = match procfs::process::Process::new(stat.pgrp) {
// We successfully looked up the PID/PGID of the TID in procfs
Ok(p) => match p.stat() {
// We successfully looked up the PID binary name from stat
Ok(stat2) => stat2.comm,
// We were unable to get the PID's binary name from stat
Err(_) => "<could not fetch process name>".to_string(),
},
// We failed to look up the PID/PGID of the TID in procfs
Err(_) => "<could not fetch process name>".to_string(),
};
(process_name, stat.comm)
}
}
// Was unable to lookup the PID binary or thread name from stat
Err(_) => (
"<could not fetch process name>".to_string(),
"<could not fetch thread name>".to_string(),
),
},
// Completely failed to look up the PID/TID in procfs
Err(_) => (
"<could not fetch process name>".to_string(),
"<could not fetch thread name>".to_string(),
),
};
let task_and_process_names = TaskName::for_task(sample.tid).unwrap_or(TaskName::errored());

writeln!(
folded,
"{};{}{}{} {}",
process_name,
thread_name,
task_and_process_names.main_thread,
task_and_process_names.current_thread,
if ustack.trim().is_empty() {
"".to_string()
} else {
Expand Down Expand Up @@ -233,14 +182,12 @@ pub fn symbolize_profile(
let ksyms = KsymIter::from_kallsyms().collect::<Vec<_>>();

for sample in profile {
debug!("--- raw sample:\n{}", sample);
let mut symbolized_sample: SymbolizedAggregatedSample = SymbolizedAggregatedSample {
pid: sample.pid,
tid: sample.tid,
count: sample.count,
..Default::default()
};
symbolized_sample.pid = sample.pid;
symbolized_sample.count = sample.count;

if let Some(ustack) = sample.ustack {
symbolized_sample.ustack =
Expand Down

0 comments on commit 753bd59

Please sign in to comment.