Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse and validate threshold during init #2356

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ An archive is a fully self-contained test run, and can be executed identically e
return err
}

_, err = deriveAndValidateConfig(conf, r.IsExecutable, logger)
_, err = deriveAndValidateConfig(conf, registry, r.IsExecutable, logger)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
return err
}

derivedConf, err := deriveAndValidateConfig(conf, r.IsExecutable, logger)
derivedConf, err := deriveAndValidateConfig(conf, registry, r.IsExecutable, logger)
if err != nil {
return err
}
Expand Down
80 changes: 77 additions & 3 deletions cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib"
"go.k6.io/k6/lib/executor"
"go.k6.io/k6/lib/metrics"
"go.k6.io/k6/lib/types"
"go.k6.io/k6/stats"
)
Expand Down Expand Up @@ -235,17 +236,21 @@ func applyDefault(conf Config) Config {
}

func deriveAndValidateConfig(
conf Config, isExecutable func(string) bool, logger logrus.FieldLogger,
conf Config,
registry *metrics.Registry,
isExecutable func(string) bool,
logger logrus.FieldLogger,
) (result Config, err error) {
result = conf
result.Options, err = executor.DeriveScenariosFromShortcuts(conf.Options, logger)
if err == nil {
err = validateConfig(result, isExecutable)
err = validateConfig(result, registry, isExecutable)
}

return result, errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
}

func validateConfig(conf Config, isExecutable func(string) bool) error {
func validateConfig(conf Config, registry *metrics.Registry, isExecutable func(string) bool) error {
errList := conf.Validate()

for _, ec := range conf.Scenarios {
Expand All @@ -254,9 +259,78 @@ func validateConfig(conf Config, isExecutable func(string) bool) error {
}
}

// If there are thresholds to validate, the registry paramater is not allowed to be nil.
// Note that the reason for passing it as a pointer in the first place is
// because it holds a Mutex, which effectively forbids passing it by value.
if conf.Thresholds != nil && len(conf.Thresholds) > 0 && registry == nil {
err := fmt.Errorf(
"unable to validate thresholds configuration; " +
"reason: provided registry is nil",
)
errList = append(errList, err)
return consolidateErrorMessage(errList, "there were problems while validating the specified script configuration: ")
}
Comment on lines +262 to +272
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this seems like defensive programming to me - there is no case where registry should be nil to begin with IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is defensive programming; however, I would like to keep it that way. I'm happy to discuss how we could improve this, and it's probably my C programming habits kicking in, but the pointer should be checked. It's defensive in the sense that I don't want to risk a potential segfault, or security issue to go through to production because my future self did something stupid and somehow ended up passing nil in there :)


for thresholdName, thresholds := range conf.Thresholds {
// Fetch metric matching the threshold's name
metric, ok := registry.Get(thresholdName)
if !ok {
Comment on lines +274 to +277
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also break this example even though in practice all the thresholds are valid, they just should be evaluated differently from how they are currently.

The problem here is that thresholdName includes the tags while we can only check the name of the treshold not the name+ the tags. This is the error:

ERRO[0000] invalid threshold defined on http_reqs{key:""}; reason: no metric named http_reqs{key:""} found

p.s. The thresholds also are evaluated wrongly with this PR as well - a tag with empty value in threshold is always "matched" even if it doesn't have the tag at all

Copy link
Member Author

@oleiade oleiade Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing the issue out. Would you have pointers or a proposal to make this work, or improve it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 I guess something a kin to

k6/core/engine.go

Lines 106 to 114 in 737dfa7

e.submetrics = make(map[string][]*stats.Submetric)
for name := range e.thresholds {
if !strings.Contains(name, "{") {
continue
}
parent, sm := stats.NewSubmetric(name)
e.submetrics[parent] = append(e.submetrics[parent], sm)
}

// The defined threshold applies to a non-existing metrics
err := fmt.Errorf("invalid threshold defined on %s; reason: no metric named %s found", thresholdName, thresholdName)
return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the below) should just add to the list so we get all errors in one go

}

// Validate the threshold definition against its matching
// metric.
err := validateThresholdsConfig(thresholdName, thresholds, metric)
if err != nil {
return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
}
}

return consolidateErrorMessage(errList, "There were problems with the specified script configuration:")
}

// validateThresholdsConfig validates a threshold definition is consistent with the metric it applies to.
// Given a threshold name, expressions and a metric to apply the expressions too, validateThresholdConfig will
// assert that each expression uses an aggregation method that's supported by the provided metric. It returns
// an error otherwise. Note that this function expects the passed in thresholds to have been parsed already, and
// have their Parsed (ThresholdExpression) field already filled.
func validateThresholdsConfig(thresholdName string, expressions stats.Thresholds, metric *stats.Metric) error {
var supportedMethods []string

switch metric.Type {
case stats.Counter:
supportedMethods = []string{stats.TokenCount, stats.TokenRate}
case stats.Gauge:
supportedMethods = []string{stats.TokenValue}
case stats.Rate:
supportedMethods = []string{stats.TokenRate}
case stats.Trend:
supportedMethods = []string{
stats.TokenAvg,
stats.TokenMin,
stats.TokenMax,
stats.TokenMed,
stats.TokenPercentile,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really check if the percentile is valid

Copy link
Member Author

@oleiade oleiade Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncertain if I understand why, could you elaborate?

Some context: in the current state of the PR, one could argue the validation is done in two phases: parsing, which happens during bundling as of this PR's state (expression format validation), and actual validation which happens in config validation: "does the metric to you apply that expression to supports the operation?".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you are correct, I probably commented on this before I made the general comment that currently parsing of a threshold will still fail the test.

}
}

for _, expression := range expressions.Thresholds {
if !lib.Contains(supportedMethods, expression.Parsed.AggregationMethod) {
return fmt.Errorf(
"invalid threshold expression %s: '%s'; "+
"reason: invalid aggregation method '%s' applied to the '%s' metric. "+
"%s is a metric of type %s, did you mean to use the any of the "+
"'count' or 'rate' aggregation methods instead?",
thresholdName, expression.Source, expression.Parsed.AggregationMethod, thresholdName, thresholdName, metric.Type,
)
}
}

return nil
}

func consolidateErrorMessage(errList []error, title string) error {
if len(errList) == 0 {
return nil
Expand Down
Loading