From 1d9c60c69c80d8c40ee663a4a44b1688dee6466b Mon Sep 17 00:00:00 2001
From: Mihail Stoykov <M.Stoikov@gmail.com>
Date: Thu, 11 May 2023 14:09:36 +0300
Subject: [PATCH] k6/metrics: Warn on metric names not compatible with
 Prometheus

This adds a warning for custom metrics that are not Prometheus
compatible.

The warning is in the js/bundle mostly for convenience.

It is a place we already have a registry and a logger and we go through
only once.

As this is temporary warning until we enforce stricter metric names
I don't think it is good idea to refactor the registry to support this
for a little while.

Updates #3065
---
 cmd/tests/cmd_run_test.go    | 38 ++++++++++++++++++++++++++++++++++++
 js/bundle.go                 | 21 ++++++++++++++++++++
 lib/testutils/logrus_hook.go | 12 ++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/cmd/tests/cmd_run_test.go b/cmd/tests/cmd_run_test.go
index 77c5c5f2f57..82068c5b550 100644
--- a/cmd/tests/cmd_run_test.go
+++ b/cmd/tests/cmd_run_test.go
@@ -1543,6 +1543,44 @@ func TestMinIterationDuration(t *testing.T) {
 	assert.Contains(t, stdout, "✓ test_counter.........: 3")
 }
 
+func TestMetricNameWarning(t *testing.T) {
+	t.Parallel()
+	script := `
+		import { Counter } from 'k6/metrics';
+
+		export let options = {
+			vus: 2,
+			iterations: 2,
+			thresholds: {
+				'test counter': ['count == 4'],
+			},
+		};
+
+		var c = new Counter('test counter');
+		new Counter('test_counter_#');
+
+		export function setup() { c.add(1); };
+		export default function () { c.add(1); };
+		export function teardown() { c.add(1); };
+	`
+
+	ts := getSimpleCloudOutputTestState(t, script, nil, cloudapi.RunStatusFinished, cloudapi.ResultStatusPassed, 0)
+
+	cmd.ExecuteWithGlobalState(ts.GlobalState)
+
+	stdout := ts.Stdout.String()
+	t.Log(stdout)
+
+	logEntries := ts.LoggerHook.Drain()
+	expectedMsg := `Metric name should only include ASCII letters, numbers and underscores. This name will stop working in `
+	filteredEntries := testutils.FilterEntries(logEntries, logrus.WarnLevel, expectedMsg)
+	require.Len(t, filteredEntries, 2)
+	// we do it this way as ordering is not guaranteed
+	names := []interface{}{filteredEntries[0].Data["name"], filteredEntries[1].Data["name"]}
+	require.Contains(t, names, "test counter")
+	require.Contains(t, names, "test_counter_#")
+}
+
 func TestRunTags(t *testing.T) {
 	t.Parallel()
 
diff --git a/js/bundle.go b/js/bundle.go
index 27009191660..cccfcc75d98 100644
--- a/js/bundle.go
+++ b/js/bundle.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/url"
 	"path/filepath"
+	"regexp"
 	"runtime"
 
 	"github.com/dop251/goja"
@@ -39,6 +40,24 @@ type Bundle struct {
 	moduleResolver  *moduleResolver
 }
 
+// TODO: this is to be removed once this is not a warning and it can be moved to the registry
+// https://github.com/grafana/k6/issues/3065
+func (b *Bundle) checkMetricNamesForPrometheusCompatibility() {
+	const (
+		nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,63}$"
+		badNameWarning  = "Metric name should only include ASCII letters, numbers and underscores. " +
+			"This name will stop working in k6 v0.48.0 (around December 2023)."
+	)
+
+	compileNameRegex := regexp.MustCompile(nameRegexString)
+
+	for _, metric := range b.preInitState.Registry.All() {
+		if !compileNameRegex.MatchString(metric.Name) {
+			b.preInitState.Logger.WithField("name", metric.Name).Warn(badNameWarning)
+		}
+	}
+}
+
 // A BundleInstance is a self-contained instance of a Bundle.
 type BundleInstance struct {
 	Runtime *goja.Runtime
@@ -107,6 +126,8 @@ func newBundle(
 		return nil, err
 	}
 
+	bundle.checkMetricNamesForPrometheusCompatibility()
+
 	return bundle, nil
 }
 
diff --git a/lib/testutils/logrus_hook.go b/lib/testutils/logrus_hook.go
index 770d7d02f44..0813a1d73b3 100644
--- a/lib/testutils/logrus_hook.go
+++ b/lib/testutils/logrus_hook.go
@@ -87,3 +87,15 @@ func LogContains(logEntries []logrus.Entry, expLevel logrus.Level, expContents s
 	}
 	return false
 }
+
+// FilterEntries is a helper function that checks the provided list of log entries
+// for a messages matching the provided level and contents and returns an array with only them.
+func FilterEntries(logEntries []logrus.Entry, expLevel logrus.Level, expContents string) []logrus.Entry {
+	filtered := make([]logrus.Entry, 0)
+	for _, entry := range logEntries {
+		if entry.Level == expLevel && strings.Contains(entry.Message, expContents) {
+			filtered = append(filtered, entry)
+		}
+	}
+	return filtered
+}