Validate instrument names when creating them (#4210)

* validate instrument names when creating them * add changelog entry * remove now invalid instrument name from prometheus test * fix invalid names in meter_test * keep returning the instrument even if its name is invalid * make invalid instrument name a known error * bring back prometheus invalid instrument name test * remove warning * fix lint * move name validation into the lookup method * fix lint again * Revert "move name validation into the lookup method" This reverts commit ec8ccc5fa00b84f24b63e48714672a8ee8e3e5d2. * rename ErrInvalidInstrumentName to ErrInstrumentName * switch to explicit validations, instead of a regexp * Update CHANGELOG.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * remove double check for empty name * test validation shortcut with a single character --------- Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
2025-02-07 13:31:42 +02:00 · 2023-06-09 17:32:56 +02:00 · 2023-06-09 17:32:56 +02:00 · 844b107e98
commit 844b107e98
parent 4f4815406a
4 changed files with 387 additions and 19 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

 - Starting from `v1.21.0` of semantic conventions, `go.opentelemetry.io/otel/semconv/{version}/httpconv` and `go.opentelemetry.io/otel/semconv/{version}/netconv` packages will no longer be published. (#4145)
 - Log duplicate instrument conflict at a warning level instead of info in `go.opentelemetry.io/otel/sdk/metric`. (#4202)
+- Return an error on the creation of new instruments if their name doesn't pass regexp validation. (#4210)

 ## [1.16.0/0.39.0] 2023-05-18

--- a/exporters/prometheus/exporter_test.go
+++ b/exporters/prometheus/exporter_test.go
@ -154,7 +154,7 @@ func TestPrometheusExporter(t *testing.T) {
 				gauge.Add(ctx, 100, opt)

 				counter, err := meter.Float64Counter("0invalid.counter.name", otelmetric.WithDescription("a counter with an invalid name"))
-				require.NoError(t, err)
+				require.ErrorIs(t, err, metric.ErrInstrumentName)
 				counter.Add(ctx, 100, opt)

 				histogram, err := meter.Float64Histogram("invalid.hist.name", otelmetric.WithDescription("a histogram with an invalid name"))
--- a/sdk/metric/meter.go
+++ b/sdk/metric/meter.go
@ -26,6 +26,12 @@ import (
 	"go.opentelemetry.io/otel/sdk/metric/internal"
 )

+var (
+	// ErrInstrumentName indicates the created instrument has an invalid name.
+	// Valid names must consist of 63 or fewer characters including alphanumeric, _, ., -, and start with a letter.
+	ErrInstrumentName = errors.New("invalid instrument name")
+)
+
 // meter handles the creation and coordination of all metric instruments. A
 // meter represents a single instrumentation scope; all metric telemetry
 // produced by an instrumentation scope will use metric instruments from a
@ -62,7 +68,12 @@ var _ metric.Meter = (*meter)(nil)
 func (m *meter) Int64Counter(name string, options ...metric.Int64CounterOption) (metric.Int64Counter, error) {
 	cfg := metric.NewInt64CounterConfig(options...)
 	const kind = InstrumentKindCounter
-	return m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Int64UpDownCounter returns a new instrument identified by name and
@ -71,7 +82,12 @@ func (m *meter) Int64Counter(name string, options ...metric.Int64CounterOption)
 func (m *meter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) {
 	cfg := metric.NewInt64UpDownCounterConfig(options...)
 	const kind = InstrumentKindUpDownCounter
-	return m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Int64Histogram returns a new instrument identified by name and configured
@ -80,7 +96,12 @@ func (m *meter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCou
 func (m *meter) Int64Histogram(name string, options ...metric.Int64HistogramOption) (metric.Int64Histogram, error) {
 	cfg := metric.NewInt64HistogramConfig(options...)
 	const kind = InstrumentKindHistogram
-	return m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Int64ObservableCounter returns a new instrument identified by name and
@ -95,7 +116,7 @@ func (m *meter) Int64ObservableCounter(name string, options ...metric.Int64Obser
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
 }

 // Int64ObservableUpDownCounter returns a new instrument identified by name and
@ -110,7 +131,7 @@ func (m *meter) Int64ObservableUpDownCounter(name string, options ...metric.Int6
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
 }

 // Int64ObservableGauge returns a new instrument identified by name and
@ -125,7 +146,7 @@ func (m *meter) Int64ObservableGauge(name string, options ...metric.Int64Observa
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
 }

 // Float64Counter returns a new instrument identified by name and configured
@ -134,7 +155,12 @@ func (m *meter) Int64ObservableGauge(name string, options ...metric.Int64Observa
 func (m *meter) Float64Counter(name string, options ...metric.Float64CounterOption) (metric.Float64Counter, error) {
 	cfg := metric.NewFloat64CounterConfig(options...)
 	const kind = InstrumentKindCounter
-	return m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Float64UpDownCounter returns a new instrument identified by name and
@ -143,7 +169,12 @@ func (m *meter) Float64Counter(name string, options ...metric.Float64CounterOpti
 func (m *meter) Float64UpDownCounter(name string, options ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) {
 	cfg := metric.NewFloat64UpDownCounterConfig(options...)
 	const kind = InstrumentKindUpDownCounter
-	return m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Float64Histogram returns a new instrument identified by name and configured
@ -152,7 +183,12 @@ func (m *meter) Float64UpDownCounter(name string, options ...metric.Float64UpDow
 func (m *meter) Float64Histogram(name string, options ...metric.Float64HistogramOption) (metric.Float64Histogram, error) {
 	cfg := metric.NewFloat64HistogramConfig(options...)
 	const kind = InstrumentKindHistogram
-	return m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit())
+	if err != nil {
+		return i, err
+	}
+
+	return i, validateInstrumentName(name)
 }

 // Float64ObservableCounter returns a new instrument identified by name and
@ -167,7 +203,7 @@ func (m *meter) Float64ObservableCounter(name string, options ...metric.Float64O
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
 }

 // Float64ObservableUpDownCounter returns a new instrument identified by name
@ -182,7 +218,7 @@ func (m *meter) Float64ObservableUpDownCounter(name string, options ...metric.Fl
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
 }

 // Float64ObservableGauge returns a new instrument identified by name and
@ -197,7 +233,34 @@ func (m *meter) Float64ObservableGauge(name string, options ...metric.Float64Obs
 		return nil, err
 	}
 	p.registerCallbacks(inst, cfg.Callbacks())
-	return inst, nil
+	return inst, validateInstrumentName(name)
+}
+
+func validateInstrumentName(name string) error {
+	if len(name) == 0 {
+		return fmt.Errorf("%w: %s: is empty", ErrInstrumentName, name)
+	}
+	if len(name) > 63 {
+		return fmt.Errorf("%w: %s: longer than 63 characters", ErrInstrumentName, name)
+	}
+	if !isAlpha([]rune(name)[0]) {
+		return fmt.Errorf("%w: %s: must start with a letter", ErrInstrumentName, name)
+	}
+	if len(name) == 1 {
+		return nil
+	}
+	for _, c := range name[1:] {
+		if !isAlphanumeric(c) && c != '_' && c != '.' && c != '-' {
+			return fmt.Errorf("%w: %s: must only contain [A-Za-z0-9_.-]", ErrInstrumentName, name)
+		}
+	}
+	return nil
+}
+func isAlpha(c rune) bool {
+	return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z')
+}
+func isAlphanumeric(c rune) bool {
+	return isAlpha(c) || ('0' <= c && c <= '9')
 }

 // RegisterCallback registers f to be called each collection cycle so it will
--- a/sdk/metric/meter_test.go
+++ b/sdk/metric/meter_test.go
@ -483,6 +483,310 @@ func TestMeterCreatesInstruments(t *testing.T) {
 	}
 }

+func TestMeterCreatesInstrumentsValidations(t *testing.T) {
+	testCases := []struct {
+		name string
+		fn   func(*testing.T, metric.Meter) error
+
+		wantErr error
+	}{
+		{
+			name: "Int64Counter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64Counter("counter")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64Counter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64Counter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Int64UpDownCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64UpDownCounter("upDownCounter")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64UpDownCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64UpDownCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Int64Histogram with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64Histogram("histogram")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64Histogram with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64Histogram("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Int64ObservableCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableCounter("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64ObservableCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Int64ObservableUpDownCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableUpDownCounter("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64ObservableUpDownCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableUpDownCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Int64ObservableGauge with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableGauge("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Int64ObservableGauge with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableGauge("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64Counter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64Counter("counter")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64Counter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64Counter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64UpDownCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64UpDownCounter("upDownCounter")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64UpDownCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64UpDownCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64Histogram with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64Histogram("histogram")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64Histogram with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64Histogram("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64ObservableCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64ObservableCounter("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64ObservableCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Int64ObservableCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64ObservableUpDownCounter with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64ObservableUpDownCounter("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64ObservableUpDownCounter with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64ObservableUpDownCounter("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "Float64ObservableGauge with no validation issues",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64ObservableGauge("aint")
+				assert.NotNil(t, i)
+				return err
+			},
+		},
+		{
+			name: "Float64ObservableGauge with an invalid name",
+
+			fn: func(t *testing.T, m metric.Meter) error {
+				i, err := m.Float64ObservableGauge("_")
+				assert.NotNil(t, i)
+				return err
+			},
+
+			wantErr: fmt.Errorf("%w: _: must start with a letter", ErrInstrumentName),
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			m := NewMeterProvider().Meter("testInstruments")
+			err := tt.fn(t, m)
+			assert.Equal(t, err, tt.wantErr)
+		})
+	}
+}
+
+func TestValidateInstrumentName(t *testing.T) {
+	testCases := []struct {
+		name string
+
+		wantErr error
+	}{
+		{
+			name:    "",
+			wantErr: fmt.Errorf("%w: : is empty", ErrInstrumentName),
+		},
+		{
+			name:    "1",
+			wantErr: fmt.Errorf("%w: 1: must start with a letter", ErrInstrumentName),
+		},
+		{
+			name: "a",
+		},
+		{
+			name: "n4me",
+		},
+		{
+			name: "n-me",
+		},
+		{
+			name: "na_e",
+		},
+		{
+			name: "nam.",
+		},
+		{
+			name:    "name!",
+			wantErr: fmt.Errorf("%w: name!: must only contain [A-Za-z0-9_.-]", ErrInstrumentName),
+		},
+		{
+			name:    "someverylongnamewhichisover63charactersbutallofwhichmatchtheregexp",
+			wantErr: fmt.Errorf("%w: someverylongnamewhichisover63charactersbutallofwhichmatchtheregexp: longer than 63 characters", ErrInstrumentName),
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.wantErr, validateInstrumentName(tt.name))
+		})
+	}
+}
+
 func TestRegisterNonSDKObserverErrors(t *testing.T) {
 	rdr := NewManualReader()
 	mp := NewMeterProvider(WithReader(rdr))
@ -509,9 +813,9 @@ func TestMeterMixingOnRegisterErrors(t *testing.T) {

 	m1 := mp.Meter("scope1")
 	m2 := mp.Meter("scope2")
-	iCtr, err := m2.Int64ObservableCounter("int64 ctr")
+	iCtr, err := m2.Int64ObservableCounter("int64ctr")
 	require.NoError(t, err)
-	fCtr, err := m2.Float64ObservableCounter("float64 ctr")
+	fCtr, err := m2.Float64ObservableCounter("float64ctr")
 	require.NoError(t, err)
 	_, err = m1.RegisterCallback(
 		func(context.Context, metric.Observer) error { return nil },
@ -520,13 +824,13 @@ func TestMeterMixingOnRegisterErrors(t *testing.T) {
 	assert.ErrorContains(
 		t,
 		err,
-		`invalid registration: observable "int64 ctr" from Meter "scope2", registered with Meter "scope1"`,
+		`invalid registration: observable "int64ctr" from Meter "scope2", registered with Meter "scope1"`,
 		"Instrument registered with non-creation Meter",
 	)
 	assert.ErrorContains(
 		t,
 		err,
-		`invalid registration: observable "float64 ctr" from Meter "scope2", registered with Meter "scope1"`,
+		`invalid registration: observable "float64ctr" from Meter "scope2", registered with Meter "scope1"`,
 		"Instrument registered with non-creation Meter",
 	)
 }
@ -540,9 +844,9 @@ func TestCallbackObserverNonRegistered(t *testing.T) {
 	require.NoError(t, err)

 	m2 := mp.Meter("scope2")
-	iCtr, err := m2.Int64ObservableCounter("int64 ctr")
+	iCtr, err := m2.Int64ObservableCounter("int64ctr")
 	require.NoError(t, err)
-	fCtr, err := m2.Float64ObservableCounter("float64 ctr")
+	fCtr, err := m2.Float64ObservableCounter("float64ctr")
 	require.NoError(t, err)

 	type int64Obsrv struct{ metric.Int64Observable }