Validate thresholds at init #2463

oleiade · 2022-03-25T11:04:57Z

What this PR does

We merged Go-based thresholds parsing in v0.37.0. This Pull Request builds upon that and implements thresholds validation at init. As of this PR, unless the --no-thresholds flag is passed, thresholds will be checked before the execution of k6 commands. Specifically, if a threshold applies to a non-existing metric, or if it uses an aggregation method that's not supported by the former, k6 will immediately produce an error; the execution won't even start.

This addresses #2330 to the extent of the internally agreed upon scope.

cmd/testdata/thresholds/non_existing_submetric.js

metrics/metric.go

mstoykov · 2022-04-06T08:30:53Z

The PR descriptions still talks about submetric checking

Specifically, if a threshold applies to a non-existing metric, or submetric (including groups), or if it uses an aggregation method that's not supported by the former, k6 will immediately produce an error; the execution won't even start.

oleiade · 2022-04-06T08:33:30Z

@mstoykov Good catch 👍🏻 Edited out 🙇🏻

oleiade · 2022-04-06T08:42:53Z

For your information: I've updated the code of the ParseMetricName function to cater for the cases where tags definition curly braces are either unmatched, or in the wrong order 👍🏻

cmd/archive_test.go

cmd/run_test.go

cmd/testdata/thresholds/non_existing_metric.js

metrics/metric_test.go

metrics/thresholds.go

mstoykov · 2022-04-06T08:50:31Z

As a general nitpick: I think you are adding why too many assert/require custom messages which quickly add up to the size of each test, but ... I would argue most of them (definitely not all) have very little to add to the readability of the test. And the rare cases the test fail I would argue will require someone to go read the code either way.

And in that later case, each assert/require not being on 4 lines will be more beneficial IMO.

olegbespalov

Looks good 👍

metrics/thresholds_parser.go

metrics/metric.go

codebien

Thanks for the great comments, and for making errors with high dev experience for k6 users. 🙏

I left some comments, some of them could be applied in multiple places but I didn't spot all of them because I expect some could require some discussions before.

metrics/metric.go

metrics/metric_type.go

metrics/thresholds.go

metrics/thresholds_parser.go

metrics/thresholds_test.go

oleiade · 2022-04-11T13:25:35Z

@mstoykov I've removed most assert/require custom error messages indeed. I kept them in table tests where I do them, find them helpful in figuring out which tests are failing when they do.

@olegbespalov @codebien I've addressed your comments 👍🏻

Let me know if you have other comments and suggestions. Otherwise, I'll be looking forward to the approval 🚅 🎉

codebien

LGTM 👏 Just one minor thing

metrics/metric_test.go

mstoykov

LGTM 🎉 👏

The newly introduced ParseMetricName function takes a metric name expression of the form `name{tag_key:value,...}` and splits into its name, and tags. This function proves useful when needing to address metrics, and their submetrics.

The newly introduced Validate method ensures that for a given collection of Thresholds, they apply to existing metrics/submetrics, and apply methods that are supported by the former.

This commit ensures that impacted commands (run, archive, cloud) validate thresholds before running.

github-actions bot requested review from na-- and olegbespalov March 25, 2022 11:05

oleiade requested review from mstoykov and codebien and removed request for na-- March 25, 2022 11:06

oleiade added this to the v0.38.0 milestone Mar 25, 2022

oleiade added the feature label Mar 25, 2022

oleiade self-assigned this Mar 25, 2022

This was referenced Mar 25, 2022

Parse and validate threshold during init #2356

Closed

Use the registry for (sub-)metric validation and move data crunching out of the Engine #2426

Merged

oleiade force-pushed the refactor/move_stats_to_metrics branch 8 times, most recently from 8ea9ce4 to 766afc4 Compare March 29, 2022 16:27

Base automatically changed from refactor/move_stats_to_metrics to master March 30, 2022 11:43

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch 2 times, most recently from 8950690 to 1605cfa Compare April 1, 2022 16:28

mstoykov reviewed Apr 4, 2022

View reviewed changes

cmd/testdata/thresholds/non_existing_submetric.js Outdated Show resolved Hide resolved

mstoykov reviewed Apr 4, 2022

View reviewed changes

metrics/metric.go Outdated Show resolved Hide resolved

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch 4 times, most recently from a2010e7 to b5192d0 Compare April 6, 2022 08:16

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch from b5192d0 to 052e2d4 Compare April 6, 2022 08:42

mstoykov reviewed Apr 6, 2022

View reviewed changes

cmd/archive_test.go Show resolved Hide resolved

cmd/run_test.go Show resolved Hide resolved

cmd/testdata/thresholds/non_existing_metric.js Show resolved Hide resolved

metrics/metric_test.go Show resolved Hide resolved

metrics/thresholds.go Show resolved Hide resolved

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch 2 times, most recently from 46e8a7b to 43e29c1 Compare April 6, 2022 11:31

olegbespalov previously approved these changes Apr 11, 2022

View reviewed changes

metrics/thresholds_parser.go Outdated Show resolved Hide resolved

metrics/metric.go Outdated Show resolved Hide resolved

codebien requested changes Apr 11, 2022

View reviewed changes

oleiade dismissed olegbespalov’s stale review via ea71326 April 11, 2022 12:47

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch 4 times, most recently from bc07116 to 0dc981a Compare April 11, 2022 13:11

codebien previously approved these changes Apr 11, 2022

View reviewed changes

metrics/metric_test.go Outdated Show resolved Hide resolved

olegbespalov previously approved these changes Apr 11, 2022

View reviewed changes

mstoykov previously approved these changes Apr 12, 2022

View reviewed changes

oleiade dismissed stale reviews from mstoykov, olegbespalov, and codebien via 924fcf7 April 12, 2022 08:38

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch from 0dc981a to 924fcf7 Compare April 12, 2022 08:38

oleiade added 3 commits April 12, 2022 10:56

Add ParseMetricName function to metrics package

48dea1f

The newly introduced ParseMetricName function takes a metric name expression of the form `name{tag_key:value,...}` and splits into its name, and tags. This function proves useful when needing to address metrics, and their submetrics.

Add a Thresholds.Validate method

da663f5

The newly introduced Validate method ensures that for a given collection of Thresholds, they apply to existing metrics/submetrics, and apply methods that are supported by the former.

Ensure commands validate thresholds

130e61d

This commit ensures that impacted commands (run, archive, cloud) validate thresholds before running.

oleiade force-pushed the feature/2330_validate_thresholds_at_init branch from 924fcf7 to 130e61d Compare April 12, 2022 08:57

olegbespalov approved these changes Apr 12, 2022

View reviewed changes

mstoykov approved these changes Apr 12, 2022

View reviewed changes

codebien approved these changes Apr 12, 2022

View reviewed changes

oleiade merged commit 8298ee4 into master Apr 12, 2022

oleiade deleted the feature/2330_validate_thresholds_at_init branch April 12, 2022 12:05

na-- mentioned this pull request Jun 9, 2022

OpenTelemetry metrics output #2557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate thresholds at init #2463

Validate thresholds at init #2463

oleiade commented Mar 25, 2022 •

edited

Loading

mstoykov commented Apr 6, 2022

oleiade commented Apr 6, 2022

oleiade commented Apr 6, 2022

mstoykov commented Apr 6, 2022

olegbespalov left a comment

codebien left a comment

oleiade commented Apr 11, 2022

codebien left a comment

mstoykov left a comment

Validate thresholds at init #2463

Validate thresholds at init #2463

Conversation

oleiade commented Mar 25, 2022 • edited Loading

What this PR does

mstoykov commented Apr 6, 2022

oleiade commented Apr 6, 2022

oleiade commented Apr 6, 2022

mstoykov commented Apr 6, 2022

olegbespalov left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

oleiade commented Apr 11, 2022

codebien left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

oleiade commented Mar 25, 2022 •

edited

Loading