Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PeriodType, PeriodUnit and Duration unset in resulting pprof #16

Closed
brancz opened this issue Apr 27, 2022 · 4 comments
Closed

PeriodType, PeriodUnit and Duration unset in resulting pprof #16

brancz opened this issue Apr 27, 2022 · 4 comments

Comments

@brancz
Copy link
Contributor

brancz commented Apr 27, 2022

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
Copy link
Owner

felixge commented Apr 27, 2022

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
Copy link
Contributor Author

brancz commented Apr 27, 2022

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
Copy link
Contributor Author

brancz commented Apr 27, 2022

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

@felixge
Copy link
Owner

felixge commented Aug 27, 2022

Closing this, since all related PRs have been merged.

@felixge felixge closed this as completed Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants