From b91229aba4c429d8361561513e37f142f7aae18a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Tue, 22 Oct 2019 17:15:12 +0200 Subject: [PATCH] feat(cmd): set default trend columns in CLI flag, cleanup static resolvers Resolves: https://github.com/loadimpact/k6/pull/1143#discussion_r337355756 --- cmd/config.go | 18 ++++++++++++-- cmd/config_consolidation_test.go | 14 +++++++++++ cmd/options.go | 27 +++++++++++++-------- lib/options.go | 10 ++++---- lib/options_test.go | 1 - ui/summary.go | 41 ++++++++++++-------------------- 6 files changed, 68 insertions(+), 43 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index 366e87c86fe..351c9c9b89f 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -43,6 +43,7 @@ import ( "github.com/loadimpact/k6/stats/influxdb" "github.com/loadimpact/k6/stats/kafka" "github.com/loadimpact/k6/stats/statsd/common" + "github.com/loadimpact/k6/ui" ) // configFlagSet returns a FlagSet with the default run configuration flags. @@ -304,17 +305,30 @@ func getConsolidatedConfig(fs afero.Fs, cliConf Config, runner lib.Runner) (conf conf = conf.Apply(envConf).Apply(cliConf) conf = applyDefault(conf) + // TODO(imiric): Move this validation where it makes sense in the configuration + // refactor of #883. Currently we validate CLI flags in cmd.getOptions. Yet + // there is no place where configuration is validated after applying all other + // sources, like environment variables (besides inline in the runCmd itself...). + // So this repeats the trend stats validation in case other sources overrode our + // default value. + if err = ui.ValidateSummary(conf.SummaryTrendStats); err != nil { + return conf, err + } + return conf, nil } -// applyDefault applys default options value if it is not specified by any mechenisms. This happens with types -// which does not support by "gopkg.in/guregu/null.v3". +// applyDefault applies the default options value if it is not specified. +// This happens with types which are not supported by "gopkg.in/guregu/null.v3". // // Note that if you add option default value here, also add it in command line argument help text. func applyDefault(conf Config) Config { if conf.Options.SystemTags == nil { conf = conf.Apply(Config{Options: lib.Options{SystemTags: lib.GetTagSet(lib.DefaultSystemTagList...)}}) } + if len(conf.Options.SummaryTrendStats) == 0 { + conf = conf.Apply(Config{Options: lib.Options{SummaryTrendStats: lib.DefaultSummaryTrendStats}}) + } return conf } diff --git a/cmd/config_consolidation_test.go b/cmd/config_consolidation_test.go index 74f206ba9e3..8b3dc6efbf1 100644 --- a/cmd/config_consolidation_test.go +++ b/cmd/config_consolidation_test.go @@ -405,6 +405,20 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase { assert.Equal(t, lib.GetTagSet("proto", "url"), c.Options.SystemTags) }, }, + // Test summary trend stats + {opts{}, exp{}, func(t *testing.T, c Config) { + assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) + }}, + {opts{cli: []string{"--summary-trend-stats", `""`}}, exp{}, func(t *testing.T, c Config) { + assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) + }}, + { + opts{runner: &lib.Options{SummaryTrendStats: []string{"avg", "p(90)", "count"}}}, + exp{}, + func(t *testing.T, c Config) { + assert.Equal(t, []string{"avg", "p(90)", "count"}, c.Options.SummaryTrendStats) + }, + }, //TODO: test for differences between flagsets //TODO: more tests in general, especially ones not related to execution parameters... } diff --git a/cmd/options.go b/cmd/options.go index 0f184942887..5189009af88 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -63,7 +63,14 @@ func optionFlagSet() *pflag.FlagSet { flags.Duration("min-iteration-duration", 0, "minimum amount of time k6 will take executing a single iteration") 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), one or more as 'avg,p(95),...'") + + // The comment about system-tags also applies for summary-trend-stats. The default values + // are set in applyDefault(). + sumTrendStatsHelp := fmt.Sprintf( + "define `stats` for trend metrics (response times), one or more as 'avg,p(95),...' (default '%s')", + strings.Join(lib.DefaultSummaryTrendStats, ","), + ) + flags.StringSlice("summary-trend-stats", nil, sumTrendStatsHelp) flags.String("summary-time-unit", "", "define the time unit used to display the trend stats. Possible units are: 's', 'ms' and 'us'") // system-tags must have a default value, but we can't specify it here, otherwiese, it will always override others. // set it to nil here, and add the default in applyDefault() instead. @@ -143,17 +150,17 @@ func getOptions(flags *pflag.FlagSet) (lib.Options, error) { opts.BlacklistIPs = append(opts.BlacklistIPs, net) } - trendStatStrings, err := flags.GetStringSlice("summary-trend-stats") - if err != nil { - return opts, err - } - - if summaryErr := ui.ValidateSummary(trendStatStrings); summaryErr != nil { - return opts, summaryErr + if flags.Changed("summary-trend-stats") { + trendStats, errSts := flags.GetStringSlice("summary-trend-stats") + if errSts != nil { + return opts, errSts + } + if errSts = ui.ValidateSummary(trendStats); err != nil { + return opts, errSts + } + opts.SummaryTrendStats = trendStats } - opts.SummaryTrendStats = trendStatStrings - summaryTimeUnit, err := flags.GetString("summary-time-unit") if err != nil { return opts, err diff --git a/lib/options.go b/lib/options.go index 5537e389bc8..f8c7bb6d9e4 100644 --- a/lib/options.go +++ b/lib/options.go @@ -43,10 +43,12 @@ const DefaultSchedulerName = "default" // DefaultSystemTagList includes all of the system tags emitted with metrics by default. // Other tags that are not enabled by default include: iter, vu, ocsp_status, ip -var DefaultSystemTagList = []string{ - - "proto", "subproto", "status", "method", "url", "name", "group", "check", "error", "error_code", "tls_version", -} +//nolint: gochecknoglobals +var ( + DefaultSystemTagList = []string{"proto", "subproto", "status", "method", "url", "name", + "group", "check", "error", "error_code", "tls_version"} + DefaultSummaryTrendStats = []string{"avg", "min", "med", "max", "p(90)", "p(95)"} +) // TagSet is a string to bool map (for lookup efficiency) that is used to keep track // which system tags should be included with with metrics. diff --git a/lib/options_test.go b/lib/options_test.go index 0f601ad9ef4..dddfaa05a1e 100644 --- a/lib/options_test.go +++ b/lib/options_test.go @@ -495,7 +495,6 @@ func TestOptionsEnv(t *testing.T) { } func TestTagSetTextUnmarshal(t *testing.T) { - var testMatrix = map[string]map[string]bool{ "": {}, "test": {"test": true}, diff --git a/ui/summary.go b/ui/summary.go index 7e7830160f3..39ad573b739 100644 --- a/ui/summary.go +++ b/ui/summary.go @@ -46,6 +46,14 @@ var ( errStatEmptyString = errors.New("invalid stat, empty string") errStatUnknownFormat = errors.New("invalid stat, unknown format") errPercentileStatInvalidValue = errors.New("invalid percentile stat value, accepts a number") + //nolint: gochecknoglobals + staticResolvers = map[string]func(s *stats.TrendSink) interface{}{ + "avg": func(s *stats.TrendSink) interface{} { return s.Avg }, + "min": func(s *stats.TrendSink) interface{} { return s.Min }, + "med": func(s *stats.TrendSink) interface{} { return s.Med }, + "max": func(s *stats.TrendSink) interface{} { return s.Max }, + "count": func(s *stats.TrendSink) interface{} { return s.Count }, + } ) // ErrInvalidStat represents an invalid trend column stat @@ -55,7 +63,7 @@ type ErrInvalidStat struct { } func (e ErrInvalidStat) Error() string { - return errors.Wrapf(e.err, "stat '%s'", e.name).Error() + return errors.Wrapf(e.err, "'%s'", e.name).Error() } // Summary handles test summary output @@ -64,29 +72,12 @@ type Summary struct { trendValueResolvers map[string]func(s *stats.TrendSink) interface{} } -func getDefaultResolvers() map[string]func(s *stats.TrendSink) interface{} { - return map[string]func(s *stats.TrendSink) interface{}{ - "avg": func(s *stats.TrendSink) interface{} { return s.Avg }, - "min": func(s *stats.TrendSink) interface{} { return s.Min }, - "med": func(s *stats.TrendSink) interface{} { return s.Med }, - "max": func(s *stats.TrendSink) interface{} { return s.Max }, - "p(90)": func(s *stats.TrendSink) interface{} { return s.P(0.90) }, - "p(95)": func(s *stats.TrendSink) interface{} { return s.P(0.95) }, - "count": func(s *stats.TrendSink) interface{} { return s.Count }, - } -} - // NewSummary returns a new Summary instance, used for writing a // summary/report of the test metrics data. -func NewSummary(sts []string) *Summary { - tc := []string{"avg", "min", "med", "max", "p(90)", "p(95)"} - if len(sts) > 0 { - tc = sts - } +func NewSummary(cols []string) *Summary { + s := Summary{trendColumns: cols, trendValueResolvers: staticResolvers} - s := Summary{trendColumns: tc, trendValueResolvers: getDefaultResolvers()} - - customResolvers := s.generateCustomTrendValueResolvers(sts) + customResolvers := s.generateCustomTrendValueResolvers(cols) for name, res := range customResolvers { s.trendValueResolvers[name] = res } @@ -94,10 +85,10 @@ func NewSummary(sts []string) *Summary { return &s } -func (s *Summary) generateCustomTrendValueResolvers(sts []string) map[string]func(s *stats.TrendSink) interface{} { +func (s *Summary) generateCustomTrendValueResolvers(cols []string) map[string]func(s *stats.TrendSink) interface{} { resolvers := make(map[string]func(s *stats.TrendSink) interface{}) - for _, stat := range sts { + for _, stat := range cols { if _, exists := s.trendValueResolvers[stat]; !exists { percentile, err := validatePercentile(stat) if err == nil { @@ -112,14 +103,12 @@ func (s *Summary) generateCustomTrendValueResolvers(sts []string) map[string]fun // ValidateSummary checks if passed trend columns are valid for use in // the summary output. func ValidateSummary(trendColumns []string) error { - resolvers := getDefaultResolvers() - for _, stat := range trendColumns { if stat == "" { return ErrInvalidStat{stat, errStatEmptyString} } - if _, exists := resolvers[stat]; exists { + if _, exists := staticResolvers[stat]; exists { continue }