Merge pull request #2463 from grafana/feature/2330_validate_threshold…

…s_at_init

Validate thresholds at init

cmd/archive_test.go

@@ -32,6 +32,36 @@ func TestArchiveThresholds(t *testing.T) {
 			testFilename: "testdata/thresholds/malformed_expression.js",
 			wantErr:      false,
 		},
+		{
+			name:         "run should fail with exit status 104 on a threshold applied to a non existing metric",
+			noThresholds: false,
+			testFilename: "testdata/thresholds/non_existing_metric.js",
+			wantErr:      true,
+		},
+		{
+			name:         "run should succeed on a threshold applied to a non existing metric with the --no-thresholds flag set",
+			noThresholds: true,
+			testFilename: "testdata/thresholds/non_existing_metric.js",
+			wantErr:      false,
+		},
+		{
+			name:         "run should succeed on a threshold applied to a non existing submetric with the --no-thresholds flag set",
+			noThresholds: true,
+			testFilename: "testdata/thresholds/non_existing_metric.js",
+			wantErr:      false,
+		},
+		{
+			name:         "run should fail with exit status 104 on a threshold applying an unsupported aggregation method to a metric",
+			noThresholds: false,
+			testFilename: "testdata/thresholds/unsupported_aggregation_method.js",
+			wantErr:      true,
+		},
+		{
+			name:         "run should succeed on a threshold applying an unsupported aggregation method to a metric with the --no-thresholds flag set",
+			noThresholds: true,
+			testFilename: "testdata/thresholds/unsupported_aggregation_method.js",
+			wantErr:      false,
+		},
 	}
 
 	for _, testCase := range testCases {

cmd/run_test.go

@@ -167,6 +167,22 @@ func TestRunScriptErrorsAndAbort(t *testing.T) {
 			expErr:       "ReferenceError: someUndefinedVar is not defined",
 			expExitCode:  exitcodes.ScriptException,
 		},
+		{
+			testFilename: "thresholds/non_existing_metric.js",
+			name:         "run should fail with exit status 104 on a threshold applied to a non existing metric",
+			expErr:       "invalid threshold",
+			expExitCode:  exitcodes.InvalidConfig,
+		},
+		{
+			testFilename: "thresholds/non_existing_metric.js",
+			name:         "run should succeed on a threshold applied to a non existing metric with the --no-thresholds flag set",
+			extraArgs:    []string{"--no-thresholds"},
+		},
+		{
+			testFilename: "thresholds/non_existing_metric.js",
+			name:         "run should succeed on a threshold applied to a non existing submetric with the --no-thresholds flag set",
+			extraArgs:    []string{"--no-thresholds"},
+		},
 		{
 			testFilename: "thresholds/malformed_expression.js",
 			name:         "run should fail with exit status 104 on a malformed threshold expression",
@@ -179,6 +195,17 @@ func TestRunScriptErrorsAndAbort(t *testing.T) {
 			extraArgs:    []string{"--no-thresholds"},
 			// we don't expect an error
 		},
+		{
+			testFilename: "thresholds/unsupported_aggregation_method.js",
+			name:         "run should fail with exit status 104 on a threshold applying an unsupported aggregation method to a metric",
+			expErr:       "invalid threshold",
+			expExitCode:  exitcodes.InvalidConfig,
+		},
+		{
+			testFilename: "thresholds/unsupported_aggregation_method.js",
+			name:         "run should succeed on a threshold applying an unsupported aggregation method to a metric with the --no-thresholds flag set",
+			extraArgs:    []string{"--no-thresholds"},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -212,3 +239,48 @@ func TestRunScriptErrorsAndAbort(t *testing.T) {
 		})
 	}
 }
+
+func TestInvalidOptionsThresholdErrExitCode(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name         string
+		testFilename string
+		expExitCode  errext.ExitCode
+		extraArgs    []string
+	}{
+		{
+			name:         "run should fail with exit status 104 on a malformed threshold expression",
+			testFilename: "thresholds/malformed_expression.js",
+			expExitCode:  exitcodes.InvalidConfig,
+		},
+		{
+			name:         "run should fail with exit status 104 on a threshold applied to a non existing metric",
+			testFilename: "thresholds/non_existing_metric.js",
+			expExitCode:  exitcodes.InvalidConfig,
+		},
+		{
+			name:         "run should fail with exit status 104 on a threshold method being unsupported by the metric",
+			testFilename: "thresholds/unsupported_aggregation_method.js",
+			expExitCode:  exitcodes.InvalidConfig,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			testScript, err := ioutil.ReadFile(path.Join("testdata", tc.testFilename))
+			require.NoError(t, err)
+
+			testState := newGlobalTestState(t)
+			require.NoError(t, afero.WriteFile(testState.fs, filepath.Join(testState.cwd, tc.testFilename), testScript, 0o644))
+			testState.args = append([]string{"k6", "run", tc.testFilename}, tc.extraArgs...)
+
+			testState.expectedExitCode = int(tc.expExitCode)
+			newRootCommand(testState.globalState).execute()
+		})
+	}
+}

cmd/test_load.go

@@ -208,8 +208,13 @@ func (lt *loadedTest) consolidateDeriveAndValidateConfig(
 	// If parsing the threshold expressions failed, consider it as an
 	// invalid configuration error.
 	if !lt.runtimeOptions.NoThresholds.Bool {
-		for _, thresholds := range consolidatedConfig.Options.Thresholds {
-			err = thresholds.Parse()
+		for metricName, thresholdsDefinition := range consolidatedConfig.Options.Thresholds {
+			err = thresholdsDefinition.Parse()
+			if err != nil {
+				return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+			}
+
+			err = thresholdsDefinition.Validate(metricName, lt.metricsRegistry)
 			if err != nil {
 				return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
 			}

cmd/testdata/thresholds/non_existing_metric.js

@@ -0,0 +1,13 @@
+export const options = {
+	thresholds: {
+		// non existing is neither registered, nor a builtin metric.
+		// k6 should catch that.
+		"non existing": ["rate>0"],
+	},
+};
+
+export default function () {
+	console.log(
+		"asserting that a threshold over a non-existing metric fails with exit code 104 (Invalid config)"
+	);
+}

cmd/testdata/thresholds/unsupported_aggregation_method.js

@@ -0,0 +1,15 @@
+export const options = {
+	thresholds: {
+		// http_reqs is a Counter metric. As such, it supports
+		// only the 'count' and 'rate' operations. Thus, 'value'
+		// being a Gauge's metric aggregation method, the threshold
+		// configuration evaluation should fail.
+		http_reqs: ["value>0"],
+	},
+};
+
+export default function () {
+	console.log(
+		"asserting that a threshold applying a method over a metric not supporting it fails with exit code 104 (Invalid config)"
+	);
+}

metrics/metric.go

@@ -1,6 +1,7 @@
 package metrics
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -40,6 +41,7 @@ func newMetric(name string, mt MetricType, vt ...ValueType) *Metric {
 	if len(vt) > 0 {
 		valueType = vt[0]
 	}
+
 	var sink Sink
 	switch mt {
 	case Counter:
@@ -53,6 +55,7 @@ func newMetric(name string, mt MetricType, vt ...ValueType) *Metric {
 	default:
 		return nil
 	}
+
 	return &Metric{
 		Name:     name,
 		Type:     mt,
@@ -121,3 +124,60 @@ func (m *Metric) AddSubmetric(keyValues string) (*Submetric, error) {
 
 	return subMetric, nil
 }
+
+// ErrMetricNameParsing indicates parsing a metric name failed
+var ErrMetricNameParsing = errors.New("parsing metric name failed")
+
+// ParseMetricName parses a metric name expression of the form metric_name{tag_key:tag_value,...}
+// Its first return value is the parsed metric name, second are parsed tags as as slice
+// of "key:value" strings. On failure, it returns an error containing the `ErrMetricNameParsing` in its chain.
+func ParseMetricName(name string) (string, []string, error) {
+	openingTokenPos := strings.IndexByte(name, '{')
+	closingTokenPos := strings.IndexByte(name, '}')
+	containsOpeningToken := openingTokenPos != -1
+	containsClosingToken := closingTokenPos != -1
+
+	// Neither the opening '{' token nor the closing '}' token
+	// are present, thus the metric name only consists of a literal.
+	if !containsOpeningToken && !containsClosingToken {
+		return name, nil, nil
+	}
+
+	// If the name contains an opening or closing token, but not
+	// its counterpart, the expression is malformed.
+	if (containsOpeningToken && !containsClosingToken) ||
+		(!containsOpeningToken && containsClosingToken) {
+		return "", nil, fmt.Errorf("%w; reason: unmatched opening/close curly brace", ErrMetricNameParsing)
+	}
+
+	if closingTokenPos < openingTokenPos {
+		return "", nil, fmt.Errorf("%w; reason: closing curly brace appears before opening one", ErrMetricNameParsing)
+	}
+
+	parserFn := func(c rune) bool {
+		return c == '{' || c == '}'
+	}
+
+	// Split the metric_name{tag_key:tag_value,...} expression
+	// into two "metric_name" and "tag_key:tag_value,..." strings.
+	parts := strings.FieldsFunc(name, parserFn)
+	if len(parts) == 0 || len(parts) > 2 {
+		return "", nil, ErrMetricNameParsing
+	}
+
+	// Split the tag key values
+	tags := strings.Split(parts[1], ",")
+
+	// For each tag definition, ensure
+	for i, t := range tags {
+		keyValue := strings.SplitN(t, ":", 2)
+
+		if len(keyValue) != 2 || keyValue[1] == "" {
+			return "", nil, fmt.Errorf("%w; reason: malformed tag expression %q", ErrMetricNameParsing, t)
+		}
+
+		tags[i] = strings.TrimSpace(t)
+	}
+
+	return parts[0], tags, nil
+}

metrics/metric_test.go

@@ -66,3 +66,116 @@ func TestAddSubmetric(t *testing.T) {
 		})
 	}
 }
+
+func TestParseMetricName(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name                 string
+		metricNameExpression string
+		wantMetricName       string
+		wantTags             []string
+		wantErr              bool
+	}{
+		{
+			name:                 "metric name without tags",
+			metricNameExpression: "test_metric",
+			wantMetricName:       "test_metric",
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with single tag",
+			metricNameExpression: "test_metric{abc:123}",
+			wantMetricName:       "test_metric",
+			wantTags:             []string{"abc:123"},
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with multiple tags",
+			metricNameExpression: "test_metric{abc:123,easyas:doremi}",
+			wantMetricName:       "test_metric",
+			wantTags:             []string{"abc:123", "easyas:doremi"},
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with multiple spaced tags",
+			metricNameExpression: "test_metric{abc:123, easyas:doremi}",
+			wantMetricName:       "test_metric",
+			wantTags:             []string{"abc:123", "easyas:doremi"},
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with group tag",
+			metricNameExpression: "test_metric{group:::mygroup}",
+			wantMetricName:       "test_metric",
+			wantTags:             []string{"group:::mygroup"},
+			wantErr:              false,
+		},
+		{
+			name:                 "metric name with tag definition missing `:value`",
+			metricNameExpression: "test_metric{easyas}",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with tag definition missing value",
+			metricNameExpression: "test_metric{easyas:}",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with mixed valid and invalid tag definitions",
+			metricNameExpression: "test_metric{abc:123,easyas:}",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with valid name and unmatched opening tags definition token",
+			metricNameExpression: "test_metric{abc:123,easyas:doremi",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with valid name and unmatched closing tags definition token",
+			metricNameExpression: "test_metricabc:123,easyas:doremi}",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with valid name and invalid starting tags definition token",
+			metricNameExpression: "test_metric}abc:123,easyas:doremi}",
+			wantErr:              true,
+		},
+		{
+			name:                 "metric name with valid name and invalid curly braces in tags definition",
+			metricNameExpression: "test_metric}abc{bar",
+			wantErr:              true,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			gotMetricName, gotTags, gotErr := ParseMetricName(tt.metricNameExpression)
+
+			assert.Equal(t,
+				gotErr != nil, tt.wantErr,
+				"ParseMetricName() error = %v, wantErr %v", gotErr, tt.wantErr,
+			)
+
+			if gotErr != nil {
+				assert.ErrorIs(t,
+					gotErr, ErrMetricNameParsing,
+					"ParseMetricName() error chain should contain ErrMetricNameParsing",
+				)
+			}
+
+			assert.Equal(t,
+				gotMetricName, tt.wantMetricName,
+				"ParseMetricName() gotMetricName = %v, want %v", gotMetricName, tt.wantMetricName,
+			)
+
+			assert.Equal(t,
+				gotTags, tt.wantTags,
+				"ParseMetricName() gotTags = %v, want %v", gotTags, tt.wantTags,
+			)
+		})
+	}
+}