Improve unwind debugging #31
Conversation
- Fix order of process name and thread name
There was a problem hiding this comment.
Looks great! 🎉 Preemptively approving, I've left some small comments and nits.
As we discussed, we are going to move most of the thread name infrastructure to the metadata providers once we focus on that area. This will reduce the amount of reads from procfs.
As we don't have integration tests yet it would be super helpful if you could add some small "test plan" to the PR to show how this looks!
| @@ -535,12 +593,11 @@ impl Profiler<'_> { | |||
| } | |||
|
|
|||
| let raw_sample = RawAggregatedSample { | |||
| pid: key.task_id, | |||
| pid: key.pid, | |||
There was a problem hiding this comment.
Thanks for the fix! We should probably pass both the pid and task_id but we can do this in another PR if you prefer!
| let kstack_rep = format_symbolized_stack(&self.kstack); | ||
| let final_rep = write!( | ||
| f, | ||
| "SymbolizedAggregatedSample:\npid: {}\nustack: {}\nkstack: {}\ncount: {}", |
There was a problem hiding this comment.
This is lovely! Some small improvement we could make here is to use the formatting facilities from fmt::Formatter, something like:
| "SymbolizedAggregatedSample:\npid: {}\nustack: {}\nkstack: {}\ncount: {}", | |
| f.debug_struct("SymbolizedAggregatedSample") | |
| .field("pid", &pid) | |
| .field("ustack", &ustack) | |
| .finish() |
| let kstack_rep = format_native_stack(self.kstack); | ||
| let final_rep = write!( | ||
| f, | ||
| "RawAggregatedSample:\npid: {}\nustack: {}\nkstack: {}\ncount: {}", |
There was a problem hiding this comment.
Same as below, feel free to use the formatting helpers 😄
| // Completely failed to look up the PID/TID in procfs | ||
| Err(_) => ( | ||
| "<could not fetch proc comm>".to_string(), | ||
| "<could not fetch thread name>".to_string(), |
There was a problem hiding this comment.
nit: should we call them the same, either both name or comm?
|
(oh and let's squash the commits when merging!) |
- add doc for processes and threads
|
Good stuff!! 🎉 |
This improves unwind debugging to a degree:
implement Display trait for RawAggregatedSample
Implement Display trait for SymbolizedAggregatedSample
Since Linux PIDs are all actually threads, each folded stack should include both
commfield from/proc/<PGID>/stat)comm, we should name the threads as <binary_name>_thread_X, where X could be abs(PGID - PID).