diff options
author | Nayana Bidari <nybidari@google.com> | 2021-04-15 20:01:04 -0700 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-04-15 20:02:50 -0700 |
commit | 14b7d775c950070378ea799a0b6b7907f67a1f1e (patch) | |
tree | 7cfae25f149909f460fb6ea07bdf28b6fcaf4cd9 /pkg | |
parent | 82dc881dba7399afa4268c5e3a70624db0b1e7ee (diff) |
Add field support to the sentry metrics.
Fields allow counter metrics to have multiple tabular values.
At most one field is supported at the moment.
PiperOrigin-RevId: 368767040
Diffstat (limited to 'pkg')
-rw-r--r-- | pkg/metric/metric.go | 175 | ||||
-rw-r--r-- | pkg/metric/metric.proto | 11 | ||||
-rw-r--r-- | pkg/metric/metric_test.go | 92 | ||||
-rw-r--r-- | pkg/sentry/syscalls/linux/error.go | 12 | ||||
-rw-r--r-- | pkg/tcpip/tcpip.go | 2 |
5 files changed, 258 insertions, 34 deletions
diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index c9f9357de..2a2f0d611 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -38,7 +38,7 @@ var ( ) // Uint64Metric encapsulates a uint64 that represents some kind of metric to be -// monitored. +// monitored. We currently support metrics with at most one field. // // Metrics are not saved across save/restore and thus reset to zero on restore. // @@ -46,6 +46,16 @@ var ( type Uint64Metric struct { // value is the actual value of the metric. It must be accessed atomically. value uint64 + + // numFields is the number of metric fields. It is immutable once + // initialized. + numFields int + + // mu protects the below fields. + mu sync.RWMutex `state:"nosave"` + + // fields is the map of fields in the metric. + fields map[string]uint64 } var ( @@ -97,8 +107,19 @@ type customUint64Metric struct { // metadata describes the metric. It is immutable. metadata *pb.MetricMetadata - // value returns the current value of the metric. - value func() uint64 + // 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 +} + +// Field contains the field name and allowed values for the metric which is +// used in registration of the metric. +type Field struct { + // name is the metric field name. + name string + + // allowedValues is the list of allowed values for the field. + allowedValues []string } // RegisterCustomUint64Metric registers a metric with the given name. @@ -109,7 +130,8 @@ type customUint64Metric struct { // Preconditions: // * name must be globally unique. // * Initialize/Disable have not been called. -func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.MetricMetadata_Units, description string, value func() uint64) error { +// * 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 { if initialized { return ErrInitializationDone } @@ -129,13 +151,25 @@ func RegisterCustomUint64Metric(name string, cumulative, sync bool, units pb.Met }, value: value, } + + // Metrics can exist without fields. + if len(fields) > 1 { + panic("Sentry metrics support at most one field") + } + + for _, field := range fields { + allMetrics.m[name].metadata.Fields = append(allMetrics.m[name].metadata.Fields, &pb.MetricMetadata_Field{ + FieldName: field.name, + AllowedValues: field.allowedValues, + }) + } return nil } -// MustRegisterCustomUint64Metric calls RegisterCustomUint64Metric and panics -// if it returns an error. -func MustRegisterCustomUint64Metric(name string, cumulative, sync bool, description string, value func() uint64) { - if err := RegisterCustomUint64Metric(name, cumulative, sync, pb.MetricMetadata_UNITS_NONE, description, value); err != nil { +// 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) { + if err := RegisterCustomUint64Metric(name, cumulative, sync, pb.MetricMetadata_UNITS_NONE, description, value, fields...); err != nil { panic(fmt.Sprintf("Unable to register metric %q: %v", name, err)) } } @@ -144,15 +178,24 @@ func MustRegisterCustomUint64Metric(name string, cumulative, sync bool, descript // name. // // Metrics must be statically defined (i.e., at init). -func NewUint64Metric(name string, sync bool, units pb.MetricMetadata_Units, description string) (*Uint64Metric, error) { - var m Uint64Metric - return &m, RegisterCustomUint64Metric(name, true /* cumulative */, sync, units, description, m.Value) +func NewUint64Metric(name string, sync bool, units pb.MetricMetadata_Units, description string, fields ...Field) (*Uint64Metric, error) { + m := Uint64Metric{ + numFields: len(fields), + } + + if m.numFields == 1 { + m.fields = make(map[string]uint64) + for _, fieldValue := range fields[0].allowedValues { + m.fields[fieldValue] = 0 + } + } + return &m, RegisterCustomUint64Metric(name, true /* cumulative */, sync, units, description, m.Value, fields...) } // MustCreateNewUint64Metric calls NewUint64Metric and panics if it returns an // error. -func MustCreateNewUint64Metric(name string, sync bool, description string) *Uint64Metric { - m, err := NewUint64Metric(name, sync, pb.MetricMetadata_UNITS_NONE, description) +func MustCreateNewUint64Metric(name string, sync bool, description string, fields ...Field) *Uint64Metric { + m, err := NewUint64Metric(name, sync, pb.MetricMetadata_UNITS_NONE, description, fields...) if err != nil { panic(fmt.Sprintf("Unable to create metric %q: %v", name, err)) } @@ -169,19 +212,56 @@ func MustCreateNewUint64NanosecondsMetric(name string, sync bool, description st return m } -// Value returns the current value of the metric. -func (m *Uint64Metric) Value() uint64 { - return atomic.LoadUint64(&m.value) +// Value returns the current value of the metric for the given set of fields. +func (m *Uint64Metric) Value(fieldValues ...string) uint64 { + if m.numFields != len(fieldValues) { + panic(fmt.Sprintf("Number of fieldValues %d is not equal to the number of metric fields %d", len(fieldValues), m.numFields)) + } + + switch m.numFields { + case 0: + return atomic.LoadUint64(&m.value) + case 1: + m.mu.RLock() + defer m.mu.RUnlock() + + fieldValue := fieldValues[0] + if _, ok := m.fields[fieldValue]; !ok { + panic(fmt.Sprintf("Metric does not allow to have field value %s", fieldValue)) + } + return m.fields[fieldValue] + default: + panic("Sentry metrics do not support more than one field") + } } -// Increment increments the metric by 1. -func (m *Uint64Metric) Increment() { - atomic.AddUint64(&m.value, 1) +// Increment increments the metric field by 1. +func (m *Uint64Metric) Increment(fieldValues ...string) { + m.IncrementBy(1, fieldValues...) } // IncrementBy increments the metric by v. -func (m *Uint64Metric) IncrementBy(v uint64) { - atomic.AddUint64(&m.value, v) +func (m *Uint64Metric) IncrementBy(v uint64, fieldValues ...string) { + if m.numFields != len(fieldValues) { + panic(fmt.Sprintf("Number of fieldValues %d is not equal to the number of metric fields %d", len(fieldValues), m.numFields)) + } + + switch m.numFields { + case 0: + atomic.AddUint64(&m.value, v) + return + case 1: + fieldValue := fieldValues[0] + m.mu.Lock() + defer m.mu.Unlock() + + if _, ok := m.fields[fieldValue]; !ok { + panic(fmt.Sprintf("Metric does not allow to have field value %s", fieldValue)) + } + m.fields[fieldValue] += v + default: + panic("Sentry metrics do not support more than one field") + } } // metricSet holds named metrics. @@ -199,14 +279,30 @@ func makeMetricSet() metricSet { // Values returns a snapshot of all values in m. func (m *metricSet) Values() metricValues { vals := make(metricValues) + for k, v := range m.m { - vals[k] = v.value() + fields := v.metadata.GetFields() + switch len(fields) { + case 0: + vals[k] = v.value() + case 1: + values := fields[0].GetAllowedValues() + fieldsMap := make(map[string]uint64) + for _, fieldValue := range values { + fieldsMap[fieldValue] = v.value(fieldValue) + } + vals[k] = fieldsMap + default: + panic(fmt.Sprintf("Unsupported number of metric fields: %d", len(fields))) + } } return vals } -// metricValues contains a copy of the values of all metrics. -type metricValues map[string]uint64 +// metricValues contains a copy of the values of all metrics. It is a map +// with key as metric name and value can be either uint64 or map[string]uint64 +// to support metrics with one field. +type metricValues map[string]interface{} var ( // emitMu protects metricsAtLastEmit and ensures that all emitted @@ -233,14 +329,37 @@ func EmitMetricUpdate() { snapshot := allMetrics.Values() m := pb.MetricUpdate{} + // On the first call metricsAtLastEmit will be empty. Include all + // metrics then. for k, v := range snapshot { - // On the first call metricsAtLastEmit will be empty. Include - // all metrics then. - if prev, ok := metricsAtLastEmit[k]; !ok || prev != v { + prev, ok := metricsAtLastEmit[k] + switch t := v.(type) { + case uint64: + // Metric exists and value did not change. + if ok && prev.(uint64) == t { + continue + } + m.Metrics = append(m.Metrics, &pb.MetricValue{ Name: k, - Value: &pb.MetricValue_Uint64Value{v}, + Value: &pb.MetricValue_Uint64Value{t}, }) + case map[string]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) { + continue + } + + m.Metrics = append(m.Metrics, &pb.MetricValue{ + Name: k, + FieldValues: []string{fieldValue}, + Value: &pb.MetricValue_Uint64Value{metricValue}, + }) + } } } diff --git a/pkg/metric/metric.proto b/pkg/metric/metric.proto index 3cc89047d..53c8b4b50 100644 --- a/pkg/metric/metric.proto +++ b/pkg/metric/metric.proto @@ -48,6 +48,15 @@ message MetricMetadata { // units is the units of the metric value. Units units = 6; + + message Field { + string field_name = 1; + repeated string allowed_values = 2; + } + + // fields contains the metric fields. Currently a metric can have at most + // one field. + repeated Field fields = 7; } // MetricRegistration contains the metadata for all metrics that will be in @@ -66,6 +75,8 @@ message MetricValue { oneof value { uint64 uint64_value = 2; } + + repeated string field_values = 4; } // MetricUpdate contains new values for multiple distinct metrics. diff --git a/pkg/metric/metric_test.go b/pkg/metric/metric_test.go index aefd0ea5c..c71dfd460 100644 --- a/pkg/metric/metric_test.go +++ b/pkg/metric/metric_test.go @@ -59,8 +59,9 @@ func reset() { } const ( - fooDescription = "Foo!" - barDescription = "Bar Baz" + fooDescription = "Foo!" + barDescription = "Bar Baz" + counterDescription = "Counter" ) func TestInitialize(t *testing.T) { @@ -95,7 +96,7 @@ func TestInitialize(t *testing.T) { foundBar := false for _, m := range mr.Metrics { if m.Type != pb.MetricMetadata_TYPE_UINT64 { - t.Errorf("Metadata %+v Type got %v want %v", m, m.Type, pb.MetricMetadata_TYPE_UINT64) + t.Errorf("Metadata %+v Type got %v want pb.MetricMetadata_TYPE_UINT64", m, m.Type) } if !m.Cumulative { t.Errorf("Metadata %+v Cumulative got false want true", m) @@ -256,3 +257,88 @@ func TestEmitMetricUpdate(t *testing.T) { t.Errorf("%v: Value got %v want 1", m, uv.Uint64Value) } } + +func TestEmitMetricUpdateWithFields(t *testing.T) { + defer reset() + + field := Field{ + name: "weirdness_type", + allowedValues: []string{"weird1", "weird2"}} + + counter, err := NewUint64Metric("/weirdness", false, pb.MetricMetadata_UNITS_NONE, counterDescription, field) + if err != nil { + t.Fatalf("NewUint64Metric got err %v want nil", err) + } + + Initialize() + + // Don't care about the registration metrics. + emitter.Reset() + EmitMetricUpdate() + + // For metrics with fields, we do not emit data unless the value is + // incremented. + if len(emitter) != 0 { + t.Fatalf("EmitMetricUpdate emitted %d events want 0", len(emitter)) + } + + counter.IncrementBy(4, "weird1") + counter.Increment("weird2") + + emitter.Reset() + EmitMetricUpdate() + + if len(emitter) != 1 { + t.Fatalf("EmitMetricUpdate emitted %d events want 1", len(emitter)) + } + + update, ok := emitter[0].(*pb.MetricUpdate) + if !ok { + t.Fatalf("emitter %v got %T want pb.MetricUpdate", emitter[0], emitter[0]) + } + + if len(update.Metrics) != 2 { + t.Errorf("MetricUpdate got %d metrics want 2", len(update.Metrics)) + } + + foundWeird1 := false + foundWeird2 := false + for i := 0; i < len(update.Metrics); i++ { + m := update.Metrics[i] + + if m.Name != "/weirdness" { + t.Errorf("Metric %+v name got %q want '/weirdness'", m, m.Name) + } + if len(m.FieldValues) != 1 { + t.Errorf("MetricUpdate got %d fields want 1", len(m.FieldValues)) + } + + switch m.FieldValues[0] { + case "weird1": + 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) + } + if uv.Uint64Value != 4 { + t.Errorf("%v: Value got %v want 4", m, uv.Uint64Value) + } + foundWeird1 = true + case "weird2": + 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) + } + if uv.Uint64Value != 1 { + t.Errorf("%v: Value got %v want 1", m, uv.Uint64Value) + } + foundWeird2 = true + } + } + + if !foundWeird1 { + t.Errorf("Field value weird1 not found: %+v", emitter) + } + if !foundWeird2 { + t.Errorf("Field value weird2 not found: %+v", emitter) + } +} diff --git a/pkg/sentry/syscalls/linux/error.go b/pkg/sentry/syscalls/linux/error.go index efec93f73..37121186a 100644 --- a/pkg/sentry/syscalls/linux/error.go +++ b/pkg/sentry/syscalls/linux/error.go @@ -33,6 +33,14 @@ var ( partialResultOnce sync.Once ) +// incrementPartialResultMetric increments PartialResultMetric by calling +// Increment(). This is added as the func Do() which is called below requires +// us to pass a function which does not take any arguments, whereas Increment() +// takes a variadic number of arguments. +func incrementPartialResultMetric() { + partialResultMetric.Increment() +} + // HandleIOErrorVFS2 handles special error cases for partial results. For some // errors, we may consume the error and return only the partial read/write. // @@ -48,7 +56,7 @@ func HandleIOErrorVFS2(ctx context.Context, partialResult bool, ioerr, intr erro root := vfs.RootFromContext(ctx) name, _ := fs.PathnameWithDeleted(ctx, root, f.VirtualDentry()) log.Traceback("Invalid request partialResult %v and err (type %T) %v for %s operation on %q", partialResult, ioerr, ioerr, op, name) - partialResultOnce.Do(partialResultMetric.Increment) + partialResultOnce.Do(incrementPartialResultMetric) } return nil } @@ -66,7 +74,7 @@ func handleIOError(ctx context.Context, partialResult bool, ioerr, intr error, o // An unknown error is encountered with a partial read/write. name, _ := f.Dirent.FullName(nil /* ignore chroot */) log.Traceback("Invalid request partialResult %v and err (type %T) %v for %s operation on %q, %T", partialResult, ioerr, ioerr, op, name, f.FileOperations) - partialResultOnce.Do(partialResultMetric.Increment) + partialResultOnce.Do(incrementPartialResultMetric) } return nil } diff --git a/pkg/tcpip/tcpip.go b/pkg/tcpip/tcpip.go index 2b6e6a89f..d1b5982f5 100644 --- a/pkg/tcpip/tcpip.go +++ b/pkg/tcpip/tcpip.go @@ -1212,7 +1212,7 @@ func (s *StatCounter) Decrement() { } // Value returns the current value of the counter. -func (s *StatCounter) Value() uint64 { +func (s *StatCounter) Value(name ...string) uint64 { return atomic.LoadUint64(&s.count) } |