Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mangglesh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new capability to generate flame graph data in JSON format specifically for Go language profiles. This enhancement allows for more flexible and interactive visualization of Go CPU profiling data, improving performance analysis workflows.
Highlights
- New Output Type: A new
FlameJsonoutput type has been added to represent flame graph data in JSON format. This enables a new way to consume profiling data for visualization. - Go PProf Integration: The
PProfprofiling tool now supports theFlameJsonoutput type, allowing the generation of JSON flame graphs directly from Go CPU profiles. - JSON Conversion Logic: New Go code has been implemented to parse raw pprof output, extract sample and location data, and transform it into a hierarchical JSON structure suitable for flame graph visualization tools.
- File Extension: Profiles generated with the
FlameJsonoutput type will now automatically be assigned a.jsonfile extension.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for generating a JSON-based flame graph from Go's pprof output. The implementation introduces the new output type and conversion logic. Address the error handling, performance, and code duplication issues identified to ensure the new feature is robust and maintainable.
| childMap map[string]*Node | ||
| } | ||
|
|
||
| func (m *goPprofManager) convertRawToJson(content string) (string, error) { |
There was a problem hiding this comment.
Multiple calls to strconv.Atoi and strconv.ParseFloat ignore the returned error (e.g., lines 181, 186, 197, 198, 202, 214, 225). This can lead to silent data corruption if the input format is unexpected. Check and propagate all parsing errors to ensure data integrity.
| } | ||
| getSourceFile := func(fn string, locID int) string { | ||
| for _, line := range locations[locID] { | ||
| match := regexp.MustCompile(`(.+\.go):(\d+):`).FindStringSubmatch(line) |
There was a problem hiding this comment.
Inside getSourceFile, a regular expression is compiled on every call. Compile the regex once at the top of buildTree for performance.
| if err != nil { | ||
| return errors.Wrapf(err, "failed to convert raw profile to JSON for PID %s", job.PID) | ||
| } | ||
| file.Write(rawFilePath, flameJson) |
There was a problem hiding this comment.
The file.Write function suppresses errors. Failing to write the final profile is a critical failure. Handle and propagate the error.
err = os.WriteFile(rawFilePath, []byte(flameJson), 0644)
if err != nil {
return errors.Wrapf(err, "failed to write flamegraph JSON to %s", rawFilePath)
}| Callgrind OutputType = "callgrind" | ||
| Raw OutputType = "raw" | ||
| Pprof OutputType = "pprof" | ||
| FlameJson OutputType = "flame-json" // Used for AsyncProfiler flamegraph in JSON format |
There was a problem hiding this comment.
The comment for FlameJson indicates it's for AsyncProfiler, but this pull request introduces it for Go's PProf profiler. Clarify the comment to reflect its actual use.
| FlameJson OutputType = "flame-json" // Used for AsyncProfiler flamegraph in JSON format | |
| FlameJson OutputType = "flame-json" // Used for flamegraph in JSON format |
| case api.FlameJson: | ||
| return ".json" | ||
| default: | ||
| // api.FlameGraph | ||
| return ".pprof" |
There was a problem hiding this comment.
The default case comment // api.FlameGraph is misleading since FlameGraph is not a supported output type for the PProf tool. Clarify the comment to avoid confusion.
| case api.FlameJson: | |
| return ".json" | |
| default: | |
| // api.FlameGraph | |
| return ".pprof" | |
| default: | |
| // api.Pprof, api.HeapDump |
| sampleRegex := regexp.MustCompile(`^\s*(\d+)\s+(\d+):\s+(.+)`) | ||
| locLineRegex := regexp.MustCompile(`^\d+:`) |
There was a problem hiding this comment.
The regular expressions are compiled on every call to convertRawToJson. Define them as package-level variables for better performance.
var (
sampleRegex = regexp.MustCompile(`^\s*(\d+)\s+(\d+):\s+(.+)`)
locLineRegex = regexp.MustCompile(`^\d+:`)
)| } | ||
|
|
||
| for _, s := range samples { | ||
| stack := reverseStrings(s.StackFuncs) |
There was a problem hiding this comment.
The reverseStrings function is a duplicate of the goPprofManager.reverseStrings method defined on line 336. Remove the local function and use m.reverseStrings(s).
| if err != nil { | ||
| return errors.Wrapf(err, "failed to fetch CPU profile for PID %s", job.PID) | ||
| } | ||
| } else if job.OutputType == api.FlameJson { |
There was a problem hiding this comment.
There's duplicated logic for fetching the CPU profile between the FlameJson branch and the Raw branch (line 384). Extract the common parts into a helper function.
|
Closing — feature is stale (10 months old) and the implementation needs rework before merge (parses pprof --raw text instead of using the pprof Go library, custom JSON shape isn't compatible with standard flame-graph viewers, no tests). Please reopen with a fresh implementation if still needed. |
3 participants
No description provided.