Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics: enforce more otel/prometheus aligned names #3448

Merged
merged 3 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions cmd/tests/cmd_run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1542,7 +1542,7 @@ func TestMinIterationDuration(t *testing.T) {
assert.Contains(t, stdout, "✓ test_counter.........: 3")
}

func TestMetricNameWarning(t *testing.T) {
func TestMetricNameError(t *testing.T) {
t.Parallel()
script := `
import { Counter } from 'k6/metrics';
Expand All @@ -1556,28 +1556,25 @@ func TestMetricNameWarning(t *testing.T) {
};

var c = new Counter('test counter');
new Counter('test_counter_#');
new Counter('test_counter_#'); // this is also bad but we error on the one above

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)
ts := getSingleFileTestState(t, script, nil, exitcodes.ScriptException)

cmd.ExecuteWithGlobalState(ts.GlobalState)

stdout := ts.Stdout.String()
t.Log(stdout)

logEntries := ts.LoggerHook.Drain()
expectedMsg := `Metric name should only include up to 128 ASCII letters, numbers and/or underscores.`
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_#")
expectedMsg := `Metric names must only include up to 128 ASCII letters, numbers, or underscores`
filteredEntries := testutils.FilterEntries(logEntries, logrus.ErrorLevel, expectedMsg)
require.Len(t, filteredEntries, 1)
require.Contains(t, filteredEntries[0].Message, "'test counter'")
}

func TestRunTags(t *testing.T) {
Expand Down
23 changes: 0 additions & 23 deletions js/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"fmt"
"net/url"
"path/filepath"
"regexp"
"runtime"

"github.com/dop251/goja"
Expand Down Expand Up @@ -42,26 +41,6 @@ type Bundle struct {
ModuleResolver *modules.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 (
// The name restrictions are the union of Otel and Prometheus naming restrictions, with the length restrictions of 128
// coming from old k6 restrictions where character set was way bigger though.
nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,128}$"
badNameWarning = "Metric name should only include up to 128 ASCII letters, numbers and/or 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
Expand Down Expand Up @@ -134,8 +113,6 @@ func newBundle(
return nil, err
}

bundle.checkMetricNamesForPrometheusCompatibility()

return bundle, nil
}

Expand Down
8 changes: 6 additions & 2 deletions metrics/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ func NewRegistry() *Registry {
}
}

const nameRegexString = "^[\\p{L}\\p{N}\\._ !\\?/&#\\(\\)<>%-]{1,128}$"
const (
nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,128}$"
badNameWarning = "Metric names must only include up to 128 ASCII letters, numbers, or underscores " +
"and start with a letter or an underscore."
)

var compileNameRegex = regexp.MustCompile(nameRegexString)

Expand All @@ -41,7 +45,7 @@ func (r *Registry) NewMetric(name string, typ MetricType, t ...ValueType) (*Metr
defer r.l.Unlock()

if !checkName(name) {
return nil, fmt.Errorf("Invalid metric name: '%s'", name) //nolint:golint,stylecheck
return nil, fmt.Errorf("Invalid metric name: '%s'. %s", name, badNameWarning) //nolint:golint,stylecheck
}
oldMetric, ok := r.metrics[name]

Expand Down
4 changes: 2 additions & 2 deletions metrics/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ func TestMetricNames(t *testing.T) {
"still_simple": true,
"": false,
"@": false,
"a": true,
"a": false, // too short
"special\n\t": false,
// this has both hangul and japanese numerals .
"hello.World_in_한글一안녕一세상": true,
"hello.World_in_한글一안녕一세상": false,
// too long
"tooolooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooog": false,
}
Expand Down