From 2511e2e937ec956f836909c33e1ff2ba74fd3fdc Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Wed, 28 Aug 2024 16:31:32 -0700 Subject: [PATCH] Actually support arbitrary number of fields in uint64 metrics. PiperOrigin-RevId: 668653086 --- pkg/metric/BUILD | 2 + pkg/metric/metric.go | 82 ++++++------ pkg/metric/metric_test.go | 264 ++++++++++++++++++-------------------- 3 files changed, 170 insertions(+), 178 deletions(-) diff --git a/pkg/metric/BUILD b/pkg/metric/BUILD index 04235af55..4bd2fa2b0 100644 --- a/pkg/metric/BUILD +++ b/pkg/metric/BUILD @@ -52,8 +52,10 @@ go_test( "//pkg/eventchannel", "//pkg/prometheus", "//pkg/sync", + "@com_github_google_go_cmp//cmp:go_default_library", "@com_github_prometheus_common//expfmt", "@org_golang_google_protobuf//proto:go_default_library", + "@org_golang_google_protobuf//testing/protocmp:go_default_library", ], ) diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index f3c0d1c77..002ad7fd0 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -241,6 +241,10 @@ type customUint64Metric struct { // fields is the set of fields of the metric. fields []Field + // fieldMapper is used to generate index keys for the fields array (above) + // based on field value combinations, and vice-versa. + fieldMapper fieldMapper + // 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 ...*FieldValue) uint64 @@ -398,8 +402,6 @@ func (m fieldMapper) lookupSingle(fieldIndex int, fieldValue *FieldValue, idx, r 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] @@ -552,6 +554,11 @@ func RegisterCustomUint64Metric(name string, metadata Uint64Metadata, value func promType = prometheus.TypeCounter } + fm, err := newFieldMapper(metadata.Fields...) + if err != nil { + return fmt.Errorf("invalid fields: %w", err) + } + allMetrics.uint64Metrics[name] = customUint64Metric{ metadata: &pb.MetricMetadata{ Name: name, @@ -567,15 +574,10 @@ func RegisterCustomUint64Metric(name string, metadata Uint64Metadata, value func Help: metadata.Description, Type: promType, }, - fields: metadata.Fields, - value: value, + fields: metadata.Fields, + fieldMapper: fm, + value: value, } - - // Metrics can exist without fields. - if l := len(metadata.Fields); l > 1 { - return fmt.Errorf("%d fields provided, must be <= 1", l) - } - for _, field := range metadata.Fields { allMetrics.uint64Metrics[name].metadata.Fields = append(allMetrics.uint64Metrics[name].metadata.Fields, field.toProto()) } @@ -598,20 +600,14 @@ func NewUint64Metric(name string, metadata Uint64Metadata) (*Uint64Metric, error if err := verifyName(name); err != nil { return nil, err } - f, err := newFieldMapper(metadata.Fields...) - if err != nil { - return nil, err - } - m := Uint64Metric{ - name: name, - fieldMapper: f, - fields: make([]atomicbitops.Uint64, f.numKeys()), - } + m := Uint64Metric{name: name} if err := RegisterCustomUint64Metric(name, metadata, m.Value); err != nil { return nil, err } cm := allMetrics.uint64Metrics[name] cm.forEachNonZero = m.forEachNonZero + m.fieldMapper = cm.fieldMapper + m.fields = make([]atomicbitops.Uint64, cm.fieldMapper.numKeys()) allMetrics.uint64Metrics[name] = cm return &m, nil } @@ -1250,25 +1246,30 @@ func (m *metricSet) Values() metricValues { distributionStatistics: make(map[string][]distributionStatisticsSnapshot, len(m.distributionMetrics)), stages: stages, } + var tmpFieldValues []*FieldValue for k, v := range m.uint64Metrics { fields := v.fields switch len(fields) { case 0: vals.uint64Metrics[k] = v.value() - case 1: - fieldsMap := make(map[*FieldValue]uint64) + default: + numFieldCombinations := v.fieldMapper.numKeys() + perFieldKeyVals := make([]uint64, numFieldCombinations) if v.forEachNonZero != nil { v.forEachNonZero(func(fieldValues []*FieldValue, val uint64) { - fieldsMap[fieldValues[0]] = val + perFieldKeyVals[v.fieldMapper.lookup(fieldValues...)] = val }) } else { - for _, fieldValue := range fields[0].values { - fieldsMap[fieldValue] = v.value(fieldValue) + if len(tmpFieldValues) < len(v.fields) { + tmpFieldValues = make([]*FieldValue, len(v.fields)) + } + fieldValues := tmpFieldValues[:len(v.fields)] + for fieldKey := 0; fieldKey < numFieldCombinations; fieldKey++ { + v.fieldMapper.keyToMultiFieldInPlace(fieldKey, fieldValues) + perFieldKeyVals[fieldKey] = v.value(fieldValues...) } } - vals.uint64Metrics[k] = fieldsMap - default: - panic(fmt.Sprintf("Unsupported number of metric fields: %d", len(fields))) + vals.uint64Metrics[k] = perFieldKeyVals } } for name, metric := range m.distributionMetrics { @@ -1304,8 +1305,9 @@ 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[*FieldValue]uint64 - // to support metrics with one field. + // with key as metric name. Value can be either uint64, or []uint64 + // to support metrics with fields. The index corresponds to the field + // value combination key as managed by `fieldMapper`. uint64Metrics map[string]any // distributionMetrics is a map of distribution metrics. @@ -1375,19 +1377,19 @@ func EmitMetricUpdate() { Name: k, Value: &pb.MetricValue_Uint64Value{Uint64Value: t}, }) - case map[*FieldValue]uint64: - for fieldValue, metricValue := range t { + case []uint64: + for fieldKey, 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[*FieldValue]uint64)[fieldValue] == metricValue) { + if (!ok && metricValue == 0) || (ok && prev.([]uint64)[fieldKey] == metricValue) { continue } m.Metrics = append(m.Metrics, &pb.MetricValue{ Name: k, - FieldValues: []string{fieldValue.Value}, + FieldValues: allMetrics.uint64Metrics[k].fieldMapper.keyToMultiField(fieldKey), Value: &pb.MetricValue_Uint64Value{Uint64Value: metricValue}, }) } @@ -1533,16 +1535,18 @@ func GetSnapshot(options SnapshotOptions) (*prometheus.Snapshot, error) { continue } snapshot.Add(prometheus.NewIntData(m.prometheusMetric, int64(t))) - case map[*FieldValue]uint64: - for fieldValue, metricValue := range t { + case []uint64: + for fieldKey, metricValue := range t { if m.metadata.GetCumulative() && metricValue == 0 { // Zero-valued counter, ignore. continue } - 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.Value, - }, int64(metricValue))) + fieldValues := m.fieldMapper.keyToMultiField(fieldKey) + fieldKeysAndValues := make(map[string]string, len(fieldValues)) + for i, field := range m.fields { + fieldKeysAndValues[field.name] = fieldValues[i] + } + snapshot.Add(prometheus.LabeledIntData(m.prometheusMetric, fieldKeysAndValues, int64(metricValue))) } default: panic(fmt.Sprintf("unsupported type in uint64Metrics: %T (%v)", v, v)) diff --git a/pkg/metric/metric_test.go b/pkg/metric/metric_test.go index 1dd11296c..0d1dd6c71 100644 --- a/pkg/metric/metric_test.go +++ b/pkg/metric/metric_test.go @@ -27,8 +27,10 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/prometheus/common/expfmt" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/testing/protocmp" pb "gvisor.dev/gvisor/pkg/metric/metric_go_proto" "gvisor.dev/gvisor/pkg/prometheus" "gvisor.dev/gvisor/pkg/sync" @@ -99,6 +101,8 @@ func TestVerifyName(t *testing.T) { func TestInitialize(t *testing.T) { defer resetTest() + field1 := NewField("field1", &fieldValFoo, &fieldValBar) + field2 := NewField("field2", &fieldValBaz, &fieldValQuux) _, err := NewUint64Metric("/foo", Uint64Metadata{ Cumulative: true, @@ -112,6 +116,7 @@ func TestInitialize(t *testing.T) { Cumulative: true, Sync: true, Description: barDescription, + Fields: []Field{field1, field2}, Unit: pb.MetricMetadata_UNITS_NANOSECONDS, }) if err != nil { @@ -119,8 +124,6 @@ func TestInitialize(t *testing.T) { } bucketer := NewExponentialBucketer(3, 2, 0, 1) - 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) @@ -180,6 +183,13 @@ func TestInitialize(t *testing.T) { if !m.Sync { t.Errorf("/bar %+v Sync got false want true", m) } + wantFields := []*pb.MetricMetadata_Field{ + {FieldName: "field1", AllowedValues: []string{"foo", "bar"}}, + {FieldName: "field2", AllowedValues: []string{"baz", "quux"}}, + } + if diff := cmp.Diff(m.Fields, wantFields, protocmp.Transform()); diff != "" { + t.Errorf("/bar %+v fields: got %v want %v\ndiff:\n%s", m, m.Fields, wantFields, diff) + } if m.Units != pb.MetricMetadata_UNITS_NANOSECONDS { t.Errorf("/bar %+v Units got %v want %v", m, m.Units, pb.MetricMetadata_UNITS_NANOSECONDS) } @@ -260,6 +270,8 @@ func TestDisable(t *testing.T) { func TestEmitMetricUpdate(t *testing.T) { defer resetTest() + field1 := NewField("field1", &fieldValFoo, &fieldValBar) + field2 := NewField("field2", &fieldValBaz, &fieldValQuux) foo, err := NewUint64Metric("/foo", Uint64Metadata{ Cumulative: true, @@ -269,17 +281,16 @@ func TestEmitMetricUpdate(t *testing.T) { t.Fatalf("NewUint64Metric got err %v want nil", err) } - _, err = NewUint64Metric("/bar", Uint64Metadata{ + bar, err := NewUint64Metric("/bar", Uint64Metadata{ Cumulative: true, Description: barDescription, + Fields: []Field{field1, field2}, }) if err != nil { t.Fatalf("NewUint64Metric got err %v want nil", err) } bucketer := NewExponentialBucketer(2, 2, 0, 1) - 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) @@ -302,11 +313,11 @@ func TestEmitMetricUpdate(t *testing.T) { 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 %d", len(update.Metrics), 2) + // We only expect /foo, as /bar is initially omitted due to being all-zero. + if len(update.Metrics) != 1 { + t.Errorf("MetricUpdate got %d metrics want %d", len(update.Metrics), 1) } - // Both are included for their initial values. foundFoo := false foundBar := false foundDistrib := false @@ -334,8 +345,8 @@ func TestEmitMetricUpdate(t *testing.T) { if !foundFoo { t.Errorf("/foo not found: %+v", emitter) } - if !foundBar { - t.Errorf("/bar not found: %+v", emitter) + if foundBar { + t.Errorf("/bar unexpectedly found: %+v", emitter) } if foundDistrib { t.Errorf("/distrib unexpectedly found: %+v", emitter) @@ -375,13 +386,16 @@ func TestEmitMetricUpdate(t *testing.T) { } verifyPrometheusParsing(t) - // Add a few samples to the distribution metric. + // Add a few samples to the distribution metric and increment two of the + // /bar field combinations. 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) + bar.Increment(&fieldValFoo, &fieldValBaz) + bar.IncrementBy(1337, &fieldValFoo, &fieldValQuux) emitter.Reset() EmitMetricUpdate() if len(emitter) != 1 { @@ -391,40 +405,61 @@ func TestEmitMetricUpdate(t *testing.T) { if !ok { t.Fatalf("emitter %v got %T want pb.MetricUpdate", emitter[0], emitter[0]) } - if len(update.Metrics) != 2 { - t.Fatalf("MetricUpdate got %d metrics want %d", len(update.Metrics), 1) + if len(update.Metrics) != 4 { + t.Fatalf("MetricUpdate got %d metrics want %d", len(update.Metrics), 4) } for _, m := range update.Metrics { - if m.Name != "/distrib" { - t.Fatalf("Metric %+v name got %q want '/distrib'", m, m.Name) - } - if len(m.FieldValues) != 2 { - t.Fatalf("Metric %+v fields: got %v want %d fields", m, m.FieldValues, 2) - } - if m.FieldValues[0] != "foo" { - t.Fatalf("Metric %+v field 0: got %v want %v", m, m.FieldValues[0], "foo") - } - dv, ok := m.Value.(*pb.MetricValue_DistributionValue) - if !ok { - t.Fatalf("%+v: value %v got %T want pb.MetricValue_DistributionValue", m, m.Value, m.Value) - } - samples := dv.DistributionValue.GetNewSamples() - if len(samples) != 4 { - t.Fatalf("%+v: got %d buckets, want %d", dv.DistributionValue, len(samples), 4) - } - var wantSamples []uint64 - switch m.FieldValues[1] { - case "baz": - wantSamples = []uint64{0, 2, 1, 0} - case "quux": - wantSamples = []uint64{1, 1, 0, 1} - default: - t.Fatalf("%+v: got unexpected field[1]: %q", m, m.FieldValues[1]) - } - for i, s := range samples { - if s != wantSamples[i] { - t.Errorf("%+v [fields %v]: sample %d: got %d want %d", dv.DistributionValue, m.FieldValues, i, s, wantSamples[i]) + switch m.Name { + case "/distrib": + if len(m.FieldValues) != 2 { + t.Fatalf("Metric %+v fields: got %v want %d fields", m, m.FieldValues, 2) } + if m.FieldValues[0] != "foo" { + t.Fatalf("Metric %+v field 0: got %v want %v", m, m.FieldValues[0], "foo") + } + dv, ok := m.Value.(*pb.MetricValue_DistributionValue) + if !ok { + t.Fatalf("%+v: value %v got %T want pb.MetricValue_DistributionValue", m, m.Value, m.Value) + } + samples := dv.DistributionValue.GetNewSamples() + if len(samples) != 4 { + t.Fatalf("%+v: got %d buckets, want %d", dv.DistributionValue, len(samples), 4) + } + var wantSamples []uint64 + switch m.FieldValues[1] { + case "baz": + wantSamples = []uint64{0, 2, 1, 0} + case "quux": + wantSamples = []uint64{1, 1, 0, 1} + default: + t.Fatalf("%+v: got unexpected field[1]: %q", m, m.FieldValues[1]) + } + for i, s := range samples { + if s != wantSamples[i] { + t.Errorf("%+v [fields %v]: sample %d: got %d want %d", dv.DistributionValue, m.FieldValues, i, s, wantSamples[i]) + } + } + case "/bar": + if len(m.FieldValues) != 2 { + t.Fatalf("Metric %+v fields: got %v want %d fields", m, m.FieldValues, 2) + } + if m.FieldValues[0] != "foo" { + t.Fatalf("Metric %+v field 0: got %v want %v", m, m.FieldValues[0], "foo") + } + var wantValue uint64 + switch m.FieldValues[1] { + case "baz": + wantValue = 1 + case "quux": + wantValue = 1337 + default: + t.Fatalf("%+v: got unexpected field[1]: %q", m, m.FieldValues[1]) + } + if uv, ok := m.Value.(*pb.MetricValue_Uint64Value); !ok || uv.Uint64Value != wantValue { + t.Errorf("%+v: value for fields %v: got %T value %v want uint64 value %d", m, m.FieldValues, m.Value, m.Value, wantValue) + } + default: + t.Fatalf("Metric update %+v: Unexpected metric name got %q", m, m.Name) } } verifyPrometheusParsing(t) @@ -464,103 +499,6 @@ func TestEmitMetricUpdate(t *testing.T) { verifyPrometheusParsing(t) } -func TestEmitMetricUpdateWithFields(t *testing.T) { - defer resetTest() - - var ( - weird1 = FieldValue{"weird1"} - weird2 = FieldValue{"weird2"} - ) - field := NewField("weirdness_type", &weird1, &weird2) - - counter, err := NewUint64Metric("/weirdness", Uint64Metadata{ - Cumulative: true, - Description: counterDescription, - Fields: []Field{field}, - }) - if err != nil { - t.Fatalf("NewUint64Metric got err %v want nil", err) - } - - if err := Initialize(); err != nil { - t.Fatalf("Initialize(): %s", err) - } - verifyPrometheusParsing(t) - - // 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)) - } - verifyPrometheusParsing(t) - - 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.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) - } - if uv.Uint64Value != 4 { - t.Errorf("%v: Value got %v want 4", m, uv.Uint64Value) - } - foundWeird1 = true - 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) - } - 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) - } - - verifyPrometheusParsing(t) -} - func TestMetricUpdateStageTiming(t *testing.T) { defer resetTest() @@ -1065,6 +1003,12 @@ func TestMetricProfiling(t *testing.T) { fieldVal1 := &FieldValue{"val1"} fieldVal2 := &FieldValue{"val2"} fieldVal3 := &FieldValue{"val3"} + fieldVal4 := &FieldValue{"val4"} + fieldVal5 := &FieldValue{"val5"} + fieldVal6 := &FieldValue{"val6"} + fieldVal7 := &FieldValue{"val7"} + fieldVal8 := &FieldValue{"val8"} + fieldVal9 := &FieldValue{"val9"} for _, test := range []struct { name string @@ -1122,6 +1066,48 @@ func TestMetricProfiling(t *testing.T) { numIterations: 100, errOnStartProfiling: false, }, + { + name: "single metric with multiple fields", + profilingMetricsFlag: "/foo", + metricNames: []string{"/foo"}, + firstMetricFields: []Field{ + NewField("field1", fieldVal1, fieldVal2, fieldVal3), + NewField("field2", fieldVal4, fieldVal5, fieldVal6), + NewField("field3", fieldVal7, fieldVal8, fieldVal9), + }, + expectedHeader: TimeColumn + "\t" + strings.Join([]string{ + "/foo[val1,val4,val7]", + "/foo[val1,val4,val8]", + "/foo[val1,val4,val9]", + "/foo[val1,val5,val7]", + "/foo[val1,val5,val8]", + "/foo[val1,val5,val9]", + "/foo[val1,val6,val7]", + "/foo[val1,val6,val8]", + "/foo[val1,val6,val9]", + "/foo[val2,val4,val7]", + "/foo[val2,val4,val8]", + "/foo[val2,val4,val9]", + "/foo[val2,val5,val7]", + "/foo[val2,val5,val8]", + "/foo[val2,val5,val9]", + "/foo[val2,val6,val7]", + "/foo[val2,val6,val8]", + "/foo[val2,val6,val9]", + "/foo[val3,val4,val7]", + "/foo[val3,val4,val8]", + "/foo[val3,val4,val9]", + "/foo[val3,val5,val7]", + "/foo[val3,val5,val8]", + "/foo[val3,val5,val9]", + "/foo[val3,val6,val7]", + "/foo[val3,val6,val8]", + "/foo[val3,val6,val9]", + }, "\t"), + incrementMetricBy: []uint64{1337}, + numIterations: 100, + errOnStartProfiling: false, + }, { name: "multiple metrics and one has fields", profilingMetricsFlag: "/foo,/bar",