Skip to content

Commit

Permalink
feat(cmd): set default trend columns in CLI flag, cleanup static reso…
Browse files Browse the repository at this point in the history 
…lvers

Resolves: #1143 (comment)
  • Loading branch information
Ivan Mirić committed Oct 22, 2019
1 parent 371d0fa commit b91229a
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 43 deletions.
18 changes: 16 additions & 2 deletions cmd/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down
14 changes: 14 additions & 0 deletions cmd/config_consolidation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}},

This comment has been minimized.

Sign in to view

Copy link
@mstoykov

mstoykov Oct 23, 2019

Contributor

I think that setting it to "" should mean that no columns are shown ? I suppose this is how it worked previously, but still I think that is what it should mean ...
If you check for nil at b91229a#diff-31f9f805fb3e45c114bdaa74f9447759R329 should do the trick, I think ?

This comment has been minimized.

Sign in to view

Copy link
@imiric

imiric Oct 23, 2019

Contributor

I considered checking for nil, but that would actually be a regression as on current master we render all columns if --summary-trend-stats == "".

I'm not sure if it functionally makes sense for the user to disable trend columns as the output looks borked, take a look:

    data_received..............: 4.0 kB 5.4 kB/s
    data_sent..................: 1.2 kB 1.6 kB/s
    http_req_blocked...........:
    http_req_connecting........:
    http_req_duration..........:
    http_req_receiving.........:
    http_req_sending...........:
    http_req_tls_handshaking...:
    http_req_waiting...........:
    http_reqs..................: 3      4.010167/s
    iteration_duration.........:
    iterations.................: 1      1.336722/s
    my_trend...................:
    vus........................: 1      min=1 max=1
    vus_max....................: 1      min=1 max=1

So not sure if we should "fix" this here. Let me know.

Actually, there is still a regression related to this in the current PR: if K6_SUMMARY_TREND_STATS is specified and is empty (""), the additional env var validation/hack added to getConsolidatedConfig will fail, failing the entire test. Fixing it would also require something like:

	if len(conf.SummaryTrendStats) == 1 && conf.SummaryTrendStats[0] == "" {
		conf.SummaryTrendStats = lib.DefaultSummaryTrendStats
	}

:-/ Let me know what would be the preferred approach here.

This comment has been minimized.

Sign in to view

Copy link
@mstoykov

mstoykov Oct 23, 2019

Contributor

I still think that the principle of least astonishment will say that if you set the list of trend stats you want to see to an empty string should mean that ... no trend stats are shown :).
I agree that is ... not very useful, but it's what I would expect and IMO everybody else would expect the same thing :)

This comment has been minimized.

Sign in to view

Copy link
@imiric

imiric Oct 23, 2019 •
edited
Loading

Contributor

I agree with you and the principle, I was just concered about changing existing functionality, but as you say, previously it could've been considered a bug. And this likely isn't used anyway.

So I pushed the fix in 333ce28.

It's also a bit cleaner since it avoids having to call ui.ValidateSummary() again, and we're just stuck with a workaround we can remove once we update to envconfig 1.4.0.

No, we actually need that additional validation anyway... :-/
{
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...
}
Expand Down
27 changes: 17 additions & 10 deletions cmd/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions lib/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 0 additions & 1 deletion lib/options_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,6 @@ func TestOptionsEnv(t *testing.T) {
}

func TestTagSetTextUnmarshal(t *testing.T) {

var testMatrix = map[string]map[string]bool{
"": {},
"test": {"test": true},
Expand Down
41 changes: 15 additions & 26 deletions ui/summary.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

This comment has been minimized.

Sign in to view

Copy link
@mstoykov

mstoykov Oct 23, 2019

Contributor

Can you move this to above the var
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
Expand All @@ -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
Expand All @@ -64,40 +72,23 @@ 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
}

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 {
Expand All @@ -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
}

Expand Down

0 comments on commit b91229a

Please sign in to comment.