runsc metric-server: Do not include zero-valued counters by default.

This cuts down on a lot of needless metric data in the output.

PiperOrigin-RevId: 520711059

pkg/metric/metric.go

@@ -1137,9 +1137,17 @@ func GetSnapshot() (*prometheus.Snapshot, error) {
 		m := allMetrics.uint64Metrics[k]
 		switch t := v.(type) {
 		case uint64:
+			if m.metadata.GetCumulative() && t == 0 {
+				// Zero-valued counter, ignore.
+				continue
+			}
 			snapshot.Add(prometheus.NewIntData(m.prometheusMetric, int64(t)))
 		case map[string]uint64:
 			for fieldValue, 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,
@@ -1162,6 +1170,7 @@ func GetSnapshot() (*prometheus.Snapshot, error) {
 			}
 			currentSamples := distributionSamples[fieldKey]
 			buckets := make([]prometheus.Bucket, numFiniteBuckets+2)
+			samplesForFieldKey := uint64(0)
 			for b := 0; b < numFiniteBuckets+2; b++ {
 				var upperBound prometheus.Number
 				if b == numFiniteBuckets+1 {
@@ -1172,12 +1181,18 @@ func GetSnapshot() (*prometheus.Snapshot, error) {
 				samples := uint64(0)
 				if currentSamples != nil {
 					samples = currentSamples[b]
+					samplesForFieldKey += samples
 				}
 				buckets[b] = prometheus.Bucket{
 					Samples:    samples,
 					UpperBound: upperBound,
 				}
 			}
+			if samplesForFieldKey == 0 {
+				// Zero-valued distribution (no samples in any bucket for this field
+				// combination). Ignore.
+				continue
+			}
 			snapshot.Add(&prometheus.Data{
 				Metric: m.prometheusMetric,
 				Labels: labels,

runsc/container/metric_server_test.go

@@ -20,6 +20,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -53,6 +54,14 @@ type metricsTest struct {
 	serverExtraArgs []string
 }
 
+// applyConf applies metric-server-related configuration options to the given config.
+// Returns the passed-in config itself.
+func (mt *metricsTest) applyConf(conf *config.Config) *config.Config {
+	conf.MetricServer = mt.sleepConf.MetricServer
+	conf.RootDir = mt.rootDir
+	return conf
+}
+
 // setupMetrics sets up a container configuration with metrics enabled, and returns it all.
 // Also returns a cleanup function.
 func setupMetrics(t *testing.T, forceTempUDS bool) (*metricsTest, func()) {
@@ -559,3 +568,114 @@ func TestMetricServerToleratesNoRootDirectory(t *testing.T) {
 		t.Errorf("Metric server was not able to be spawned despite being configured to tolerate a non-existent root directory: %v", err)
 	}
 }
+
+func TestMetricServerDoesNotExportZeroValueCounters(t *testing.T) {
+	te, cleanup := setupMetrics(t, false /* forceTempUDS */)
+	defer cleanup()
+	app, err := testutil.FindFile("test/cmd/test_app/test_app")
+	if err != nil {
+		t.Fatalf("error finding test_app: %v", err)
+	}
+	unimpl1Spec := testutil.NewSpecWithArgs("sh", "-c", fmt.Sprintf("%s syscall --syscall=1337; sleep 1h", app))
+	unimpl1Conf := te.applyConf(testutil.TestConfig(t))
+	unimpl1Bundle, cleanup, err := testutil.SetupBundleDir(unimpl1Spec)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer cleanup()
+	unimpl2Spec := testutil.NewSpecWithArgs("sh", "-c", fmt.Sprintf("%s syscall --syscall=1338; sleep 1h", app))
+	unimpl2Conf := te.applyConf(testutil.TestConfig(t))
+	unimpl2Bundle, cleanup, err := testutil.SetupBundleDir(unimpl2Spec)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer cleanup()
+	unimpl1, err := New(unimpl1Conf, Args{
+		ID:        testutil.RandomContainerID(),
+		Spec:      unimpl1Spec,
+		BundleDir: unimpl1Bundle,
+	})
+	if err != nil {
+		t.Fatalf("error creating first container: %v", err)
+	}
+	defer unimpl1.Destroy()
+	if err := unimpl1.Start(unimpl1Conf); err != nil {
+		t.Fatalf("Cannot start first container: %v", err)
+	}
+	unimpl2, err := New(unimpl2Conf, Args{
+		ID:        testutil.RandomContainerID(),
+		Spec:      unimpl2Spec,
+		BundleDir: unimpl2Bundle,
+	})
+	if err != nil {
+		t.Fatalf("error creating second container: %v", err)
+	}
+	defer unimpl2.Destroy()
+	if err := unimpl2.Start(unimpl2Conf); err != nil {
+		t.Fatalf("Cannot start second container: %v", err)
+	}
+	metricData, err := te.client.GetMetrics(te.testCtx)
+	if err != nil {
+		t.Fatalf("Cannot get metrics: %v", err)
+	}
+	metricDataPtr := &metricData
+
+	// For this test to work, it must wait for long enough such that the containers have
+	// actually tried to call the unimplemented syscall so that it shows up in metrics.
+	waitCtx, waitCtxCancel := context.WithTimeout(te.testCtx, 50*time.Second)
+	defer waitCtxCancel()
+
+	for _, test := range []struct {
+		cont          *Container
+		sysno         uintptr
+		wantExistence bool
+	}{
+		{unimpl1, 1337, true},
+		{unimpl1, 1338, false},
+		{unimpl2, 1337, false},
+		{unimpl2, 1338, true},
+	} {
+		t.Run(fmt.Sprintf("container %s syscall %d", test.cont.ID, test.sysno), func(t *testing.T) {
+			check := func() error {
+				got, _, err := metricDataPtr.GetPrometheusContainerInteger(metricclient.WantMetric{
+					Metric:      "testmetric_unimplemented_syscalls",
+					Sandbox:     test.cont.sandboxID(),
+					ExtraLabels: map[string]string{"sysno": strconv.Itoa(int(test.sysno))},
+				})
+				if test.wantExistence {
+					if err != nil {
+						return fmt.Errorf("cannot get unimplemented syscall metric for sysno=%d even though we expected its presence: %v", test.sysno, err)
+					}
+					if got != 1 {
+						return fmt.Errorf("expected counter value for unimplemented syscall %d be exactly 1, got %d", test.sysno, got)
+					}
+				} else /* !test.wantExistence */ {
+					if err == nil {
+						return fmt.Errorf("unimplemented syscall metric for sysno=%d was unexpectedly present (value: %d)", test.sysno, got)
+					}
+				}
+				return nil
+			}
+			for waitCtx.Err() == nil {
+				if check() == nil {
+					break
+				}
+				select {
+				case <-time.After(20 * time.Millisecond):
+					newMetricData, err := te.client.GetMetrics(te.testCtx)
+					if err != nil {
+						t.Fatalf("Cannot get metrics: %v", err)
+					}
+					*metricDataPtr = newMetricData
+				case <-waitCtx.Done():
+				}
+			}
+			if err := check(); err != nil {
+				t.Error(err.Error())
+			}
+		})
+	}
+	if t.Failed() {
+		t.Logf("Last metric data:\n\n%s\n\n", metricData)
+	}
+}

test/metricclient/metricclient.go

@@ -351,6 +351,8 @@ type WantMetric struct {
 	// Pod and Namespace are the pod and namespace labels associated with the sandbox.
 	// Leave empty if the sandbox metadata doesn't contain this information.
 	Pod, Namespace string
+	// ExtraLabels are additional key-value labels that must match.
+	ExtraLabels map[string]string
 }
 
 // GetPrometheusContainerInteger returns the integer value of a Prometheus metric from the
@@ -365,6 +367,9 @@ func (m MetricData) GetPrometheusContainerInteger(want WantMetric) (int64, time.
 	if want.Namespace != "" {
 		labels["namespace_name"] = want.Namespace
 	}
+	for k, v := range want.ExtraLabels {
+		labels[k] = v
+	}
 	return m.GetPrometheusInteger(want.Metric, labels)
 }