This repository has been archived by the owner on Mar 7, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 243
Flamegraph options #50
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0701a25
Enable more FlameGraph options
tgrk 9879969
Fix default width information
tgrk 4b7c45d
Update doc with correct CLI arguments
tgrk 28e1b37
Fixing wording, typos and improving error messages
tgrk bca3f67
Apply wording changes rest of the changes and related tests
tgrk a325c5d
Fix output CLI arguments validation and messages based on PR review
tgrk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ import ( | |
"fmt" | ||
"io/ioutil" | ||
"os" | ||
"strconv" | ||
"strings" | ||
|
||
"github.com/uber/go-torch/pprof" | ||
|
@@ -42,10 +43,16 @@ type options struct { | |
} | ||
|
||
type outputOptions struct { | ||
File string `short:"f" long:"file" default:"torch.svg" description:"Output file name (must be .svg)"` | ||
Print bool `short:"p" long:"print" description:"Print the generated svg to stdout instead of writing to file"` | ||
Raw bool `short:"r" long:"raw" description:"Print the raw call graph output to stdout instead of creating a flame graph; use with Brendan Gregg's flame graph perl script (see https://github.com/brendangregg/FlameGraph)"` | ||
Title string `long:"title" default:"Flame Graph" description:"Graph title to display in the output file"` | ||
File string `short:"f" long:"file" default:"torch.svg" description:"Output file name (must be .svg)"` | ||
Print bool `short:"p" long:"print" description:"Print the generated svg to stdout instead of writing to file"` | ||
Raw bool `short:"r" long:"raw" description:"Print the raw call graph output to stdout instead of creating a flame graph; use with Brendan Gregg's flame graph perl script (see https://github.com/brendangregg/FlameGraph)"` | ||
Title string `long:"title" default:"Flame Graph" description:"Graph title to display in the output file"` | ||
Width int64 `long:"width" default:"1200" description:"Generated graph width"` | ||
Hash bool `long:"hash" description:"Colors are keyed by function name hash"` | ||
Colors string `long:"colors" default:"" description:"set color palette. choices are: hot (default), mem, io, wakeup, chain, java, js, perl, red, green, blue, aqua, yellow, purple, orange"` | ||
ConsistentPalette bool `long:"cp" description:"Use consistent palette (palette.map)"` | ||
Reverse bool `long:"reverse" description:"Generate stack-reversed flame graph"` | ||
Inverted bool `long:"inverted" description:"icicle graph"` | ||
} | ||
|
||
// main is the entry point of the application | ||
|
@@ -98,7 +105,8 @@ func runWithOptions(allOpts *options, remaining []string) error { | |
return nil | ||
} | ||
|
||
flameGraph, err := renderer.GenerateFlameGraph(flameInput, "--title", opts.Title) | ||
var flameGraphArgs = buildFlameGraphArgs(opts) | ||
flameGraph, err := renderer.GenerateFlameGraph(flameInput, flameGraphArgs...) | ||
if err != nil { | ||
return fmt.Errorf("could not generate flame graph: %v", err) | ||
} | ||
|
@@ -125,5 +133,62 @@ func validateOptions(opts *options) error { | |
if opts.PProfOptions.TimeSeconds < 1 { | ||
return fmt.Errorf("seconds must be an integer greater than 0") | ||
} | ||
|
||
// extra FlameGraph options | ||
if opts.OutputOpts.Title == "" { | ||
return fmt.Errorf("FlameGraph title should not be empty") | ||
} | ||
if opts.OutputOpts.Width < 1200 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this enforced by the flame graph script? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NO, my bad... Thanks for pointing out. |
||
return fmt.Errorf("FlameGraph miminal graph width is 1200 pixels") | ||
} | ||
if opts.OutputOpts.Colors != "" { | ||
var validColors = []string{"hot", "mem", "io", "wakeup", "chain", "java", "js", "perl", "red", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this becomes a lot simpler with a switch, and as a bonus, it's also faster. switch opts.OutputOpts.Colors {
case "hot", "mem", "io", [...]:
// valid
default:
// return error
} |
||
"green", "blue", "aqua", "yellow", "purple", "orange"} | ||
var isValidColors = false | ||
for _, color := range validColors { | ||
if opts.OutputOpts.Colors == color { | ||
isValidColors = true | ||
break | ||
} | ||
} | ||
if !isValidColors { | ||
return fmt.Errorf("FlameGraph unknown colors %q. Valid ones are: %s", opts.OutputOpts.Colors, strings.Join(validColors, ", ")) | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func buildFlameGraphArgs(opts outputOptions) []string { | ||
var args []string | ||
|
||
if opts.Title != "" { | ||
args = append(args, "--title", opts.Title) | ||
} | ||
|
||
if opts.Width > 0 { | ||
args = append(args, "--width", strconv.FormatInt(opts.Width, 10)) | ||
} | ||
|
||
if opts.Colors != "" { | ||
args = append(args, "--colors", opts.Colors) | ||
} | ||
|
||
if opts.Hash { | ||
args = append(args, "--hash") | ||
} | ||
|
||
if opts.ConsistentPalette { | ||
args = append(args, "--cp") | ||
} | ||
|
||
if opts.Reverse { | ||
args = append(args, "--reverse") | ||
} | ||
|
||
if opts.Inverted { | ||
args = append(args, "--inverted") | ||
} | ||
|
||
return args | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
nit: errors should be lowercase,
https://github.com/golang/go/wiki/CodeReviewComments#error-strings
same for all errors added
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.
OK my idea here was to use name of the utility called. I am going to change all occurrences to lower-case
flame graph
.