Parse and validate thresholds before starting the execution

[oleiade]

Problem statement

We would like to parse and validate threshold expressions at the end of the init context's evaluation, in order to have invalid thresholds make K6 fail early.

Context

As of #2251 (addressing #1443), thresholds are parsed and executed in Go, as opposed to prior to that, when the operation was done in JS. Still, threshold evaluation happens during the running phase of K6. During the review of #2251, @na-- mentioned that as a follow-up, we probably should move the parsing and validation of thresholds right after the initial evaluation of the init context. That way, if thresholds expression don't parse, or parse into a case we know to be an error (see Scope), we may stop right at the end of the initialization phase and have K6 fail early.

As of today, parsing a threshold expression is done using the stats.parseThresholdExpression function. Parsing is made in stats.newThreshold. Executing the threshold, and comparing using it against the actual numbers we've collected (sinks) is done in stats.Threshold.runNoTaint.

Scope

The goal would then be for our changes to halt the execution of K6, right after the evaluation of the init context, in the following scenarios.

Invalid threshold expression

export const options = {
    vus: 5,
    iterations: 30,
    thresholds: {
         http_req_failed: ['rave<0.01'],  // "rave" is not a valid aggregation method
    },
};

Invalid aggregation method for metric

export const options = {
    vus: 5,
    iterations: 30,
    thresholds: {
        my_counter: ['p(95)<200'], // error, Counters do not have p()
    },
};

Threshold on non-existing metric

export const options = {
    vus: 5,
    iterations: 30,
    thresholds: {
        iDoNotExist: ['p(95)<200'], // error, the metric it is applied to doesn't exist
    },
};

Requirements

• When an invalid threshold is found, k6 should exit with the code 104 InvalidConfig

Proposed change

Although it's still unclear to me at this point exactly what the concrete solution should look like, the rationale would be to find the right place in the evaluation of the init context (somewhere where the metrics registry is available), to use stats.parseThresholdExpression.

Open questions

While we believe this change's scope shouldn't be very big, and don't expect complications, the following questions remain were raised or remain open until implementation:

• @mstoykov mentioned that certain metrics might not be registered or have data points until we actually started the execution (Don't hesitate to correct me, if I misunderstood).
• • to be discussed and decided: we shouldn't parse the thresholds at all when the --no-thresholds option is passed to k6

Connected issue

• @na-- mentioned that resolving this issue could lead (or be done at the same time as) to solving Metrics for which no data was recorded are not displayed or evaluated for Thresholds #1346
• It might also facilitate Remove the Format method of stats.Sink #2320
• It happens under the umbrella of Remove the JS runtime from threshold calculations #1443

Edits

• 14th of January 2022:
  • Added requirements section
  • Added "threshold over non-existing metric" to scope, following up on @na--'s comment
  • Added question about the --no-thresholds option

[na--]

This may be implicitly understood but probably still worth pointing out as a part of the scope - thresholds on metrics that don't exist should also be invalid. We've seen multiple cases where a typo in a metric or threshold name silently made someone's script useless, since its threshold would never trigger and signal a discrepancy in the SUT.

[oleiade]

Edits made to the description (+added an edits section):

• 14th of January 2022:
  • Added requirements section
  • Added "threshold over non-existing metric" to scope, following up on @na--'s comment
  • Added question about the --no-thresholds option.

[na--]

I realized (or, rather, remembered) that there is a slight problem which will complicate the implementation of this feature... 😞

With plain .js test scripts, we will run the init context once to get the exported JS options, which will also allow us to gather all of the defined custom metrics and their types in the registry after this code runs:

k6/cmd/run.go

Lines 113 to 115 in 13b2372

	registry := metrics.NewRegistry()
	builtinMetrics := metrics.RegisterBuiltinMetrics(registry)
	initRunner, err := newRunner(logger, src, runType, filesystems, runtimeOptions, builtinMetrics, registry)

However, with .tar archive bundles, we are not actually executing the init context even once, we're getting the Options directly from the archive's metadata.json file. So:

1. We probably need to add the custom metrics and their types and options to the metadata.json when k6 creates new archives. This should be relatively easy to do (:crossed_fingers:), a new entry (e.g. metrics, customMetrics or something like that) at the same level as options, k6version, etc. in that file will be backwards compatible and not cause any issues.
2. Newer k6 versions should be able to execute .tar archive bundles generated by older k6 versions that lack the metrics property. So the threshold validation will have to be more complicated. For example, it can consist of 2 parts:
   • the generic syntax parser error checks that can work even without the metric type information and should probably always be performed
   • the type specific checks (e.g. Counters do not have p()) should only be performed on built-in metrics or when we know the type of a custom metric (because we're running a .js script and have executed the init context or we're running a new archive)

Or maybe some other solution? Maybe emit warnings if we have thresholds on unknown metrics, instead of errors? 🤔 🤷‍♂️ Not sure what the best and most user-friendly way to tackle this issue is, just wanted to explicitly mention it so we design and implement the solution with it in mind.

[mstoykov]

However, with .tar archive bundles, we are not actually executing the init context even once, we're getting the Options directly from the archive's metadata.json file. So:

Actually it does since #1040 - I also checked it manually ;)

[na--]

That doesn't necessarily solve the issue, given that we do some of the validation early on in the cloud process and based solely on the metadata.json from the archive, to return nice human-readable error messages to users before we've allocated instances for the test and cost them money...