Add a threshold validation function to cmd/config

This commit adds a `validateThresholdConfig` function to `cmd/config`.
This function validates that any thresholds defined in the config apply
to existing metrics, and use methods that are valid for the metrics
they apply to.

As a side effect, this commit adds both a `Get` and `Has` methods to
`metrics.Registry` in order to be able to query registered metrics,
regardless of whether they are custom or builtin metrics. Note that
the `validateThresholdConfig` function makes the assumption that the
passed in `metrics.Registry` has been loaded with 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

cmd/config.go

@@ -37,6 +37,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"
 )
@@ -267,3 +268,98 @@ func validateScenarioConfig(conf lib.ExecutorConfig, isExecutable func(string) b
 	}
 	return nil
 }
+
+func validateThresholdsConfig(conf Config, registry *metrics.Registry) error {
+	// Registry should not be nil under any circumstances.
+	// N.B: 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 registry == nil {
+		err := fmt.Errorf(
+			"unable to validate thresholds configuration; reason: " +
+				"provided registry is nil. It looks like you have found a bug, " +
+				"please open an issue on our GitHub: " +
+				"https://github.com/grafana/k6/issues/new?assignees=&labels=bug&template=bug.yaml",
+		)
+		return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+	}
+
+	// Focusing on builtin metric for now. For each threshold,
+	// found in the options, do we have a matching builtin metric?
+	for name, thresholds := range conf.Thresholds {
+		// The defined threshold applies to a non-existing metrics
+		if !registry.Has(name) {
+			err := fmt.Errorf("invalid threshold defined on %s; reason: no metric named %s found", name, name)
+			return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+		}
+
+		// Get the metric from the registry
+		metric := registry.Get(name)
+
+		// TODO: factorize the looping and looking over the the thresholds.Thresholds somehow
+		switch metric.Type {
+		case stats.Counter:
+			supportedMethods := []string{stats.TokenCount, stats.TokenRate}
+
+			for _, threshold := range thresholds.Thresholds {
+				if !lib.Contains(supportedMethods, threshold.Parsed.AggregationMethod) {
+					// if threshold.Parsed.AggregationMethod != stats.TokenCount &&
+					// 	threshold.Parsed.AggregationMethod != stats.TokenRate {
+					err := fmt.Errorf(
+						"invalid threshold expression %s: '%s'; "+
+							"reason: invalid aggregation method '%s' applied to the '%s' metric. "+
+							"%s is a metric of type Counter, did you mean to use the any of the "+
+							"'count' or 'rate' aggregation methods instead?",
+						name, threshold.Source, threshold.Parsed.AggregationMethod, name, name,
+					)
+					return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+				}
+			}
+		case stats.Gauge:
+			for _, threshold := range thresholds.Thresholds {
+				if threshold.Parsed.AggregationMethod != stats.TokenValue {
+					err := fmt.Errorf(
+						"invalid threshold expression %s: '%s'; "+
+							"reason: invalid aggregation method '%s' applied to the '%s' metric. "+
+							"%s is a metric of type Gauge, did you mean to use the 'value' aggregation method instead?",
+						name, threshold.Source, threshold.Parsed.AggregationMethod, name, name,
+					)
+					return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+				}
+			}
+		case stats.Rate:
+			for _, threshold := range thresholds.Thresholds {
+				if threshold.Parsed.AggregationMethod != stats.TokenRate {
+					err := fmt.Errorf(
+						"invalid threshold expression %s: '%s'; "+
+							"reason: invalid aggregation method '%s' applied to the '%s' metric. "+
+							"%s is a metric of type Rate, did you mean to use the 'rate' aggregation method instead?",
+						name, threshold.Source, threshold.Parsed.AggregationMethod, name, name,
+					)
+					return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+				}
+			}
+		case stats.Trend:
+			supportedMethods := []string{
+				stats.TokenAvg,
+				stats.TokenMin,
+				stats.TokenMax,
+				stats.TokenMed,
+				stats.TokenPercentile,
+			}
+			for _, threshold := range thresholds.Thresholds {
+				if !lib.Contains(supportedMethods, threshold.Parsed.AggregationMethod) {
+					err := fmt.Errorf(
+						"invalid threshold expression %s: '%s'; "+
+							"reason: invalid aggregation method '%s' applied to the '%s' metric. "+
+							"%s is a metric of type Trend, did you mean to use any of the "+
+							"'avg', 'min', 'med', or 'p(N)' aggregation methods instead?",
+						name, threshold.Source, threshold.Parsed.AggregationMethod, name, name,
+					)
+					return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+				}
+			}
+		}
+	}
+
+	return nil
+}

cmd/config_test.go

@@ -27,14 +27,16 @@ import (
 
 	"github.com/kelseyhightower/envconfig"
 	"github.com/stretchr/testify/assert"
-	"gopkg.in/guregu/null.v3"
-
+	"github.com/stretchr/testify/require"
 	"go.k6.io/k6/errext"
 	"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/testutils"
 	"go.k6.io/k6/lib/types"
+	"go.k6.io/k6/stats"
+	"gopkg.in/guregu/null.v3"
 )
 
 type testCmdData struct {
@@ -189,3 +191,116 @@ func TestDeriveAndValidateConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateThresholdsConfigWithNilRegistry(t *testing.T) {
+	var registry *metrics.Registry
+	config := Config{}
+	var wantErrType errext.HasExitCode
+
+	gotErr := validateThresholdsConfig(config, registry)
+
+	assert.Error(t, gotErr, "validateThresholdsConfig should fail when passed registry is nil")
+	assert.ErrorAs(t, gotErr, &wantErrType, "validateThresholdsConfig error should be an instance of errext.HasExitCode")
+}
+
+func TestValidateThresholdsConfigAppliesToBuiltinMetrics(t *testing.T) {
+	// Prepare a registry loaded with builtin metrics
+	registry := metrics.NewRegistry()
+	metrics.RegisterBuiltinMetrics(registry)
+
+	// Assuming builtin metrics are indeed registered, and
+	// thresholds parsing works as expected, we prepare
+	// thresholds for a counter builting metric; namely http_reqs
+	HTTPReqsThresholds, err := stats.NewThresholds([]string{"count>0", "rate>1"})
+	require.NoError(t, err, "instantiating Thresholds with expression 'count>0' should not fail")
+	options := lib.Options{
+		Thresholds: map[string]stats.Thresholds{
+			metrics.HTTPReqsName: HTTPReqsThresholds,
+		},
+	}
+	config := Config{Options: options}
+
+	gotErr := validateThresholdsConfig(config, registry)
+
+	assert.NoError(t, gotErr, "validateThresholdsConfig should not fail against builtin metrics")
+}
+
+func TestValidateThresholdsConfigAppliesToCustomMetrics(t *testing.T) {
+	t.Parallel()
+
+	// Prepare a registry loaded with both builtin metrics,
+	// and a custom counter metric.
+	testCounterMetricName := "testcounter"
+	registry := metrics.NewRegistry()
+	metrics.RegisterBuiltinMetrics(registry)
+	_, err := registry.NewMetric(testCounterMetricName, stats.Counter)
+	require.NoError(t, err, "registering custom counter metric should not fail")
+
+	// Prepare a configuration containing a Threshold
+	counterThresholds, err := stats.NewThresholds([]string{"count>0", "rate>1"})
+	require.NoError(t, err, "instantiating Thresholds with expression 'count>0' should not fail")
+	options := lib.Options{
+		Thresholds: map[string]stats.Thresholds{
+			testCounterMetricName: counterThresholds,
+		},
+	}
+	config := Config{Options: options}
+
+	gotErr := validateThresholdsConfig(config, registry)
+
+	// Assert
+	assert.NoError(t, gotErr, "validateThresholdsConfig should not fail against existing and valid custom metric")
+}
+
+func TestValidateThresholdsConfigFailsOnNonExistingMetric(t *testing.T) {
+	t.Parallel()
+
+	// Prepare a registry loaded with builtin metrics only
+	registry := metrics.NewRegistry()
+	metrics.RegisterBuiltinMetrics(registry)
+
+	// Prepare a configuration containing a Threshold applying to
+	// a non-existing metric
+	counterThresholds, err := stats.NewThresholds([]string{"count>0", "rate>1"})
+	require.NoError(t, err, "instantiating Thresholds with expression 'count>0' should not fail")
+	options := lib.Options{
+		Thresholds: map[string]stats.Thresholds{
+			"nonexisting": counterThresholds,
+		},
+	}
+	config := Config{Options: options}
+	var wantErrType errext.HasExitCode
+
+	gotErr := validateThresholdsConfig(config, registry)
+
+	// Assert
+	assert.Error(t, gotErr, "validateThresholdsConfig should fail on thresholds applied to a non-existing metric")
+	assert.ErrorAs(t, gotErr, &wantErrType, "validateThresholdsConfig error should be an instance of errext.HasExitCode")
+}
+
+func TestValidateThresholdsConfigFailsOnThresholdInvalidMetricType(t *testing.T) {
+	t.Parallel()
+
+	// Prepare a registry loaded with builtin metrics only
+	registry := metrics.NewRegistry()
+	metrics.RegisterBuiltinMetrics(registry)
+
+	// Prepare a configuration containing a Threshold using a Counter metric
+	// specific aggregation method, against a metric of type Gauge: which doesn't support
+	// that method.
+	VUsThresholds, err := stats.NewThresholds([]string{"count>0"})
+	require.NoError(t, err, "instantiating Thresholds with expression 'count>0' should not fail")
+	options := lib.Options{
+		Thresholds: map[string]stats.Thresholds{
+			metrics.VUsName: VUsThresholds,
+		},
+	}
+	config := Config{Options: options}
+	var wantErrType errext.HasExitCode
+
+	gotErr := validateThresholdsConfig(config, registry)
+
+	// Assert
+	assert.Error(t, gotErr, "validateThresholdsConfig should fail applying the count method to a Gauge metric")
+	assert.ErrorAs(t, gotErr, &wantErrType, "validateThresholdsConfig error should be an instance of errext.HasExitCode")
+}

lib/metrics/registry.go

@@ -85,3 +85,12 @@ func (r *Registry) MustNewMetric(name string, typ stats.MetricType, t ...stats.V
 	}
 	return m
 }
+
+func (r *Registry) Get(name string) *stats.Metric {
+	return r.metrics[name]
+}
+
+func (r *Registry) Has(name string) bool {
+	_, ok := r.metrics[name]
+	return ok
+}

lib/util.go

@@ -66,3 +66,14 @@ func Min(a, b int64) int64 {
 	}
 	return b
 }
+
+// Contains checks if a string is present in a slice
+func Contains(slice []string, str string) bool {
+	for _, element := range slice {
+		if element == str {
+			return true
+		}
+	}
+
+	return false
+}