gVisor metric library: Change interface for passing in field values.

This introduces a `metric.FieldValue` struct type that wraps a string. All metric interfaces that deal with field values have been updated to use pointers to this type instead of strings. The intent of this change is to make it more obvious that field values must be passed using references. Prior to this change, this was done using string pointer comparisons. Now this must be done by using a pointer to the same `metric.FieldValue` struct. The struct type still externally exposes its string so that it can be referred to in value function callbacks by "custom" metrics. (Though there are no current uses of callback metrics with fields.) PiperOrigin-RevId: 527030738
2026-05-22 17:12:49 -07:00 · 2023-04-25 11:49:02 -07:00
parent ff4f0b9fc5
commit a938259779
14 changed files with 251 additions and 213 deletions
@@ -35,37 +35,37 @@ type FakeTimedOperation struct{}
 // Value from a FakeUint64Metric always returns a meaningless value.
 //
 //go:nosplit
-func (m *FakeUint64Metric) Value(fieldValues ...string) uint64 {
+func (m *FakeUint64Metric) Value(fieldValues ...*FieldValue) uint64 {
 	return 0
 }

 // Increment on a FakeUint64Metric does nothing.
 //
 //go:nosplit
-func (m *FakeUint64Metric) Increment(fieldValues ...string) {}
+func (m *FakeUint64Metric) Increment(fieldValues ...*FieldValue) {}

 // IncrementBy on a FakeUint64Metric does nothing.
 //
 //go:nosplit
-func (m *FakeUint64Metric) IncrementBy(v uint64, fieldValues ...string) {}
+func (m *FakeUint64Metric) IncrementBy(v uint64, fieldValues ...*FieldValue) {}

 // AddSample on a FakeUint64Metric does nothing.
 //
 //go:nosplit
-func (d *FakeDistributionMetric) AddSample(sample int64, fields ...string) {}
+func (d *FakeDistributionMetric) AddSample(sample int64, fields ...*FieldValue) {}

 // Start on a FakeUint64Metric returns a FakeTimedOperation struct, which does
 // nothing and does not keep the time.
 //
 //go:nosplit
-func (t *FakeTimerMetric) Start(fields ...string) FakeTimedOperation {
+func (t *FakeTimerMetric) Start(fields ...*FieldValue) FakeTimedOperation {
 	return FakeTimedOperation{}
 }

 // Finish on a FakeTimedOperation does nothing.
 //
 //go:nosplit
-func (o FakeTimedOperation) Finish(extraFields ...string) {}
+func (o FakeTimedOperation) Finish(extraFields ...*FieldValue) {}

 // NewFakeUint64Metric is equivalent to NewUint64Metric except it creates a
 // FakeUint64Metric
@@ -55,17 +55,17 @@ var (
 )

 // Weirdness metric type constants.
-const (
-	WeirdnessTypeTimeFallback         = "time_fallback"
-	WeirdnessTypePartialResult        = "partial_result"
-	WeirdnessTypeVsyscallCount        = "vsyscall_count"
-	WeirdnessTypeWatchdogStuckStartup = "watchdog_stuck_startup"
-	WeirdnessTypeWatchdogStuckTasks   = "watchdog_stuck_tasks"
+var (
+	WeirdnessTypeTimeFallback         = FieldValue{"time_fallback"}
+	WeirdnessTypePartialResult        = FieldValue{"partial_result"}
+	WeirdnessTypeVsyscallCount        = FieldValue{"vsyscall_count"}
+	WeirdnessTypeWatchdogStuckStartup = FieldValue{"watchdog_stuck_startup"}
+	WeirdnessTypeWatchdogStuckTasks   = FieldValue{"watchdog_stuck_tasks"}
 )

 // Suspicious operations metric type constants.
-const (
-	SuspiciousOperationsTypeOpenedWriteExecuteFile = "opened_write_execute_file"
+var (
+	SuspiciousOperationsTypeOpenedWriteExecuteFile = FieldValue{"opened_write_execute_file"}
 )

 // List of global metrics that are used in multiple places.
@@ -74,20 +74,20 @@ var (
 	// of weird occurrences such as time fallback, partial_result, vsyscall
 	// count, watchdog startup timeouts and stuck tasks.
 	WeirdnessMetric = MustCreateNewUint64Metric("/weirdness", true /* sync */, "Increment for weird occurrences of problems such as time fallback, partial result, vsyscalls invoked in the sandbox, watchdog startup timeouts and stuck tasks.",
-		NewField("weirdness_type", []string{
-			WeirdnessTypeTimeFallback,
-			WeirdnessTypePartialResult,
-			WeirdnessTypeVsyscallCount,
-			WeirdnessTypeWatchdogStuckStartup,
-			WeirdnessTypeWatchdogStuckTasks,
-		}))
+		NewField("weirdness_type",
+			&WeirdnessTypeTimeFallback,
+			&WeirdnessTypePartialResult,
+			&WeirdnessTypeVsyscallCount,
+			&WeirdnessTypeWatchdogStuckStartup,
+			&WeirdnessTypeWatchdogStuckTasks,
+		))

 	// SuspiciousOperationsMetric is a metric with fields created to detect
 	// operations such as opening an executable file to write from a gofer.
 	SuspiciousOperationsMetric = MustCreateNewUint64Metric("/suspicious_operations", true /* sync */, "Increment for suspicious operations such as opening an executable file to write from a gofer.",
-		NewField("operation_type", []string{
-			SuspiciousOperationsTypeOpenedWriteExecuteFile,
-		}))
+		NewField("operation_type",
+			&SuspiciousOperationsTypeOpenedWriteExecuteFile,
+		))
 )

 // InitStage is the name of a Sentry initialization stage.
@@ -116,6 +116,8 @@ var (
 //
 // Metrics are not saved across save/restore and thus reset to zero on restore.
 type Uint64Metric struct {
+	name string
+
 	// fields is the map of field-value combination index keys to Uint64 counters.
 	fields []atomicbitops.Uint64

@@ -210,9 +212,12 @@ type customUint64Metric struct {
 	// prometheusMetric describes the metric in Prometheus format. It is immutable.
 	prometheusMetric *prometheus.Metric

+	// fields is the set of fields of the metric.
+	fields []Field
+
 	// value returns the current value of the metric for the given set of
 	// fields. It takes a variadic number of field values as argument.
-	value func(fieldValues ...string) uint64
+	value func(fieldValues ...*FieldValue) uint64

 	// forEachNonZero calls the given function on each possible field value of
 	// the metric where the metric's value is non-zero.
@@ -221,7 +226,16 @@ type customUint64Metric struct {
 	// `forEachNonZero` does not guarantee that it will be called on a
 	// consistent snapshot of this metric's values.
 	// `forEachNonZero` may be nil.
-	forEachNonZero func(f func(fields []string, val uint64))
+	forEachNonZero func(f func(fields []*FieldValue, val uint64))
+}
+
+// FieldValue is a string that can be used as a value for a Field.
+// It must be referred to by address when the Field is created and when its
+// metric value is modified. This ensures that the same FieldValue reference
+// is used, which in turn enables the metric code to use the address of a
+// FieldValue as comparison operator, rather than doing string comparisons.
+type FieldValue struct {
+	Value string
 }

 // fieldMapperMapThreshold is the number of field values after which we switch
@@ -239,23 +253,61 @@ type Field struct {
 	// `values` is always populated but not always used for lookup. It depends
 	// on the number of allowed field values. `values` is used for lookups on
 	// fields with small numbers of field values.
-	values []string
+	values []*FieldValue

-	// valuesPtrMap is a map version of `values`. For each string in `values`,
-	// its underlying byte string pointer is mapped to its index in `values`.
+	// valuesPtrMap is a map version of `values`. For each item in `values`,
+	// its pointer is mapped to its index within `values`.
 	// `valuesPtrMap` is used for fields with large numbers of possible values.
 	// For fields with small numbers of field values, it is nil.
 	// This map allows doing faster string matching than a normal string map,
 	// as it avoids the string hashing step that normal string maps need to do.
-	valuesPtrMap map[*byte]int
+	valuesPtrMap map[*FieldValue]int
 }

 // toProto returns the proto definition of this field, for use in metric
 // metadata.
 func (f Field) toProto() *pb.MetricMetadata_Field {
+	allowedValues := make([]string, len(f.values))
+	for i, v := range f.values {
+		allowedValues[i] = v.Value
+	}
 	return &pb.MetricMetadata_Field{
 		FieldName:     f.name,
-		AllowedValues: f.values,
+		AllowedValues: allowedValues,
+	}
+}
+
+// NewField defines a new Field that can be used to break down a metric.
+// The set of allowedValues must be unique strings wrapped with `FieldValue`.
+// The *same* `FieldValue` pointers must be used during metric modifications.
+// In practice, in most cases, this means you should declare these
+// `FieldValue`s as package-level `var`s, and always use the address of these
+// package-level `var`s during metric modifications.
+func NewField(name string, allowedValues ...*FieldValue) Field {
+	// Verify that all string values have a unique value.
+	strMap := make(map[string]bool, len(allowedValues))
+	for _, v := range allowedValues {
+		if strMap[v.Value] {
+			panic(fmt.Sprintf("found duplicate field value: %q", v))
+		}
+		strMap[v.Value] = true
+	}
+
+	if useMap := len(allowedValues) > fieldMapperMapThreshold; !useMap {
+		return Field{
+			name:   name,
+			values: allowedValues,
+		}
+	}
+
+	valuesPtrMap := make(map[*FieldValue]int, len(allowedValues))
+	for i, v := range allowedValues {
+		valuesPtrMap[v] = i
+	}
+	return Field{
+		name:         name,
+		values:       allowedValues,
+		valuesPtrMap: valuesPtrMap,
 	}
 }

@@ -296,6 +348,44 @@ func newFieldMapper(fields ...Field) (fieldMapper, error) {
 	}, nil
 }

+// lookupSingle looks up a single key for a single field within fieldMapper.
+// It is used internally within lookupConcat.
+// It returns the updated `idx` and `remainingCombinationBucket` values.
+// +checkescape:all
+//
+//go:nosplit
+func (m fieldMapper) lookupSingle(fieldIndex int, fieldValue *FieldValue, idx, remainingCombinationBucket int) (int, int) {
+	field := m.fields[fieldIndex]
+	numValues := len(field.values)
+
+	// Are we doing a linear search?
+	if field.valuesPtrMap == nil {
+		// We scan by pointers only. This means the caller must pass the same
+		// FieldValue pointer as the one used in `NewField`.
+		for valIdx, allowedVal := range field.values {
+			if fieldValue == allowedVal {
+				remainingCombinationBucket /= numValues
+				idx += remainingCombinationBucket * valIdx
+				return idx, remainingCombinationBucket
+			}
+		}
+		panic("invalid field value or did not reuse the same FieldValue pointer as passed in NewField")
+	}
+
+	// Use map lookup instead.
+
+	// Match using FieldValue pointer.
+	// This avoids the string hashing step that string maps otherwise do.
+	valIdx, found := field.valuesPtrMap[fieldValue]
+	if found {
+		remainingCombinationBucket /= numValues
+		idx += remainingCombinationBucket * valIdx
+		return idx, remainingCombinationBucket
+	}
+
+	panic("invalid field value or did not reuse the same FieldValue pointer as passed in NewField")
+}
+
 // lookupConcat looks up a key within the fieldMapper where the fields are
 // the concatenation of two list of fields.
 // The returned key is an index that can be used to access to map created by
@@ -304,7 +394,7 @@ func newFieldMapper(fields ...Field) (fieldMapper, error) {
 // +checkescape:all
 //
 //go:nosplit
-func (m fieldMapper) lookupConcat(fields1, fields2 []string) int {
+func (m fieldMapper) lookupConcat(fields1, fields2 []*FieldValue) int {
 	if (len(fields1) + len(fields2)) != len(m.fields) {
 		panic("invalid field lookup depth")
 	}
@@ -329,7 +419,7 @@ func (m fieldMapper) lookupConcat(fields1, fields2 []string) int {
 // +checkescape:all
 //
 //go:nosplit
-func (m fieldMapper) lookup(fields ...string) int {
+func (m fieldMapper) lookup(fields ...*FieldValue) int {
 	return m.lookupConcat(fields, nil)
 }

@@ -363,16 +453,21 @@ func (m fieldMapper) keyToMultiField(key int) []string {
 		return nil
 	}
 	fieldValues := make([]string, depth)
-	m.keyToMultiFieldInPlace(key, fieldValues)
+	remainingCombinationBucket := m.numFieldCombinations
+	for i := 0; i < depth; i++ {
+		remainingCombinationBucket /= len(m.fields[i].values)
+		fieldValues[i] = m.fields[i].values[key/remainingCombinationBucket].Value
+		key = key % remainingCombinationBucket
+	}
 	return fieldValues
 }

 // keyToMultiFieldInPlace does the operation described in `keyToMultiField`
-// but modifies `fieldValues` in-place. It must aready be of size
+// but modifies `fieldValues` in-place. It must already be of size
 // `len(m.fields)`.
 //
 //go:nosplit
-func (m fieldMapper) keyToMultiFieldInPlace(key int, fieldValues []string) {
+func (m fieldMapper) keyToMultiFieldInPlace(key int, fieldValues []*FieldValue) {
 	if len(m.fields) == 0 {
 		return
 	}
@@ -400,7 +495,7 @@ func nameToPrometheusName(name string) string {
 //   - name must be globally unique.
 //   - Initialize/Disable have not been called.
 //   - value is expected to accept exactly len(fields) arguments.
-func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.MetricMetadata_Units, description string, value func(...string) uint64, fields ...Field) error {
+func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.MetricMetadata_Units, description string, value func(...*FieldValue) uint64, fields ...Field) error {
 	if initialized.Load() {
 		return ErrInitializationDone
 	}
@@ -432,7 +527,8 @@ func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.Met
 			Help: description,
 			Type: promType,
 		},
-		value: value,
+		fields: fields,
+		value:  value,
 	}

 	// Metrics can exist without fields.
@@ -448,7 +544,7 @@ func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.Met

 // MustRegisterCustomUint64Metric calls RegisterCustomUint64Metric for metrics
 // without fields and panics if it returns an error.
-func MustRegisterCustomUint64Metric(name string, cumulative, sync bool, description string, value func(...string) uint64, fields ...Field) {
+func MustRegisterCustomUint64Metric(name string, cumulative, sync bool, description string, value func(...*FieldValue) uint64, fields ...Field) {
 	if err := RegisterCustomUint64Metric(name, cumulative, sync, pb.MetricMetadata_UNITS_NONE, description, value, fields...); err != nil {
 		panic(fmt.Sprintf("Unable to register metric %q: %s", name, err))
 	}
@@ -464,6 +560,7 @@ func NewUint64Metric(name string, sync bool, units pb.MetricMetadata_Units, desc
 		return nil, err
 	}
 	m := Uint64Metric{
+		name:        name,
 		fieldMapper: f,
 		fields:      make([]atomicbitops.Uint64, f.numKeys()),
 	}
@@ -500,14 +597,14 @@ func MustCreateNewUint64NanosecondsMetric(name string, sync bool, description st
 // This must be called with the correct number of field values or it will panic.
 //
 //go:nosplit
-func (m *Uint64Metric) Value(fieldValues ...string) uint64 {
+func (m *Uint64Metric) Value(fieldValues ...*FieldValue) uint64 {
 	key := m.fieldMapper.lookupConcat(fieldValues, nil)
 	return m.fields[key].Load()
 }

 // forEachNonZero iterates over each field combination and calls the given
 // function whenever this metric's value is not zero.
-func (m *Uint64Metric) forEachNonZero(f func(fieldValues []string, value uint64)) {
+func (m *Uint64Metric) forEachNonZero(f func(fieldValues []*FieldValue, value uint64)) {
 	numCombinations := m.fieldMapper.numKeys()
 	if len(m.fieldMapper.fields) == 0 {
 		// Special-case the "there are no fields" case for speed and to avoid
@@ -517,14 +614,14 @@ func (m *Uint64Metric) forEachNonZero(f func(fieldValues []string, value uint64)
 		}
 		return
 	}
-	var fieldValues []string
+	var fieldValues []*FieldValue
 	for k := 0; k < numCombinations; k++ {
 		val := m.fields[k].Load()
 		if val == 0 {
 			continue
 		}
 		if fieldValues == nil {
-			fieldValues = make([]string, len(m.fieldMapper.fields))
+			fieldValues = make([]*FieldValue, len(m.fieldMapper.fields))
 		}
 		m.fieldMapper.keyToMultiFieldInPlace(k, fieldValues)
 		f(fieldValues, val)
@@ -535,7 +632,7 @@ func (m *Uint64Metric) forEachNonZero(f func(fieldValues []string, value uint64)
 // This must be called with the correct number of field values or it will panic.
 //
 //go:nosplit
-func (m *Uint64Metric) Increment(fieldValues ...string) {
+func (m *Uint64Metric) Increment(fieldValues ...*FieldValue) {
 	key := m.fieldMapper.lookupConcat(fieldValues, nil)
 	m.fields[key].Add(1)
 }
@@ -544,7 +641,7 @@ func (m *Uint64Metric) Increment(fieldValues ...string) {
 // This must be called with the correct number of field values or it will panic.
 //
 //go:nosplit
-func (m *Uint64Metric) IncrementBy(v uint64, fieldValues ...string) {
+func (m *Uint64Metric) IncrementBy(v uint64, fieldValues ...*FieldValue) {
 	key := m.fieldMapper.lookupConcat(fieldValues, nil)
 	m.fields[key].Add(v)
 }
@@ -806,7 +903,7 @@ func MustCreateNewDistributionMetric(name string, sync bool, bucketer Bucketer,
 // +checkescape:all
 //
 //go:nosplit
-func (d *DistributionMetric) AddSample(sample int64, fields ...string) {
+func (d *DistributionMetric) AddSample(sample int64, fields ...*FieldValue) {
 	d.addSampleByKey(sample, d.fieldsToKey.lookup(fields...))
 }

@@ -878,7 +975,7 @@ type TimedOperation struct {

 	// partialFields is a prefix of the fields used in this operation.
 	// The rest of the fields is provided in TimedOperation.Finish.
-	partialFields []string
+	partialFields []*FieldValue

 	// startedNs is the number of nanoseconds measured in TimerMetric.Start().
 	startedNs int64
@@ -895,7 +992,7 @@ type TimedOperation struct {
 // +checkescape:all
 //
 //go:nosplit
-func (t *TimerMetric) Start(fields ...string) TimedOperation {
+func (t *TimerMetric) Start(fields ...*FieldValue) TimedOperation {
 	return TimedOperation{
 		metric:        t,
 		partialFields: fields,
@@ -910,7 +1007,7 @@ func (t *TimerMetric) Start(fields ...string) TimedOperation {
 // +checkescape:all
 //
 //go:nosplit
-func (o TimedOperation) Finish(extraFields ...string) {
+func (o TimedOperation) Finish(extraFields ...*FieldValue) {
 	ended := CheapNowNano()
 	fieldKey := o.metric.fieldsToKey.lookupConcat(o.partialFields, extraFields)
 	o.metric.addSampleByKey(ended-o.startedNs, fieldKey)
@@ -974,19 +1071,18 @@ func (m *metricSet) Values() metricValues {
 		stages:                   stages,
 	}
 	for k, v := range m.uint64Metrics {
-		fields := v.metadata.GetFields()
+		fields := v.fields
 		switch len(fields) {
 		case 0:
 			vals.uint64Metrics[k] = v.value()
 		case 1:
-			fieldsMap := make(map[string]uint64)
+			fieldsMap := make(map[*FieldValue]uint64)
 			if v.forEachNonZero != nil {
-				v.forEachNonZero(func(fieldValues []string, val uint64) {
+				v.forEachNonZero(func(fieldValues []*FieldValue, val uint64) {
 					fieldsMap[fieldValues[0]] = val
 				})
 			} else {
-				values := fields[0].GetAllowedValues()
-				for _, fieldValue := range values {
+				for _, fieldValue := range fields[0].values {
 					fieldsMap[fieldValue] = v.value(fieldValue)
 				}
 			}
@@ -1028,7 +1124,7 @@ func (m *metricSet) Values() metricValues {
 // metricValues contains a copy of the values of all metrics.
 type metricValues struct {
 	// uint64Metrics is a map of uint64 metrics,
-	// with key as metric name. Value can be either uint64, or map[string]uint64
+	// with key as metric name. Value can be either uint64, or map[*FieldValue]uint64
 	// to support metrics with one field.
 	uint64Metrics map[string]any

@@ -1100,22 +1196,24 @@ func EmitMetricUpdate() {
 				Name:  k,
 				Value: &pb.MetricValue_Uint64Value{Uint64Value: t},
 			})
-		case map[string]uint64:
+		case map[*FieldValue]uint64:
 			for fieldValue, metricValue := range t {
 				// Emit data on the first call only if the field
 				// value has been incremented. For all other
 				// calls, emit data if the field value has been
 				// changed from the previous emit.
-				if (!ok && metricValue == 0) || (ok && prev.(map[string]uint64)[fieldValue] == metricValue) {
+				if (!ok && metricValue == 0) || (ok && prev.(map[*FieldValue]uint64)[fieldValue] == metricValue) {
 					continue
 				}

 				m.Metrics = append(m.Metrics, &pb.MetricValue{
 					Name:        k,
-					FieldValues: []string{fieldValue},
+					FieldValues: []string{fieldValue.Value},
 					Value:       &pb.MetricValue_Uint64Value{Uint64Value: metricValue},
 				})
 			}
+		default:
+			panic(fmt.Sprintf("unsupported type in uint64Metrics: %T (%v)", v, v))
 		}
 	}
 	for name, dist := range snapshot.distributionTotalSamples {
@@ -1222,7 +1320,7 @@ func GetSnapshot(options SnapshotOptions) (*prometheus.Snapshot, error) {
 				continue
 			}
 			snapshot.Add(prometheus.NewIntData(m.prometheusMetric, int64(t)))
-		case map[string]uint64:
+		case map[*FieldValue]uint64:
 			for fieldValue, metricValue := range t {
 				if m.metadata.GetCumulative() && metricValue == 0 {
 					// Zero-valued counter, ignore.
@@ -1230,9 +1328,11 @@ func GetSnapshot(options SnapshotOptions) (*prometheus.Snapshot, error) {
 				}
 				snapshot.Add(prometheus.LabeledIntData(m.prometheusMetric, map[string]string{
 					// uint64 metrics currently only support at most one field name.
-					m.metadata.Fields[0].GetFieldName(): fieldValue,
+					m.metadata.Fields[0].GetFieldName(): fieldValue.Value,
 				}, int64(metricValue)))
 			}
+		default:
+			panic(fmt.Sprintf("unsupported type in uint64Metrics: %T (%v)", v, v))
 		}
 	}
 	for k, dists := range values.distributionTotalSamples {
@@ -36,6 +36,13 @@ const (
 	distribDescription = "A distribution metric for testing"
 )

+var (
+	fieldValFoo  = FieldValue{"foo"}
+	fieldValBar  = FieldValue{"bar"}
+	fieldValBaz  = FieldValue{"baz"}
+	fieldValQuux = FieldValue{"quux"}
+)
+
 // Helper method that exercises Prometheus metric exporting.
 // Ensures that current metric data, if it were to be exported and formatted as Prometheus format,
 // would be successfully parsable by the reference Prometheus implementation.
@@ -73,8 +80,8 @@ func TestInitialize(t *testing.T) {
 	}

 	bucketer := NewExponentialBucketer(3, 2, 0, 1)
-	field1 := NewField("field1", []string{"foo", "bar"})
-	field2 := NewField("field2", []string{"baz", "quux"})
+	field1 := NewField("field1", &fieldValFoo, &fieldValBar)
+	field2 := NewField("field2", &fieldValBaz, &fieldValQuux)
 	_, err = NewDistributionMetric("/distrib", true, bucketer, pb.MetricMetadata_UNITS_NANOSECONDS, distribDescription, field1, field2)
 	if err != nil {
 		t.Fatalf("NewDistributionMetric got err %v want nil", err)
@@ -220,8 +227,8 @@ func TestEmitMetricUpdate(t *testing.T) {
 	}

 	bucketer := NewExponentialBucketer(2, 2, 0, 1)
-	field1 := NewField("field1", []string{"foo", "bar"})
-	field2 := NewField("field2", []string{"baz", "quux"})
+	field1 := NewField("field1", &fieldValFoo, &fieldValBar)
+	field2 := NewField("field2", &fieldValBaz, &fieldValQuux)
 	distrib, err := NewDistributionMetric("/distrib", false, bucketer, pb.MetricMetadata_UNITS_NONE, distribDescription, field1, field2)
 	if err != nil {
 		t.Fatalf("NewDistributionMetric: %v", err)
@@ -318,12 +325,12 @@ func TestEmitMetricUpdate(t *testing.T) {
 	verifyPrometheusParsing(t)

 	// Add a few samples to the distribution metric.
-	distrib.AddSample(1, "foo", "baz")
-	distrib.AddSample(1, "foo", "baz")
-	distrib.AddSample(3, "foo", "baz")
-	distrib.AddSample(-1, "foo", "quux")
-	distrib.AddSample(1, "foo", "quux")
-	distrib.AddSample(100, "foo", "quux")
+	distrib.AddSample(1, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(1, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(3, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(-1, &fieldValFoo, &fieldValQuux)
+	distrib.AddSample(1, &fieldValFoo, &fieldValQuux)
+	distrib.AddSample(100, &fieldValFoo, &fieldValQuux)
 	emitter.Reset()
 	EmitMetricUpdate()
 	if len(emitter) != 1 {
@@ -372,10 +379,10 @@ func TestEmitMetricUpdate(t *testing.T) {
 	verifyPrometheusParsing(t)

 	// Add more samples to the distribution metric, check that we get the delta.
-	distrib.AddSample(3, "foo", "baz")
-	distrib.AddSample(2, "foo", "baz")
-	distrib.AddSample(1, "foo", "baz")
-	distrib.AddSample(3, "foo", "baz")
+	distrib.AddSample(3, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(2, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(1, &fieldValFoo, &fieldValBaz)
+	distrib.AddSample(3, &fieldValFoo, &fieldValBaz)
 	emitter.Reset()
 	EmitMetricUpdate()
 	if len(emitter) != 1 {
@@ -409,11 +416,11 @@ func TestEmitMetricUpdate(t *testing.T) {
 func TestEmitMetricUpdateWithFields(t *testing.T) {
 	defer resetTest()

-	const (
-		weird1 = "weird1"
-		weird2 = "weird2"
+	var (
+		weird1 = FieldValue{"weird1"}
+		weird2 = FieldValue{"weird2"}
 	)
-	field := NewField("weirdness_type", []string{weird1, weird2})
+	field := NewField("weirdness_type", &weird1, &weird2)

 	counter, err := NewUint64Metric("/weirdness", false, pb.MetricMetadata_UNITS_NONE, counterDescription, field)
 	if err != nil {
@@ -436,8 +443,8 @@ func TestEmitMetricUpdateWithFields(t *testing.T) {
 	}
 	verifyPrometheusParsing(t)

-	counter.IncrementBy(4, weird1)
-	counter.Increment(weird2)
+	counter.IncrementBy(4, &weird1)
+	counter.Increment(&weird2)

 	emitter.Reset()
 	EmitMetricUpdate()
@@ -468,7 +475,7 @@ func TestEmitMetricUpdateWithFields(t *testing.T) {
 		}

 		switch m.FieldValues[0] {
-		case weird1:
+		case weird1.Value:
 			uv, ok := m.Value.(*pb.MetricValue_Uint64Value)
 			if !ok {
 				t.Errorf("%+v: value %v got %T want pb.MetricValue_Uint64Value", m, m.Value, m.Value)
@@ -477,7 +484,7 @@ func TestEmitMetricUpdateWithFields(t *testing.T) {
 				t.Errorf("%v: Value got %v want 4", m, uv.Uint64Value)
 			}
 			foundWeird1 = true
-		case weird2:
+		case weird2.Value:
 			uv, ok := m.Value.(*pb.MetricValue_Uint64Value)
 			if !ok {
 				t.Errorf("%+v: value %v got %T want pb.MetricValue_Uint64Value", m, m.Value, m.Value)
@@ -652,8 +659,8 @@ func TestTimerMetric(t *testing.T) {
 	defer resetTest()
 	// This bucketer just has 2 finite buckets: [0, 500ms) and [500ms, 1s).
 	bucketer := NewExponentialBucketer(2, uint64((500 * time.Millisecond).Nanoseconds()), 0, 1)
-	field1 := NewField("field1", []string{"foo", "bar"})
-	field2 := NewField("field2", []string{"baz", "quux"})
+	field1 := NewField("field1", &fieldValFoo, &fieldValBar)
+	field2 := NewField("field2", &fieldValBaz, &fieldValQuux)
 	timer, err := NewTimerMetric("/timer", bucketer, "a timer metric", field1, field2)
 	if err != nil {
 		t.Fatalf("NewTimerMetric: %v", err)
@@ -670,8 +677,8 @@ func TestTimerMetric(t *testing.T) {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			op := timer.Start("foo")
-			defer op.Finish("quux")
+			op := timer.Start(&fieldValFoo)
+			defer op.Finish(&fieldValQuux)
 			time.Sleep(250 * time.Millisecond)
 		}()
 	}
@@ -680,7 +687,7 @@ func TestTimerMetric(t *testing.T) {
 		go func() {
 			defer wg.Done()
 			op := timer.Start()
-			defer op.Finish("foo", "quux")
+			defer op.Finish(&fieldValFoo, &fieldValQuux)
 			time.Sleep(750 * time.Millisecond)
 		}()
 	}
@@ -862,11 +869,11 @@ func TestFieldMapperWithFields(t *testing.T) {
 		fields := make([]Field, len(fieldSizes))
 		for i, fieldSize := range fieldSizes {
 			fieldName := fmt.Sprintf("%c", 'A'+i)
-			allowedValues := make([]string, fieldSize)
+			allowedValues := make([]*FieldValue, fieldSize)
 			for val := range allowedValues {
-				allowedValues[val] = fmt.Sprintf("%s%d", fieldName, val)
+				allowedValues[val] = &FieldValue{fmt.Sprintf("%s%d", fieldName, val)}
 			}
-			fields[i] = NewField(fieldName, allowedValues)
+			fields[i] = NewField(fieldName, allowedValues...)
 		}
 		return fields
 	}
@@ -893,7 +900,7 @@ func TestFieldMapperWithFields(t *testing.T) {
 		},
 		{
 			name:          "FieldMapperErrNoAllowedValues",
-			fields:        []Field{NewField("TheNoValuesField", []string{})},
+			fields:        []Field{NewField("TheNoValuesField")},
 			errOnCreation: ErrFieldHasNoAllowedValues,
 		},
 	} {
@@ -905,8 +912,8 @@ func TestFieldMapperWithFields(t *testing.T) {

 			// Test that every field value combination corresponds to just one entry.
 			mapping := make([]int, m.numKeys())
-			var visitCombinations func(curFields []string, remFields []Field)
-			visitCombinations = func(curFields []string, remFields []Field) {
+			var visitCombinations func(curFields []*FieldValue, remFields []Field)
+			visitCombinations = func(curFields []*FieldValue, remFields []Field) {
 				depth := len(remFields)
 				if depth == 0 {
 					return
@@ -920,7 +927,7 @@ func TestFieldMapperWithFields(t *testing.T) {
 						// Assert that the reverse operation is also correct.
 						fields2 := m.keyToMultiField(key)
 						for i, f1val := range fields {
-							if f1val != fields2[i] {
+							if f1val.Value != fields2[i] {
 								t.Errorf("Field values put into the map are not the same as ones returned: got %v wanted %v", fields2, f1val)
 							}
 						}
@@ -958,32 +965,31 @@ func TestFieldMapperNoFields(t *testing.T) {
 	}
 }

-func TestFieldPointerUniqueness(t *testing.T) {
-	foobar := "foobar"
-	foo := foobar[:3]
+func TestFieldValueUniqueness(t *testing.T) {
 	panicked := false
 	func() {
 		defer func() {
 			recover()
 			panicked = true
 		}()
-		NewField("field1", []string{foobar, foo})
+		NewField("field1", &FieldValue{"foo"}, &FieldValue{"foo"})
 	}()
 	if !panicked {
 		t.Error("did not panic")
 	}
 }

-func TestFieldMapperMustUseSamePointerString(t *testing.T) {
-	const constFoo = "foo"
-	heapBar := fmt.Sprintf("%sr", "ba")
-	n, err := newFieldMapper(NewField("field1", []string{constFoo, heapBar}))
+func TestFieldMapperMustUseSameValuePointer(t *testing.T) {
+	const fooString = "foo"
+	var constFoo = FieldValue{fooString}
+	var heapBar = &FieldValue{fmt.Sprintf("%sr", "ba")}
+	n, err := newFieldMapper(NewField("field1", &constFoo, heapBar))
 	if err != nil {
 		t.Fatalf("newFieldMapper err: got %v wanted nil", err)
 	}
-	n.lookup(constFoo)
+	n.lookup(&constFoo)
 	n.lookup(heapBar)
-	newFoo := fmt.Sprintf("%so", "fo")
+	newFoo := &FieldValue{fmt.Sprintf("%so", "fo")}
 	panicked := false
 	func() {
 		defer func() {
@@ -15,7 +15,6 @@
 package metric

 import (
-	"fmt"
 	"unsafe"

 	"gvisor.dev/gvisor/pkg/atomicbitops"
@@ -53,79 +52,3 @@ func snapshotDistribution(samples []atomicbitops.Uint64) []uint64 {
 func CheapNowNano() int64 {
 	return gohacks.Nanotime()
 }
-
-// NewField defines a new Field that can be used to break down a metric.
-// The set of allowedValues must have unique string pointers (i.e. one cannot
-// be a prefix of another from the same underlying byte slice).
-// The *same* string pointers must be used during metric modifications.
-// In practice, in most cases, this means you should declare these strings as
-// `const`s, and always use these `const` strings during metric modifications.
-func NewField(name string, allowedValues []string) Field {
-	// Verify that all string values have a unique pointer.
-	// We do this because we try to match strings by pointer matching first,
-	// as this will work in pretty much all cases.
-	ptrMap := make(map[uintptr]string, len(allowedValues))
-	for _, v := range allowedValues {
-		ptr := uintptr(unsafe.Pointer(unsafe.StringData(v)))
-		if duplicate, found := ptrMap[ptr]; found {
-			panic(fmt.Sprintf("found duplicate string values: %q vs %q", v, duplicate))
-		}
-		ptrMap[ptr] = v
-	}
-
-	if useMap := len(allowedValues) > fieldMapperMapThreshold; !useMap {
-		return Field{
-			name:   name,
-			values: allowedValues,
-		}
-	}
-
-	valuesPtrMap := make(map[*byte]int, len(allowedValues))
-	for i, v := range allowedValues {
-		valuesPtrMap[unsafe.StringData(v)] = i
-	}
-	return Field{
-		name:         name,
-		values:       allowedValues,
-		valuesPtrMap: valuesPtrMap,
-	}
-}
-
-// lookupSingle looks up a single key for a single field within fieldMapper.
-// It is used internally within lookupConcat.
-// It returns the updated `idx` and `remainingCombinationBucket` values.
-// +checkescape:all
-//
-//go:nosplit
-func (m fieldMapper) lookupSingle(fieldIndex int, fieldValue string, idx, remainingCombinationBucket int) (int, int) {
-	field := m.fields[fieldIndex]
-	numValues := len(field.values)
-	fieldValPtr := unsafe.StringData(fieldValue)
-
-	// Are we doing a linear search?
-	if field.valuesPtrMap == nil {
-		// We scan by pointers only. This means the caller must pass the same
-		// string as the one used in `NewField`.
-		for valIdx, allowedVal := range field.values {
-			if fieldValPtr == unsafe.StringData(allowedVal) {
-				remainingCombinationBucket /= numValues
-				idx += remainingCombinationBucket * valIdx
-				return idx, remainingCombinationBucket
-			}
-		}
-		panic("invalid field value or did not reuse the same string pointer as passed in NewField")
-	}
-
-	// Use map lookup instead.
-
-	// Match using the raw byte pointer of the string.
-	// This avoids the string hashing step that string maps otherwise do.
-	valIdx, found := field.valuesPtrMap[fieldValPtr]
-	if found {
-		remainingCombinationBucket /= numValues
-		idx += remainingCombinationBucket * valIdx
-		return idx, remainingCombinationBucket
-	}
-
-	panic("invalid field value or did not reuse the same string pointer as passed in NewField")
-}
@@ -58,7 +58,7 @@ func newRegularFileFD(mnt *vfs.Mount, d *dentry, flags uint32) (*regularFileFD,
 		return nil, err
 	}
 	if fd.vfsfd.IsWritable() && (d.mode.Load()&0111 != 0) {
-		metric.SuspiciousOperationsMetric.Increment(metric.SuspiciousOperationsTypeOpenedWriteExecuteFile)
+		metric.SuspiciousOperationsMetric.Increment(&metric.SuspiciousOperationsTypeOpenedWriteExecuteFile)
 	}
 	if d.mmapFD.Load() >= 0 {
 		fsmetric.GoferOpensHost.Increment()
@@ -119,7 +119,7 @@ func newSpecialFileFD(h handle, mnt *vfs.Mount, d *dentry, flags uint32) (*speci
 	d.fs.specialFileFDs.PushBack(fd)
 	d.fs.syncMu.Unlock()
 	if fd.vfsfd.IsWritable() && (d.mode.Load()&0111 != 0) {
-		metric.SuspiciousOperationsMetric.Increment(metric.SuspiciousOperationsTypeOpenedWriteExecuteFile)
+		metric.SuspiciousOperationsMetric.Increment(&metric.SuspiciousOperationsTypeOpenedWriteExecuteFile)
 	}
 	if h.fd >= 0 {
 		fsmetric.GoferOpensHost.Increment()
@@ -39,12 +39,12 @@ const (
 	// LINT.IfChange
 	maxSyscallNum = 2000
 	// LINT.ThenChange(../seccheck/syscall.go)
-
-	// outOfRangeSyscallNumber is used to represent a syscall number that is out of the
-	// range [0, maxSyscallNum] in monitoring.
-	outOfRangeSyscallNumber = "-1"
 )

+// outOfRangeSyscallNumber is used to represent a syscall number that is out of the
+// range [0, maxSyscallNum] in monitoring.
+var outOfRangeSyscallNumber = metric.FieldValue{"-1"}
+
 // SyscallSupportLevel is a syscall support levels.
 type SyscallSupportLevel int

@@ -359,7 +359,7 @@ var (

 	// unimplementedSyscallNumbers maps syscall numbers to their string representation.
 	// Used such that incrementing unimplementedSyscallCounter does not require allocating memory.
-	unimplementedSyscallNumbers map[uintptr]string
+	unimplementedSyscallNumbers map[uintptr]*metric.FieldValue

 	// unimplementedSyscallCounter tracks the number of times each unimplemented syscall has been
 	// called by the sandboxed application.
@@ -391,15 +391,15 @@ func RegisterSyscallTable(s *SyscallTable) {
 	}
 	allSyscallTables = append(allSyscallTables, s)
 	unimplementedSyscallCounterInit.Do(func() {
-		allowedValues := make([]string, maxSyscallNum+2)
-		unimplementedSyscallNumbers = make(map[uintptr]string, len(allowedValues))
+		allowedValues := make([]*metric.FieldValue, maxSyscallNum+2)
+		unimplementedSyscallNumbers = make(map[uintptr]*metric.FieldValue, len(allowedValues))
 		for i := uintptr(0); i <= maxSyscallNum; i++ {
-			s := strconv.Itoa(int(i))
+			s := &metric.FieldValue{strconv.Itoa(int(i))}
 			allowedValues[i] = s
 			unimplementedSyscallNumbers[i] = s
 		}
-		allowedValues[len(allowedValues)-1] = outOfRangeSyscallNumber
-		unimplementedSyscallCounter = metric.MustCreateNewUint64Metric("unimplemented_syscalls", true, "Number of times the application tried to call an unimplemented syscall, broken down by syscall number", metric.NewField("sysno", allowedValues))
+		allowedValues[len(allowedValues)-1] = &outOfRangeSyscallNumber
+		unimplementedSyscallCounter = metric.MustCreateNewUint64Metric("unimplemented_syscalls", true, "Number of times the application tried to call an unimplemented syscall, broken down by syscall number", metric.NewField("sysno", allowedValues...))
 	})
 	s.Init()
 }
@@ -501,7 +501,7 @@ type SyscallInfo struct {
 func IncrementUnimplementedSyscallCounter(sysno uintptr) {
 	s, found := unimplementedSyscallNumbers[sysno]
 	if !found {
-		s = outOfRangeSyscallNumber
+		s = &outOfRangeSyscallNumber
 	}
 	unimplementedSyscallCounter.Increment(s)
 }
@@ -367,7 +367,7 @@ func (*runSyscallExit) execute(t *Task) taskRunState {
 // indicated by an execution fault at address addr. doVsyscall returns the
 // task's next run state.
 func (t *Task) doVsyscall(addr hostarch.Addr, sysno uintptr) taskRunState {
-	metric.WeirdnessMetric.Increment(metric.WeirdnessTypeVsyscallCount)
+	metric.WeirdnessMetric.Increment(&metric.WeirdnessTypeVsyscallCount)

 	// Grab the caller up front, to make sure there's a sensible stack.
 	caller := t.Arch().Native(uintptr(0))
@@ -104,11 +104,11 @@ const (
 )

 // Field values for the get_vcpu metric acquisition path used.
-const (
-	getVCPUAcquisitionFastReused = "fast_reused"
-	getVCPUAcquisitionReused     = "reused"
-	getVCPUAcquisitionUnused     = "unused"
-	getVCPUAcquisitionStolen     = "stolen"
+var (
+	getVCPUAcquisitionFastReused = metric.FieldValue{"fast_reused"}
+	getVCPUAcquisitionReused     = metric.FieldValue{"reused"}
+	getVCPUAcquisitionUnused     = metric.FieldValue{"unused"}
+	getVCPUAcquisitionStolen     = metric.FieldValue{"stolen"}
 )

 var (
@@ -136,7 +136,7 @@ var (
 	// machine.Get() are triggered.
 	getVCPUCounter = metric.MustCreateNewProfilingUint64Metric(
 		"/kvm/get_vcpu", false, "The number of times that machine.Get() was called, split by path the function took.",
-		metric.NewField("acquisition_type", []string{getVCPUAcquisitionFastReused, getVCPUAcquisitionReused, getVCPUAcquisitionUnused, getVCPUAcquisitionStolen}))
+		metric.NewField("acquisition_type", &getVCPUAcquisitionFastReused, &getVCPUAcquisitionReused, &getVCPUAcquisitionUnused, &getVCPUAcquisitionStolen))

 	// asInvalidateDuration are durations of calling addressSpace.invalidate().
 	asInvalidateDuration = metric.MustCreateNewProfilingTimerMetric("/kvm/address_space_invalidate",
@@ -461,7 +461,7 @@ func (m *machine) Get() *vCPU {
 	if c := m.vCPUsByTID[tid]; c != nil {
 		c.lock()
 		m.mu.RUnlock()
-		getVCPUCounter.Increment(getVCPUAcquisitionFastReused)
+		getVCPUCounter.Increment(&getVCPUAcquisitionFastReused)
 		return c
 	}

@@ -481,7 +481,7 @@ func (m *machine) Get() *vCPU {
 	if c := m.vCPUsByTID[tid]; c != nil {
 		c.lock()
 		m.mu.Unlock()
-		getVCPUCounter.Increment(getVCPUAcquisitionReused)
+		getVCPUCounter.Increment(&getVCPUAcquisitionReused)
 		return c
 	}

@@ -494,7 +494,7 @@ func (m *machine) Get() *vCPU {
 			m.vCPUsByTID[tid] = c
 			m.mu.Unlock()
 			c.loadSegments(tid)
-			getVCPUCounter.Increment(getVCPUAcquisitionUnused)
+			getVCPUCounter.Increment(&getVCPUAcquisitionUnused)
 			return c
 		}

@@ -505,7 +505,7 @@ func (m *machine) Get() *vCPU {
 				m.vCPUsByTID[tid] = c
 				m.mu.Unlock()
 				c.loadSegments(tid)
-				getVCPUCounter.Increment(getVCPUAcquisitionUnused)
+				getVCPUCounter.Increment(&getVCPUAcquisitionUnused)
 				return c
 			}
 		}
@@ -533,7 +533,7 @@ func (m *machine) Get() *vCPU {
 			m.vCPUsByTID[tid] = c
 			m.mu.Unlock()
 			c.loadSegments(tid)
-			getVCPUCounter.Increment(getVCPUAcquisitionStolen)
+			getVCPUCounter.Increment(&getVCPUAcquisitionStolen)
 			return c
 		}

@@ -66,15 +66,24 @@ import (

 const bitsPerUint32 = 32

+// statCounterValue returns a function usable as callback function when defining a gVisor Sentry
+// metric that contains the value counted by the StatCounter.
+// This avoids a dependency loop in the tcpip package.
+func statCounterValue(cm *tcpip.StatCounter) func(...*metric.FieldValue) uint64 {
+	return func(...*metric.FieldValue) uint64 {
+		return cm.Value()
+	}
+}
+
 func mustCreateMetric(name, description string) *tcpip.StatCounter {
 	var cm tcpip.StatCounter
-	metric.MustRegisterCustomUint64Metric(name, true /* cumulative */, false /* sync */, description, cm.Value)
+	metric.MustRegisterCustomUint64Metric(name, true /* cumulative */, false /* sync */, description, statCounterValue(&cm))
 	return &cm
 }

 func mustCreateGauge(name, description string) *tcpip.StatCounter {
 	var cm tcpip.StatCounter
-	metric.MustRegisterCustomUint64Metric(name, false /* cumulative */, false /* sync */, description, cm.Value)
+	metric.MustRegisterCustomUint64Metric(name, false /* cumulative */, false /* sync */, description, statCounterValue(&cm))
 	return &cm
 }

@@ -36,7 +36,7 @@ var (
 // us to pass a function which does not take any arguments, whereas Increment()
 // takes a variadic number of arguments.
 func incrementPartialResultMetric() {
-	metric.WeirdnessMetric.Increment(metric.WeirdnessTypePartialResult)
+	metric.WeirdnessMetric.Increment(&metric.WeirdnessTypePartialResult)
 }

 // HandleIOError handles special error cases for partial results. For some
@@ -97,7 +97,7 @@ func (c *CalibratedClock) resetLocked(str string, v ...any) {
 	c.Warningf(str+" Resetting clock; time may jump.", v...)
 	c.ready = false
 	c.ref.Reset()
-	metric.WeirdnessMetric.Increment(metric.WeirdnessTypeTimeFallback)
+	metric.WeirdnessMetric.Increment(&metric.WeirdnessTypeTimeFallback)
 }

 // updateParams updates the timekeeping parameters based on the passed
@@ -236,7 +236,7 @@ func (w *Watchdog) waitForStart() {
 		return
 	}

-	metric.WeirdnessMetric.Increment(metric.WeirdnessTypeWatchdogStuckStartup)
+	metric.WeirdnessMetric.Increment(&metric.WeirdnessTypeWatchdogStuckStartup)

 	var buf bytes.Buffer
 	buf.WriteString(fmt.Sprintf("Watchdog.Start() not called within %s", w.StartupTimeout))
@@ -309,7 +309,7 @@ func (w *Watchdog) runTurn() {
 					// unless they are surrounded by
 					// Task.UninterruptibleSleepStart/Finish.
 					tc = &offender{lastUpdateTime: lastUpdateTime}
-					metric.WeirdnessMetric.Increment(metric.WeirdnessTypeWatchdogStuckTasks)
+					metric.WeirdnessMetric.Increment(&metric.WeirdnessTypeWatchdogStuckTasks)
 					newTaskFound = true
 				}
 				newOffenders[t] = tc
@@ -1398,7 +1398,7 @@ func (s *StatCounter) Decrement() {
 }

 // Value returns the current value of the counter.
-func (s *StatCounter) Value(...string) uint64 {
+func (s *StatCounter) Value() uint64 {
 	return s.count.Load()
 }