Merge pull request #112676 from logicalhan/stability-v2

Extend the metrics stability framework to support additional parses

staging/src/k8s.io/component-base/metrics/collector.go

@@ -45,9 +45,9 @@ type StableCollector interface {
 	HiddenMetrics() []string
 }
 
-// BaseStableCollector which implements almost all of the methods defined by StableCollector
+// BaseStableCollector which implements almost all methods defined by StableCollector
 // is a convenient assistant for custom collectors.
-// It is recommend that inherit BaseStableCollector when implementing custom collectors.
+// It is recommended to inherit BaseStableCollector when implementing custom collectors.
 type BaseStableCollector struct {
 	descriptors  map[string]*Desc // stores all descriptors by pair<fqName, Desc>, these are collected from DescribeWithStability().
 	registerable map[string]*Desc // stores registerable descriptors by pair<fqName, Desc>, is a subset of descriptors.
@@ -62,7 +62,7 @@ func (bsc *BaseStableCollector) DescribeWithStability(ch chan<- *Desc) {
 }
 
 // Describe sends all descriptors to the provided channel.
-// It intend to be called by prometheus registry.
+// It intended to be called by prometheus registry.
 func (bsc *BaseStableCollector) Describe(ch chan<- *prometheus.Desc) {
 	for _, d := range bsc.registerable {
 		ch <- d.toPrometheusDesc()

staging/src/k8s.io/component-base/metrics/opts.go

@@ -66,9 +66,15 @@ func BuildFQName(namespace, subsystem, name string) string {
 type StabilityLevel string
 
 const (
+	// INTERNAL metrics have no stability guarantees, as such, labels may
+	// be arbitrarily added/removed and the metric may be deleted at any time.
+	INTERNAL StabilityLevel = "INTERNAL"
 	// ALPHA metrics have no stability guarantees, as such, labels may
 	// be arbitrarily added/removed and the metric may be deleted at any time.
 	ALPHA StabilityLevel = "ALPHA"
+	// BETA metrics are governed by the deprecation policy outlined in by
+	// the control plane metrics stability KEP.
+	BETA StabilityLevel = "BETA"
 	// STABLE metrics are guaranteed not be mutated and removal is governed by
 	// the deprecation policy outlined in by the control plane metrics stability KEP.
 	STABLE StabilityLevel = "STABLE"

staging/src/k8s.io/component-base/metrics/opts_test.go

@@ -38,9 +38,9 @@ func TestDefaultStabilityLevel(t *testing.T) {
 			expectPanic: false,
 		},
 		{
-			name:        "ALPHA remain unchanged",
+			name:        "INTERNAL remain unchanged",
-			inputValue:  ALPHA,
+			inputValue:  INTERNAL,
-			expectValue: ALPHA,
+			expectValue: INTERNAL,
 			expectPanic: false,
 		},
 		{

test/instrumentation/decode_metric.go

@@ -17,6 +17,7 @@ limitations under the License.
 package main
 
 import (
+	"errors"
 	"fmt"
 	"go/ast"
 	"go/token"
@@ -40,7 +41,10 @@ func decodeMetricCalls(fs []*ast.CallExpr, metricsImportName string, variables m
 			errors = append(errors, err)
 			continue
 		}
-		ms = append(ms, m)
+		if m != nil {
+			ms = append(ms, *m)
+		}
+
 	}
 	return ms, errors
 }
@@ -50,34 +54,36 @@ type metricDecoder struct {
 	variables             map[string]ast.Expr
 }
 
-func (c *metricDecoder) decodeNewMetricCall(fc *ast.CallExpr) (metric, error) {
+func (c *metricDecoder) decodeNewMetricCall(fc *ast.CallExpr) (*metric, error) {
 	var m metric
 	var err error
 	se, ok := fc.Fun.(*ast.SelectorExpr)
 	if !ok {
-		return m, newDecodeErrorf(fc, errNotDirectCall)
+		return nil, newDecodeErrorf(fc, errNotDirectCall)
 	}
 	functionName := se.Sel.String()
 	functionImport, ok := se.X.(*ast.Ident)
 	if !ok {
-		return m, newDecodeErrorf(fc, errNotDirectCall)
+		return nil, newDecodeErrorf(fc, errNotDirectCall)
 	}
 	if functionImport.String() != c.kubeMetricsImportName {
-		return m, newDecodeErrorf(fc, errNotDirectCall)
+		return nil, nil
 	}
 	switch functionName {
 	case "NewCounter", "NewGauge", "NewHistogram", "NewSummary":
 		m, err = c.decodeMetric(fc)
 	case "NewCounterVec", "NewGaugeVec", "NewHistogramVec", "NewSummaryVec":
 		m, err = c.decodeMetricVec(fc)
+	case "Labels", "HandlerOpts", "HandlerFor", "HandlerWithReset":
+		return nil, nil
 	default:
-		return m, newDecodeErrorf(fc, errNotDirectCall)
+		return &m, newDecodeErrorf(fc, errNotDirectCall)
 	}
 	if err != nil {
-		return m, err
+		return &m, err
 	}
 	m.Type = getMetricType(functionName)
-	return m, nil
+	return &m, nil
 }
 
 func getMetricType(functionName string) string {
@@ -110,7 +116,7 @@ func (c *metricDecoder) decodeMetricVec(call *ast.CallExpr) (metric, error) {
 	if err != nil {
 		return m, err
 	}
-	labels, err := decodeLabels(call.Args[1])
+	labels, err := c.decodeLabels(call.Args[1])
 	if err != nil {
 		return m, err
 	}
@@ -119,16 +125,45 @@ func (c *metricDecoder) decodeMetricVec(call *ast.CallExpr) (metric, error) {
 	return m, nil
 }
 
-func decodeLabels(expr ast.Expr) ([]string, error) {
+func (c *metricDecoder) decodeLabels(expr ast.Expr) ([]string, error) {
 	cl, ok := expr.(*ast.CompositeLit)
+	if !ok {
+		id, ok := expr.(*ast.Ident)
 		if !ok {
 			return nil, newDecodeErrorf(expr, errInvalidNewMetricCall)
 		}
+		variableExpr, found := c.variables[id.Name]
+		if !found {
+			return nil, newDecodeErrorf(expr, "couldn't find variable for labels")
+		}
+		cl2, ok := variableExpr.(*ast.CompositeLit)
+		if !ok {
+			return nil, newDecodeErrorf(expr, "couldn't interpret variable for labels")
+		}
+		cl = cl2
+	}
 	labels := make([]string, len(cl.Elts))
 	for i, el := range cl.Elts {
+		v, ok := el.(*ast.Ident)
+		if ok {
+			variableExpr, found := c.variables[v.Name]
+			if !found {
+				return nil, newDecodeErrorf(expr, errBadVariableAttribute)
+			}
+			bl, ok := variableExpr.(*ast.BasicLit)
+			if !ok {
+				return nil, newDecodeErrorf(expr, errNonStringAttribute)
+			}
+			value, err := stringValue(bl)
+			if err != nil {
+				return nil, err
+			}
+			labels[i] = value
+			continue
+		}
 		bl, ok := el.(*ast.BasicLit)
 		if !ok {
-			return nil, newDecodeErrorf(bl, errLabels)
+			return nil, errors.New(errLabels)
 		}
 		value, err := stringValue(bl)
 		if err != nil {
@@ -200,7 +235,38 @@ func (c *metricDecoder) decodeOpts(expr ast.Expr) (metric, error) {
 				if err != nil {
 					return m, err
 				}
+			case *ast.BinaryExpr:
+				var binaryExpr *ast.BinaryExpr
+				binaryExpr = v
+				var okay bool
+				okay = true
 
+				for okay {
+					yV, okay := binaryExpr.Y.(*ast.BasicLit)
+					if !okay {
+						return m, newDecodeErrorf(expr, errNonStringAttribute)
+					}
+					yVal, err := stringValue(yV)
+					if err != nil {
+						return m, newDecodeErrorf(expr, errNonStringAttribute)
+					}
+					value = fmt.Sprintf("%s%s", yVal, value)
+					x, okay := binaryExpr.X.(*ast.BinaryExpr)
+					if !okay {
+						// should be basicLit
+						xV, okay := binaryExpr.X.(*ast.BasicLit)
+						if !okay {
+							return m, newDecodeErrorf(expr, errNonStringAttribute)
+						}
+						xVal, err := stringValue(xV)
+						if err != nil {
+							return m, newDecodeErrorf(expr, errNonStringAttribute)
+						}
+						value = fmt.Sprintf("%s%s", xVal, value)
+						break
+					}
+					binaryExpr = x
+				}
 			default:
 				return m, newDecodeErrorf(expr, errNonStringAttribute)
 			}
@@ -272,8 +338,19 @@ func stringValue(bl *ast.BasicLit) (string, error) {
 
 func (c *metricDecoder) decodeBuckets(expr ast.Expr) ([]float64, error) {
 	switch v := expr.(type) {
+	case *ast.Ident:
+		variableExpr, found := c.variables[v.Name]
+		if !found {
+			return nil, fmt.Errorf("couldn't find variable for bucket")
+		}
+		v2, ok := variableExpr.(*ast.CompositeLit)
+		if !ok {
+			return nil, fmt.Errorf("couldn't find variable for bucket")
+		}
+		return decodeListOfFloats(v2, v2.Elts)
+
 	case *ast.CompositeLit:
-		return decodeListOfFloats(v.Elts)
+		return decodeListOfFloats(v, v.Elts)
 	case *ast.SelectorExpr:
 		variableName := v.Sel.String()
 		importName, ok := v.X.(*ast.Ident)
@@ -415,12 +492,12 @@ func decodeFloatMap(exprs []ast.Expr) (map[float64]float64, error) {
 	return buckets, nil
 }
 
-func decodeListOfFloats(exprs []ast.Expr) ([]float64, error) {
+func decodeListOfFloats(expr ast.Expr, exprs []ast.Expr) ([]float64, error) {
 	buckets := make([]float64, len(exprs))
 	for i, elt := range exprs {
 		bl, ok := elt.(*ast.BasicLit)
 		if !ok {
-			return nil, newDecodeErrorf(bl, errBuckets)
+			return nil, newDecodeErrorf(expr, errBuckets)
 		}
 		if bl.Kind != token.FLOAT && bl.Kind != token.INT {
 			return nil, newDecodeErrorf(bl, errBuckets)
@@ -477,9 +554,7 @@ func decodeStabilityLevel(expr ast.Expr, metricsFrameworkImportName string) (*me
 	if s.String() != metricsFrameworkImportName {
 		return nil, newDecodeErrorf(expr, errStabilityLevel)
 	}
-	if se.Sel.Name != "ALPHA" && se.Sel.Name != "STABLE" {
+
-		return nil, newDecodeErrorf(expr, errStabilityLevel)
-	}
 	stability := metrics.StabilityLevel(se.Sel.Name)
 	return &stability, nil
 }

test/instrumentation/find_stable_metric.go

@@ -76,14 +76,14 @@ func (f *stableMetricFinder) Visit(node ast.Node) (w ast.Visitor) {
 			return nil
 		}
 		switch *stabilityLevel {
-		case metrics.STABLE:
+		case metrics.STABLE, metrics.BETA:
 			if f.currentFunctionCall == nil {
 				f.errors = append(f.errors, newDecodeErrorf(opts, errNotDirectCall))
 				return nil
 			}
 			f.stableMetricsFunctionCalls = append(f.stableMetricsFunctionCalls, f.currentFunctionCall)
 			f.currentFunctionCall = nil
-		case metrics.ALPHA:
+		case metrics.INTERNAL, metrics.ALPHA:
 			return nil
 		}
 	default:

test/instrumentation/main_test.go

@@ -608,18 +608,6 @@ var _ = metrics.NewCounter(
 			StabilityLevel: "stable",
 		},
 	)
-`},
-		{
-			testName: "error for passing stability as unknown const",
-			err:      fmt.Errorf("testdata/metric.go:6:20: StabilityLevel should be passed STABLE, ALPHA or removed"),
-			src: `
-package test
-import "k8s.io/component-base/metrics"
-var _ = metrics.NewCounter(
-		&metrics.CounterOpts{
-			StabilityLevel: metrics.UNKNOWN,
-		},
-	)
 `},
 		{
 			testName: "error for passing stability as variable",
@@ -670,18 +658,6 @@ var _ = RegisterMetric(
 			StabilityLevel: metrics.STABLE,
 		},
 	)
-`},
-		{
-			testName: "error stable metric opts passed to imported function",
-			err:      fmt.Errorf("testdata/metric.go:4:9: Opts for STABLE metric was not directly passed to new metric function"),
-			src: `
-package test
-import "k8s.io/component-base/metrics"
-var _ = test.RegisterMetric(
-		&metrics.CounterOpts{
-			StabilityLevel: metrics.STABLE,
-		},
-	)
 `},
 		{
 			testName: "error stable metric opts passed to imported function",
@@ -724,21 +700,6 @@ var _ = metrics.NewSummary(
 			Objectives: prometheus.FakeObjectives,
 		},
 	)
-`},
-		{
-			testName: "error stable histogram with unknown bucket variable",
-			err:      fmt.Errorf("testdata/metric.go:9:13: Buckets should be set to list of floats, result from function call of prometheus.LinearBuckets or prometheus.ExponentialBuckets"),
-			src: `
-package test
-import "k8s.io/component-base/metrics"
-var buckets = []float64{1, 2, 3}
-var _ = metrics.NewHistogram(
-		&metrics.HistogramOpts{
-			Name: "histogram",
-			StabilityLevel: metrics.STABLE,
-			Buckets: buckets,
-		},
-	)
 `},
 		{
 			testName: "error stable historgram with unknown bucket variable from unknown library",
@@ -759,7 +720,7 @@ var _ = metrics.NewHistogram(
 		t.Run(test.testName, func(t *testing.T) {
 			_, errors := searchFileForStableMetrics(fakeFilename, test.src)
 			if len(errors) != 1 {
-				t.Fatalf("Unexpected number of errors, got %d, want 1", len(errors))
+				t.Errorf("Unexpected number of errors, got %d, want 1", len(errors))
 			}
 			if !reflect.DeepEqual(errors[0], test.err) {
 				t.Errorf("error:\ngot  %v\nwant %v", errors[0], test.err)

test/instrumentation/testdata/pkg/kubelet/metrics/metrics.go

@@ -65,6 +65,8 @@ const (
 
 var (
 	defObjectives = map[float64]float64{0.5: 0.5, 0.75: 0.75}
+	testBuckets   = []float64{0, 0.5, 1.0}
+	testLabels    = []string{"a", "b", "c"}
 	// NodeName is a Gauge that tracks the ode's name. The count is always 1.
 	NodeName = metrics.NewGaugeVec(
 		&metrics.GaugeOpts{
@@ -81,7 +83,7 @@ var (
 			Subsystem:      KubeletSubsystem,
 			Name:           "containers_per_pod_count",
 			Help:           "The number of containers per pod.",
-			Buckets:        metrics.ExponentialBuckets(1, 2, 5),
+			Buckets:        testBuckets,
 			StabilityLevel: metrics.ALPHA,
 		},
 	)
@@ -95,7 +97,7 @@ var (
 			Buckets:        metrics.DefBuckets,
 			StabilityLevel: metrics.ALPHA,
 		},
-		[]string{"operation_type"},
+		testLabels,
 	)
 	// PodStartDuration is a Histogram that tracks the duration (in seconds) it takes for a single pod to go from pending to running.
 	PodStartDuration = metrics.NewHistogram(
@@ -190,7 +192,7 @@ var (
 			Name:           RuntimeOperationsDurationKey,
 			Help:           "Duration in seconds of runtime operations. Broken down by operation type.",
 			Buckets:        metrics.ExponentialBuckets(.005, 2.5, 14),
-			StabilityLevel: metrics.ALPHA,
+			StabilityLevel: metrics.BETA,
 		},
 		[]string{"operation_type"},
 	)
@@ -240,6 +242,18 @@ var (
 		},
 		[]string{"preemption_signal"},
 	)
+	// MultiLineHelp tests that we can parse multi-line strings
+	MultiLineHelp = metrics.NewCounterVec(
+		&metrics.CounterOpts{
+			Subsystem: KubeletSubsystem,
+			Name:      "multiline",
+			Help: "Cumulative number of pod preemptions by preemption resource " +
+				"asdf asdf asdf " +
+				"asdfas dfasdf",
+			StabilityLevel: metrics.STABLE,
+		},
+		[]string{"preemption_signal"},
+	)
 	// DevicePluginRegistrationCount is a Counter that tracks the cumulative number of device plugin registrations.
 	// Broken down by resource name.
 	DevicePluginRegistrationCount = metrics.NewCounterVec(
@@ -259,7 +273,7 @@ var (
 			Name:           DevicePluginAllocationDurationKey,
 			Help:           "Duration in seconds to serve a device plugin Allocation request. Broken down by resource name.",
 			Buckets:        metrics.DefBuckets,
-			StabilityLevel: metrics.ALPHA,
+			StabilityLevel: metrics.BETA,
 		},
 		[]string{"resource_name"},
 	)
@@ -445,3 +459,7 @@ func SinceInSeconds(start time.Time) float64 {
 func SetNodeName(name types.NodeName) {
 	NodeName.WithLabelValues(string(name)).Set(1)
 }
+
+func Blah() metrics.ObserverMetric {
+	return EvictionStatsAge.With(metrics.Labels{"plugins": "ASDf"})
+}

test/instrumentation/testdata/test-stable-metrics-list.yaml

@@ -1,3 +1,53 @@
+- name: device_plugin_alloc_duration_seconds
+  subsystem: kubelet
+  help: Duration in seconds to serve a device plugin Allocation request. Broken down
+    by resource name.
+  type: Histogram
+  stabilityLevel: BETA
+  labels:
+  - resource_name
+  buckets:
+  - 0.005
+  - 0.01
+  - 0.025
+  - 0.05
+  - 0.1
+  - 0.25
+  - 0.5
+  - 1
+  - 2.5
+  - 5
+  - 10
+- name: multiline
+  subsystem: kubelet
+  help: Cumulative number of pod preemptions by preemption resource asdf asdf asdf
+    asdfas dfasdf
+  type: Counter
+  stabilityLevel: STABLE
+  labels:
+  - preemption_signal
+- name: runtime_operations_duration_seconds
+  subsystem: kubelet
+  help: Duration in seconds of runtime operations. Broken down by operation type.
+  type: Histogram
+  stabilityLevel: BETA
+  labels:
+  - operation_type
+  buckets:
+  - 0.005
+  - 0.0125
+  - 0.03125
+  - 0.078125
+  - 0.1953125
+  - 0.48828125
+  - 1.220703125
+  - 3.0517578125
+  - 7.62939453125
+  - 19.073486328125
+  - 47.6837158203125
+  - 119.20928955078125
+  - 298.0232238769531
+  - 745.0580596923828
 - name: summary_metric_test
   subsystem: kubelet
   help: Cumulative number of device plugin registrations. Broken down by resource