diff --git a/pkg/metric/BUILD b/pkg/metric/BUILD index 9b7eb0d98..00fd85a6d 100644 --- a/pkg/metric/BUILD +++ b/pkg/metric/BUILD @@ -43,7 +43,9 @@ go_test( deps = [ ":metric_go_proto", "//pkg/eventchannel", + "//pkg/prometheus", "//pkg/sync", + "@com_github_prometheus_common//expfmt", "@org_golang_google_protobuf//proto:go_default_library", ], ) diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index cc8c9689b..0fd68dddc 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -156,7 +156,7 @@ func Disable() error { m := pb.MetricRegistration{} if err := eventchannel.Emit(&m); err != nil { - return fmt.Errorf("unable to emit metric disable event: %w", err) + return fmt.Errorf("unable to emit empty metric registration event (metrics disabled): %w", err) } initialized = true @@ -942,9 +942,12 @@ type metricValues struct { // The first key level is the metric name. // The second key level is an index ID corresponding to the combination of // field values. The index is decoded to field strings using keyToMultiField. - // The value is the number of samples in each bucket of the distribution, - // with the first (0-th) element being the underflow bucket and the last - // element being the "infinite" (overflow) bucket. + // The slice value is the number of samples in each bucket of the + // distribution, with the first (0-th) element being the underflow bucket + // and the last element being the "infinite" (overflow) bucket. + // The slice value may also be nil for field combinations with no samples. + // This saves memory by avoiding storing anything for unused field + // combinations. distributionMetrics map[string][][]uint64 // distributionTotalSamples is the total number of samples for each @@ -1136,10 +1139,14 @@ func GetSnapshot() *prometheus.Snapshot { if b == numFiniteBuckets+1 { upperBound = prometheus.Number{Float: math.Inf(1)} // Overflow bucket. } else { - upperBound = prometheus.Number{Int: m.exponentialBucketer.LowerBound(b + 1)} + upperBound = prometheus.Number{Int: m.exponentialBucketer.LowerBound(b)} + } + samples := uint64(0) + if currentSamples != nil { + samples = currentSamples[b] } buckets[b] = prometheus.Bucket{ - Samples: currentSamples[b], + Samples: samples, UpperBound: upperBound, } } diff --git a/pkg/metric/metric_test.go b/pkg/metric/metric_test.go index 109d6c9ce..bccac60b7 100644 --- a/pkg/metric/metric_test.go +++ b/pkg/metric/metric_test.go @@ -15,14 +15,17 @@ package metric import ( + "bytes" "fmt" "math" "reflect" "testing" "time" + "github.com/prometheus/common/expfmt" "google.golang.org/protobuf/proto" pb "gvisor.dev/gvisor/pkg/metric/metric_go_proto" + "gvisor.dev/gvisor/pkg/prometheus" "gvisor.dev/gvisor/pkg/sync" ) @@ -33,6 +36,20 @@ const ( distribDescription = "A distribution metric for testing" ) +// 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. +// However, it does not verify that the data that was parsed actually matches the metric data. +func verifyPrometheusParsing(t *testing.T) { + t.Helper() + var buf bytes.Buffer + if _, err := GetSnapshot().WriteTo(&buf, prometheus.ExportOptions{}); err != nil { + t.Errorf("failed to get Prometheus snapshot: %v", err) + } else if _, err := (&expfmt.TextParser{}).TextToMetricFamilies(&buf); err != nil { + t.Errorf("failed to parse Prometheus output: %v", err) + } +} + func TestInitialize(t *testing.T) { defer resetTest() @@ -57,6 +74,7 @@ func TestInitialize(t *testing.T) { if err := Initialize(); err != nil { t.Fatalf("Initialize(): %s", err) } + verifyPrometheusParsing(t) if len(emitter) != 1 { t.Fatalf("Initialize emitted %d events want 1", len(emitter)) @@ -140,6 +158,7 @@ func TestInitialize(t *testing.T) { if !foundDistrib { t.Errorf("/distrib not found: %+v", emitter) } + verifyPrometheusParsing(t) } func TestDisable(t *testing.T) { @@ -258,6 +277,8 @@ func TestEmitMetricUpdate(t *testing.T) { t.Fatal("Aborting test so far due to earlier errors.") } + verifyPrometheusParsing(t) + // Increment foo. Only it is included in the next update. foo.Increment() foo.Increment() @@ -285,6 +306,7 @@ func TestEmitMetricUpdate(t *testing.T) { if uv.Uint64Value != 3 { t.Errorf("%v: Value got %v want %d", m, uv.Uint64Value, 3) } + verifyPrometheusParsing(t) // Add a few samples to the distribution metric. distrib.AddSample(1, "foo", "baz") @@ -338,6 +360,7 @@ 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") @@ -363,6 +386,7 @@ func TestEmitMetricUpdate(t *testing.T) { t.Errorf("%+v: sample %d: got %d want %d", dv.DistributionValue, i, s, wantSamples[i]) } } + verifyPrometheusParsing(t) // Change nothing but still call EmitMetricUpdate. Verify that nothing gets sent. emitter.Reset() @@ -370,6 +394,7 @@ func TestEmitMetricUpdate(t *testing.T) { if len(emitter) != 0 { t.Fatalf("EmitMetricUpdate emitted %d events want %d", len(emitter), 0) } + verifyPrometheusParsing(t) } func TestEmitMetricUpdateWithFields(t *testing.T) { @@ -387,6 +412,7 @@ func TestEmitMetricUpdateWithFields(t *testing.T) { if err := Initialize(); err != nil { t.Fatalf("Initialize(): %s", err) } + verifyPrometheusParsing(t) // Don't care about the registration metrics. emitter.Reset() @@ -397,6 +423,7 @@ func TestEmitMetricUpdateWithFields(t *testing.T) { if len(emitter) != 0 { t.Fatalf("EmitMetricUpdate emitted %d events want 0", len(emitter)) } + verifyPrometheusParsing(t) counter.IncrementBy(4, "weird1") counter.Increment("weird2") @@ -457,6 +484,8 @@ func TestEmitMetricUpdateWithFields(t *testing.T) { if !foundWeird2 { t.Errorf("Field value weird2 not found: %+v", emitter) } + + verifyPrometheusParsing(t) } func TestMetricUpdateStageTiming(t *testing.T) { @@ -536,6 +565,7 @@ func TestMetricUpdateStageTiming(t *testing.T) { checkStage(firstUpdate.StageTiming[0], "before_first_update_1") checkStage(firstUpdate.StageTiming[1], "before_first_update_2") } + verifyPrometheusParsing(t) // Ensure re-emitting doesn't cause another event to be sent. emitter.Reset() @@ -543,6 +573,7 @@ func TestMetricUpdateStageTiming(t *testing.T) { if len(emitter) != 0 { t.Fatalf("EmitMetricUpdate emitted %d events want %d", len(emitter), 0) } + verifyPrometheusParsing(t) // Generate monitoring data, we should get an event with no stages. fooMetric.Increment() @@ -555,6 +586,7 @@ func TestMetricUpdateStageTiming(t *testing.T) { } else if len(update.StageTiming) != 0 { t.Errorf("unexpected stage timing information: %v", update.StageTiming) } + verifyPrometheusParsing(t) // Now generate new stages. measureStage("foo_stage_1", func() { @@ -577,6 +609,7 @@ func TestMetricUpdateStageTiming(t *testing.T) { checkStage(update.StageTiming[0], "foo_stage_1") checkStage(update.StageTiming[1], "foo_stage_2") } + verifyPrometheusParsing(t) // Now try generating data for both metrics and stages. fooMetric.Increment() @@ -601,6 +634,7 @@ func TestMetricUpdateStageTiming(t *testing.T) { checkStage(update.StageTiming[0], "last_stage_1") checkStage(update.StageTiming[1], "last_stage_2") } + verifyPrometheusParsing(t) } func TestTimerMetric(t *testing.T) { @@ -663,6 +697,7 @@ func TestTimerMetric(t *testing.T) { t.Errorf("%+v: sample %d: got %d want %d", dv.DistributionValue, i, s, wantSamples[i]) } } + verifyPrometheusParsing(t) } func TestBucketer(t *testing.T) {