Flamegraph options #50
Conversation
| 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"` | ||
| ColorPalette bool `long:"cp" description:"Use consistent palette (palette.map)"` |
There was a problem hiding this comment.
this should be ConsistentPalette instead of ColorPalette
| } | ||
| } | ||
| if !isValidColors { | ||
| return fmt.Errorf("FlameGraph unknown colors '%s'", opts.OutputOpts.Colors) |
There was a problem hiding this comment.
this error message could include the list of valid options
|
|
||
| --width= Generated graph width (default: 1200) | ||
| --hash Colors are keyed by function name hash | ||
| --colors= set color palette. choices are: hot (default), mem, io, wakeup, chain, java, |
There was a problem hiding this comment.
Set color palette. Valid choices are:
| return fmt.Errorf("FlameGraph title should not be empty") | ||
| } | ||
| if opts.OutputOpts.Width < 1200 { | ||
| return fmt.Errorf("FlameGraph miminal graph with is 1 200 pixels") |
There was a problem hiding this comment.
typos:
1200, not1 200width, notwith
| 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"` |
There was a problem hiding this comment.
this option would make more sense to me as ColorPalette or Palette instead of Colors.
it's awkward to use singular blue, red, etc. with the plural colors. the singular color doesn't really make sense because "wakeup" isn't a color.
that said, since the upstream vendor (FlameGraph) uses this as an argument, i could be convinced that matching the arguments is more important.
There was a problem hiding this comment.
Agree. I think that consistency is way to go even that there is a better way/name etc.
| } | ||
| } | ||
| if !isValidColors { | ||
| return fmt.Errorf("FlameGraph unknown colors '%s'", opts.OutputOpts.Colors) |
There was a problem hiding this comment.
use %q instead of %s to quote user input
| js, perl, red, green, blue, aqua, yellow, purple, orange | ||
| --hash Graph colors are keyed by function name hash | ||
| --cp Graph use consistent palette (palette.map) | ||
| --inverted Graph icicle graph |
There was a problem hiding this comment.
reword to avoid "graph" appearing twice. e.g. Generate an icicle graph
|
|
||
| // extra FlameGraph options | ||
| if opts.OutputOpts.Title == "" { | ||
| return fmt.Errorf("FlameGraph title should not be empty") |
There was a problem hiding this comment.
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.
OK my idea here was to use name of the utility called. I am going to change all occurrences to lower-case flame graph.
| 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.
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
}| 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.
is this enforced by the flame graph script?
There was a problem hiding this comment.
NO, my bad... Thanks for pointing out.
Add few additional CLI argument to call FlameGraph tool.