search

felixge / fgprof

🚀 fgprof is a sampling Go profiler that allows you to analyze On-CPU as well as Off-CPU (e.g. I/O) time together.
MIT License
2.81k stars 88 forks source link

PeriodType, PeriodUnit and Duration unset in resulting pprof #16

Closed brancz closed 1 year ago

brancz commented 2 years ago

I just noticed that the period type, period unit, and duration are not set in the resulting profiles created by fgprof. While not strictly required I feel it would be good to set them. Duration can be measured, and period type I assume should be cpu and period unit should be nanoseconds?

felixge commented 2 years ago

period type I assume should be cpu

"wallclock time" or "goroutine time" is probably a more suitable for the period type (sample type in pprof, right?)

Anyway, overall a PR for this would be super appreciated. I think I missed this because I was still new to the pprof format when I wrote this code. 🙇‍♂️

brancz commented 2 years ago

Period type is distinct from sample type. It's already SampleType: samples and SampleUnit: count for one of them and SampleType: time and SampleUnit: nanoseconds for the other. The period type and unit are intended to explain a measurement of events between sampled occurrences:

// The kind of events between sampled occurrences. // e.g [ "cpu","cycles" ] or [ "heap","bytes" ]

https://github.com/google/pprof/blob/83db2b799d1f74c40857232cb5eb4c60379fe6c2/proto/profile.proto#L82-L84

Since the observations by fgprof are done using a ticker, in principal similar to the stdlib CPU profile, I'd say we should probably be consistent with it.

brancz commented 2 years ago

I noticed a few more things that were unset. I think all of these should resolve those #17, #18, #19.

felixge commented 1 year ago

Closing this, since all related PRs have been merged.