CLI option to specify what stats to report for trend metrics #462
Conversation
Codecov Report
@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 62.05% 62.13% +0.08%
==========================================
Files 93 93
Lines 6580 6619 +39
==========================================
+ Hits 4083 4113 +30
- Misses 2271 2278 +7
- Partials 226 228 +2
Continue to review full report at Codecov.
|
|
Great stuff @antekresic! Will review this tomorrow morning. |
There was a problem hiding this comment.
@antekresic Looks good, just some minor UX things I think we should improve.
cmd/options.go
Outdated
| @@ -46,6 +47,7 @@ func optionFlagSet() *pflag.FlagSet { | |||
| flags.Bool("no-connection-reuse", false, "don't reuse connections between iterations") | |||
| flags.BoolP("throw", "w", false, "throw warnings (like failed http requests) as errors") | |||
| flags.StringSlice("blacklist-ip", nil, "blacklist an `ip range` from being called") | |||
| flags.StringSlice("summary-trend-stats", nil, "define `stats` for trend metrics (response times)") | |||
There was a problem hiding this comment.
I think we should give an example of the format for the option. Something like: define 'stats' for trend metrics (response times), one or more as 'avg,p(95),...'.
ui/summary.go
Outdated
| @@ -55,6 +55,62 @@ type TrendColumn struct { | |||
| Get func(s *stats.TrendSink) float64 | |||
| } | |||
|
|
|||
| // VerifyTrendColumnStat checks if stat is a valid trend column | |||
| func VerifyTrendColumnStat(stat string) bool { | |||
There was a problem hiding this comment.
I think VerifyTrendColumnStat should return bool, error (or just error perhaps?) and then in cmd/options.go when looping through the stats we print any errors returned ("stat 1 (''): empty string", "stat 2 ('p("SomeString")'): percentile stat accepts a number not 'SomeString'" etc).
ui/summary.go
Outdated
| } | ||
| } | ||
|
|
||
| func generatePercentileTrendColumn(stat string) func(s *stats.TrendSink) float64 { |
There was a problem hiding this comment.
Same as for VerifyTrendColumnStat, I think we should return func(s *stats.TrendSink) float64, error with an error messages that helps the user understand what they did wrong, things like:
- "stat 1 (''): empty string"
- "stat 2 ('p("SomeString")'): percentile stat accepts a number not 'SomeString'" etc).
cmd/options.go
Outdated
| return opts, err | ||
| } | ||
| for _, s := range trendStatStrings { | ||
| if ui.VerifyTrendColumnStat(s) { |
There was a problem hiding this comment.
See other comments on printing stats parsing errors here for the user to see.
|
@robingustafsson thanks for the feedback! Updated the code to match the comments. |
|
LGTM, merging. Thanks @antekresic! |
Add CLI option to specify what stats to report for trend metrics to stdout summary
Closes #450 & #408