Add restrictions to metric names fix grafana#317 (grafana#810)

js/modules/k6/metrics/metrics.go

@@ -23,13 +23,26 @@ package metrics
 import (
 	"context"
 	"errors"
+	"regexp"
 	"time"
 
 	"github.com/dop251/goja"
 	"github.com/loadimpact/k6/js/common"
 	"github.com/loadimpact/k6/stats"
 )
 
+// ErrInvalidMetricName is the error returned when the name of the metric is not containeing only
+// letters, numbers, hyphens, dots, underscores, dots and both normal and square brackets
+var ErrInvalidMetricName = errors.New("Invalid metric name")
+
+var nameRegexString = "^[\\p{L}\\p{N}\\._ -]{1,128}$"
+
+var compileNameRegex = regexp.MustCompile(nameRegexString)
+
+func checkName(name string) bool {
+	return compileNameRegex.Match([]byte(name))
+}
+
 type Metric struct {
 	metric *stats.Metric
 }
@@ -42,6 +55,11 @@ func newMetric(ctxPtr *context.Context, name string, t stats.MetricType, isTime
 		return nil, errors.New("Metrics must be declared in the init context")
 	}
 
+	//TODO: move verification outside the JS
+	if !checkName(name) {
+		return nil, ErrInvalidMetricName
+	}
+
 	valueType := stats.Default
 	if len(isTime) > 0 && isTime[0] {
 		valueType = stats.Time

js/modules/k6/metrics/metrics_test.go

@@ -146,3 +146,25 @@ func TestMetrics(t *testing.T) {
 		})
 	}
 }
+
+func TestMetricNames(t *testing.T) {
+	t.Parallel()
+	var testMap = map[string]bool{
+		"simple":       true,
+		"still_simple": true,
+		"":             false,
+		"@":            false,
+		"a":            true,
+		"special\n\t":  false,
+		// this has both hangul and japanese numerals .
+		"hello.World_in_한글一안녕一세상": true,
+		// too long
+		"tooolooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooog": false,
+	}
+
+	for key, value := range testMap {
+		t.Run(key, func(t *testing.T) {
+			assert.Equal(t, value, checkName(key), key)
+		})
+	}
+}

js/runner_test.go

@@ -223,6 +223,28 @@ func TestOptionsPropagationToScript(t *testing.T) {
 	}
 }
 
+func TestMetricName(t *testing.T) {
+	tb := testutils.NewHTTPMultiBin(t)
+	defer tb.Cleanup()
+
+	script := []byte(tb.Replacer.Replace(`
+		import { Counter } from "k6/metrics";
+
+		let myCounter = new Counter("not ok name @");
+
+		export default function(data) {
+			myCounter.add(1);
+		}
+	`))
+
+	_, err := New(
+		&lib.SourceData{Filename: "/script.js", Data: script},
+		afero.NewMemMapFs(),
+		lib.RuntimeOptions{},
+	)
+	require.Error(t, err)
+}
+
 func TestSetupDataIsolation(t *testing.T) {
 	tb := testutils.NewHTTPMultiBin(t)
 	defer tb.Cleanup()

release notes/upcoming.md

@@ -124,3 +124,9 @@ A new option that disables the end-of-test summary has been added. That summary
 * UX: Instead of panicking on some operations in the init context, we now return an error that the given
   action is not supported; this includes making HTTP requests (batched or not), websockets,
   adding to custom metrics, making checks and groups, or initializing cookie jars (#811)
+
+
+## Breaking changes
+* Metric names are now restricted to only allow Unicode letters or numbers, spaces, dots, underscores, and
+  hyphens. They also need to be between 1 and 128 characters. Previously practically anything was a
+  valid metric name. (#810)