[chore][translator/pprof] Convert pprofile to pprof#44357
Conversation
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
After convertPprofToPprofile implement also convertPprofileToPprof. Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
This is mainly motivated by the desire to have a simple round-trip test.
There was a problem hiding this comment.
Thanks for working on this! PR LGTM for the most parts, but the testing needs improvement and there also seems to be a bug with the location indexes in the stack table getting reordered by accident.
You can merge the commits from this PR to address most of these issue: florianl#1
For the rest see my comments on the PR.
Suggestions for PR 44357
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
Friendly ping for feedback @open-telemetry/profiling-approvers |
christos68k
left a comment
christos68k
left a comment
There was a problem hiding this comment.
Didn't have time to test extensively
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
atoulme
left a comment
atoulme
left a comment
There was a problem hiding this comment.
I am ok approving and merging this PR, however, the functions created are not exported, and therefore will not be available outside the module, and they return a pointer to a pprofile.Profiles object rather than passing by value as most translators do.
I don't mind if we merge this PR and attend to this in subsequent PRs, just pointing out the code is not currently usable outside the module.
|
Thank you for the review @atoulme . I really appreciate the feedback regarding the export visibility and the pointer vs. value returns. I wanted to share some context on the current structure: keeping this PR in a reviewable state has been a significant time investment due to the constant upstream changes in the OTel Collector and Contrib repositories over the past few months since this PR got created. To ensure the logic landed safely and stayed manageable, I prioritized getting the core functionality implemented and stabilized first. I agree that making these functions public and integrating them into components like pprofreceiver is the right next step. However, doing so within this specific PR would have significantly increased the maintenance overhead and the 'surface area' for merge conflicts. My plan is to address the visibility and pass-by-value conventions in a follow-up PR immediately after this lands. That change should be much smaller, straightforward, and easier to review, which I hope will allow for a quicker turnaround. Looking forward to getting this landed so I can start on those improvements! |
|
Sounds good. Merged! |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Follow up to #44357 Implement the functionality of transforming pprof to OTel Profiles FYI: @open-telemetry/profiling-approvers <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.--> --------- Signed-off-by: Florian Lehner <florian.lehner@elastic.co> Co-authored-by: Antoine Toulme <antoine@toulme.name>
5 participants
Description
After
convertPprofToPprofile()implement alsoconvertPprofileToPprof().Once open-telemetry/opentelemetry-collector#13915 and this PR got merged, we can start a pprof to OTel profiles and vice versa conversion.
Ping @open-telemetry/profiling-approvers
Link to tracking issue
Fixes
Testing
Documentation