Parse and validate threshold during init #2356

oleiade · 2022-01-27T13:20:03Z

What this PR does

This Pull Request addresses the scope described in #2330. Namely, it reintroduces the changes from #1443/#2251, and builds upon them to ensure that thresholds are parsed and validated before the execution starts.

Scope

What it does

Following this Pull Request:

k6 parses thresholds during bundling. Parsing makes sure that the thresholds defined in the options make sense from a syntactic point of view.
k6 validates thresholds during configuration validation. There, it makes sure that thresholds apply to existing metrics, and that the aggregation method they hold is supported by the metric they apply to.
If any of the conditions described above were not met, k6 would return immediately with an exit code 104 - Invalid Config.

What it does not

It was discussed in the context of the #2330 issue that k6 should also make sure to handle metrics properly when in an archiving/cloud execution context. Some changes were proposed to how k6 handles the metadata.json file.
In order to keep this Pull Request reviewable and avoid scope creep, I decided to address that issue separately. If you have arguments towards doing otherwise: shoot 🚀.

Notable changes

The most notable change I can think of is that deriveAndValidateConfig now takes a metrics.Registry as input. Another important one is that threshold parsing code now declares an explicit sentinel error type ErrThresholdParsing which the caller code depends on in order to decide which exit code it should use.

Review guide

This PR reverts the revert of the Fix/1443 remove the JS runtime from threshold calculations #2251 merge (that's a mouthful). Thus, please ignore the first commit of this branch, as it was already reviewed and accepted as part of the initial PR.
🔥 I would also recommend using the commits view of GitHub, as opposed to Files Changed, in order to be able to avoid the overhead of reviewing threshold parsing code again 😉
I did my best to arrange and order the commits logically. I've also included extensive commits messages, which should help you through them:
1. First: The very first brings back Go-based thresholds parsing.
2. Second: Some changes to the public API of thresholds parsing has been made so that certain symbols could be accessed from outside the stats package.
3. Third: Contains the code implementing thresholds validation against metrics.
4. Fourth: Integrates thresholds validation in the affected commands.

Dependency

ref #2330
ref #1443
ref #2251

Please let me know if any more information is needed on your side, I'll happily add it 🙇🏻

cmd/config.go

oleiade · 2022-01-27T16:23:26Z

CI has been useful once again, and outlined a number of issues (probably caused by a failed rebase). I really need to sort my git hooks out to catch those before. I'll make the branch green again and ping you reviewers once it's fixed and ready for review again. Sorry for the noise 🙇🏻

oleiade · 2022-01-31T12:33:41Z

@na-- @olegbespalov @mstoykov @codebien Alrighty, this is green again 🟢 and ready to review 🦖

olegbespalov

In general, PR looks good to me, I left one small comment.

Special thanks for changing the short variable names to the more readable ❤️

stats/thresholds.go

…from_threshold_calcultations Fix/1443 remove the JS runtime from threshold calculations

This commit makes some minor modifications to the `stats` package API. Namely, it makes `stats.ThresholdExpression` and `stats.token*` symbols public. It also makes `stats.Threshold.parsed` public. These changes are made in order to facilitate validation of thresholds from outside the `stats` package. Having access to both the parsed Threshold, and the aggregation methods symbols will allow comparing them and asserting their meaningfulness in a context where we have typed metrics available. ref #2330

This commit adds a `validateThresholdConfig` function to `cmd/config`, and integrates it as part of the `validateConfig` operations. From now on, `validateConfig` takes a `metrics.Registry` as input, and validates that thresholds defined in the config apply to existing metrics, and use methods that are valid for the metric they apply to. As a side effect, this commit adds a `Get` method to `metrics.Registry` in order to be able to query registered metrics, regardless of whether they are custom or builtin metrics. As another side effect, this commit introduces a `lib.Contains` helper function allowing to check if a slice of strings contains a given string. This is used to simplify the matching of supported aggregation methods on metrics in the `validateThresholdConfig` function. ref #2330

This commit makes sure that the threshold configuration (as passed in the script exported options for instance) is valid, before starting the execution. A valid threshold must pass the following assertion: - Its expression is syntaxically correct and is parsable - It applies to a metrics that's known to k6, either builtin or custom - Its expression's aggregation method is valid for the metric it applies to Threshold validation will be made in the context of the `run`, `cloud`, `archive`, `inspect`, and `archive` commands. If a threshold definition is invalid, the k6 program will exit with a status code of 104. ref #2330

olegbespalov

👍

mstoykov

As far as I can see (and test) you are still parsing the thresholds way too early so if you have a threshold that is just not with valid syntax to begin with it will abort even earlier (and irregardless of --no-thresholds). this is waht bea617a did very ... bluntly basically and I think somethign like that is required even now.

I see that in the issue this is listed as "undecided" but I have some memory that this is more or less required to merge the whole PR (especially for the cloud use case)

WDYT @na--

mstoykov · 2022-02-09T10:03:31Z

cmd/config.go

+	// 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: ")
+	}


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

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 :)

mstoykov · 2022-02-09T10:04:47Z

cmd/config.go

+		if !ok {
+			// 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)


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

mstoykov · 2022-02-09T10:06:23Z

cmd/config.go

+			stats.TokenMin,
+			stats.TokenMax,
+			stats.TokenMed,
+			stats.TokenPercentile,


this doesn't really check if the percentile is valid

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?".

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.

mstoykov · 2022-02-16T16:25:38Z

cmd/config.go

+	for thresholdName, thresholds := range conf.Thresholds {
+		// Fetch metric matching the threshold's name
+		metric, ok := registry.Get(thresholdName)
+		if !ok {


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

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

🤷 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)

}

oleiade · 2022-02-18T09:49:25Z

Thanks for the review @mstoykov 🙇🏻 I didn't know about bea617a, it would have been helpful to have a heads-up on this earlier. Those changes are great, if you have no objections, I would integrate them in this PR.

mstoykov · 2022-02-18T11:23:48Z

@oleiade, sorry 🙇 . This was shared internally around the time we did revert the previous PR. I though you have seen it, but apperantly not ;( .

oleiade · 2022-03-25T11:08:39Z

The scope and implementation has evolved quite a bit, to the point where it was more convinient to start over. Closing in favor of #2463

github-actions bot requested review from na-- and olegbespalov January 27, 2022 13:20

oleiade commented Jan 27, 2022

View reviewed changes

cmd/config.go Outdated Show resolved Hide resolved

na-- requested review from mstoykov and codebien January 27, 2022 13:37

oleiade force-pushed the fix/2330_parse_and_validate_threshold_during_init branch 3 times, most recently from c7777e5 to db73489 Compare January 27, 2022 15:01

mstoykov added this to the v0.37.0 milestone Jan 28, 2022

oleiade force-pushed the fix/2330_parse_and_validate_threshold_during_init branch 3 times, most recently from 2a6632f to 2a91012 Compare January 31, 2022 10:45

olegbespalov reviewed Feb 2, 2022

View reviewed changes

stats/thresholds.go Outdated Show resolved Hide resolved

oleiade force-pushed the fix/2330_parse_and_validate_threshold_during_init branch from 2aeebfc to 3aee96d Compare February 3, 2022 15:43

oleiade and others added 4 commits February 3, 2022 16:45

Merge pull request #2251 from grafana/fix/1443_remove_the_js_runtime_…

c033685

…from_threshold_calcultations Fix/1443 remove the JS runtime from threshold calculations

oleiade force-pushed the fix/2330_parse_and_validate_threshold_during_init branch from 3aee96d to c510664 Compare February 3, 2022 15:45

olegbespalov approved these changes Feb 8, 2022

View reviewed changes

mstoykov requested changes Feb 9, 2022

View reviewed changes

This was referenced Feb 10, 2022

"sliding window" thresholds #2379

Open

Custom metric threshold calculation using wrong statistics #2390

Closed

mstoykov reviewed Feb 16, 2022

View reviewed changes

na-- modified the milestones: v0.37.0, v0.38.0 Mar 1, 2022

sniku assigned oleiade Mar 9, 2022

oleiade mentioned this pull request Mar 10, 2022

Move the stats package content to metrics package #2433

Merged

oleiade closed this Mar 25, 2022

oleiade deleted the fix/2330_parse_and_validate_threshold_during_init branch March 25, 2022 11:08

oleiade restored the fix/2330_parse_and_validate_threshold_during_init branch March 25, 2022 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse and validate threshold during init #2356

Parse and validate threshold during init #2356

oleiade commented Jan 27, 2022 •

edited

Loading

oleiade commented Jan 27, 2022

oleiade commented Jan 31, 2022

olegbespalov left a comment

olegbespalov left a comment

mstoykov left a comment

mstoykov Feb 9, 2022

oleiade Feb 18, 2022

mstoykov Feb 9, 2022

mstoykov Feb 9, 2022

oleiade Feb 18, 2022 •

edited

Loading

mstoykov Feb 18, 2022

mstoykov Feb 16, 2022

oleiade Feb 18, 2022 •

edited

Loading

mstoykov Feb 18, 2022

oleiade commented Feb 18, 2022

mstoykov commented Feb 18, 2022

oleiade commented Mar 25, 2022

	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)
	}

Parse and validate threshold during init #2356

Parse and validate threshold during init #2356

Conversation

oleiade commented Jan 27, 2022 • edited Loading

What this PR does

Scope

What it does

What it does not

Notable changes

Review guide

Dependency

oleiade commented Jan 27, 2022

oleiade commented Jan 31, 2022

olegbespalov left a comment

Choose a reason for hiding this comment

olegbespalov left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Feb 9, 2022

Choose a reason for hiding this comment

oleiade Feb 18, 2022

Choose a reason for hiding this comment

mstoykov Feb 9, 2022

Choose a reason for hiding this comment

mstoykov Feb 9, 2022

Choose a reason for hiding this comment

oleiade Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

mstoykov Feb 18, 2022

Choose a reason for hiding this comment

mstoykov Feb 16, 2022

Choose a reason for hiding this comment

oleiade Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

mstoykov Feb 18, 2022

Choose a reason for hiding this comment

oleiade commented Feb 18, 2022

mstoykov commented Feb 18, 2022

oleiade commented Mar 25, 2022

oleiade commented Jan 27, 2022 •

edited

Loading

oleiade Feb 18, 2022 •

edited

Loading

oleiade Feb 18, 2022 •

edited

Loading