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
Populate PeriodType appropriately #17
Conversation
fc37cc3
to
0b9f4c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch and sorry for the slow response. PTAL at my comment.
format.go
Outdated
@@ -60,6 +60,10 @@ func toPprof(s map[string]int, hz int) *profile.Profile { | |||
Unit: "nanoseconds", | |||
}, | |||
} | |||
p.PeriodType = &profile.ValueType{ | |||
Type: "cpu", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is misleading. fgprof does not produce CPU profiles. It produces "wallclock" profiles.
So maybe this?
Type: "cpu", | |
Type: "wallclock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
PeriodType is intended to explain the kind of events between sampled occurrences. While not strictly required to be set, all the Go stdlib profilers populate it, and this patch sets it equivalent to the standard CPU profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward for this, thanks for fix @brancz 👍🏽
Having both toPprof and toProfile was an accident and the latter was dead code.
Thanks for the patch. Sorry for the slow response. Noticed some accidental code duplication while reviewing this, so pushed a small fix for that before merging. |
PeriodType is intended to explain the kind of events between sampled
occurrences. While not strictly required to be set, all the Go stdlib
profilers populate it, and this patch sets it equivalent to the standard
CPU profiles.
Resolves part of #16