diff --git a/pkg/metric/fake_metric.go b/pkg/metric/fake_metric.go index 58b45ba7d..323f35af2 100644 --- a/pkg/metric/fake_metric.go +++ b/pkg/metric/fake_metric.go @@ -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 diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index 635637fbd..4bea1b487 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -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 { diff --git a/pkg/metric/metric_test.go b/pkg/metric/metric_test.go index dd5fc5c8a..5dc58763b 100644 --- a/pkg/metric/metric_test.go +++ b/pkg/metric/metric_test.go @@ -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() { diff --git a/pkg/metric/metric_unsafe.go b/pkg/metric/metric_unsafe.go index 1a60e5ff0..06c043e2a 100644 --- a/pkg/metric/metric_unsafe.go +++ b/pkg/metric/metric_unsafe.go @@ -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") -} diff --git a/pkg/sentry/fsimpl/gofer/regular_file.go b/pkg/sentry/fsimpl/gofer/regular_file.go index afa4437cf..ef90c5915 100644 --- a/pkg/sentry/fsimpl/gofer/regular_file.go +++ b/pkg/sentry/fsimpl/gofer/regular_file.go @@ -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() diff --git a/pkg/sentry/fsimpl/gofer/special_file.go b/pkg/sentry/fsimpl/gofer/special_file.go index 7871b1104..5292c5447 100644 --- a/pkg/sentry/fsimpl/gofer/special_file.go +++ b/pkg/sentry/fsimpl/gofer/special_file.go @@ -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() diff --git a/pkg/sentry/kernel/syscalls.go b/pkg/sentry/kernel/syscalls.go index fdf8b1c67..4cdc5d385 100644 --- a/pkg/sentry/kernel/syscalls.go +++ b/pkg/sentry/kernel/syscalls.go @@ -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) } diff --git a/pkg/sentry/kernel/task_syscall.go b/pkg/sentry/kernel/task_syscall.go index 246044965..d2f26dd9d 100644 --- a/pkg/sentry/kernel/task_syscall.go +++ b/pkg/sentry/kernel/task_syscall.go @@ -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)) diff --git a/pkg/sentry/platform/kvm/machine.go b/pkg/sentry/platform/kvm/machine.go index 4ca8e32e5..f39bf1f06 100644 --- a/pkg/sentry/platform/kvm/machine.go +++ b/pkg/sentry/platform/kvm/machine.go @@ -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 } diff --git a/pkg/sentry/socket/netstack/netstack.go b/pkg/sentry/socket/netstack/netstack.go index 8a52962a0..1b5a31fe7 100644 --- a/pkg/sentry/socket/netstack/netstack.go +++ b/pkg/sentry/socket/netstack/netstack.go @@ -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 } diff --git a/pkg/sentry/syscalls/linux/error.go b/pkg/sentry/syscalls/linux/error.go index c71de33d7..7da5caafa 100644 --- a/pkg/sentry/syscalls/linux/error.go +++ b/pkg/sentry/syscalls/linux/error.go @@ -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 diff --git a/pkg/sentry/time/calibrated_clock.go b/pkg/sentry/time/calibrated_clock.go index 4ef1b9542..132de5ccf 100644 --- a/pkg/sentry/time/calibrated_clock.go +++ b/pkg/sentry/time/calibrated_clock.go @@ -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 diff --git a/pkg/sentry/watchdog/watchdog.go b/pkg/sentry/watchdog/watchdog.go index bf4325fdb..e3aba6301 100644 --- a/pkg/sentry/watchdog/watchdog.go +++ b/pkg/sentry/watchdog/watchdog.go @@ -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 diff --git a/pkg/tcpip/tcpip.go b/pkg/tcpip/tcpip.go index ede2aa6fa..1fa464c53 100644 --- a/pkg/tcpip/tcpip.go +++ b/pkg/tcpip/tcpip.go @@ -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() }