-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[receiver/pprof] initial commit to convert google/pprof to OTel profiles #42843
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
base: main
Are you sure you want to change the base?
Conversation
Follow up to open-telemetry#40548 TODO: - [ ] verify result - [ ] more tests Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
const ( | ||
// noAttrUnit is a helper to indicate that no | ||
// unit is associated to this Attribute. | ||
noAttrUnit = int32(-1) |
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.
Can this be a zero? I think with the "zero index == null" convention we've been trying to avoid having to use -1 as a special value.
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.
noAttrUnit is just an conversion internal helper and will not be seen in the resulting OTel profiles output. As every positive value can indicate a unit, i needed something to indicate that no unit is set for this attribute.
Signed-off-by: Florian Lehner <[email protected]>
Signed-off-by: Florian Lehner <[email protected]>
Thank you for this work. A nit on the test code, and a few notes on the TODOs in the code. Do you want to address the TODOs in this PR or deal with them in other PRs? |
lastFunctionTableIdx int32 | ||
} | ||
|
||
func convertPprofToPprofile(src *profile.Profile) (*pprofile.Profiles, error) { |
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.
Not a blocker for this PR, but if we are willing to add a pprof exporter, we could consider moving this to a pprof translator package
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.
maybe @MovieStoreGuy or @atoulme can answer the question on why they chose to create receiver/pprofreceiver
over a translator package.
Signed-off-by: Florian Lehner <[email protected]>
@atoulme the TODOs were a reminder for myself to discuss the topic of the id fields from pprof with the Profiling SIG. As this is not relevant for this PR, I have removed them with 00333f2. |
Follow up to #40548
FYI: @atoulme @felixge @aalexand
TODO: