Merge pull request #84373 from serathius/metrics-stability-variables

Allow usage of consts and variables for stable metrics in static analysis

test/instrumentation/decode_metric.go

@@ -27,10 +27,10 @@ import (
 	"k8s.io/component-base/metrics"
 )
 
-func decodeMetricCalls(fs []*ast.CallExpr, metricsImportName, prometheusImportName string) ([]metric, []error) {
+func decodeMetricCalls(fs []*ast.CallExpr, metricsImportName string, variables map[string]ast.Expr) ([]metric, []error) {
 	finder := metricDecoder{
 		kubeMetricsImportName: metricsImportName,
-		prometheusImportName:  prometheusImportName,
+		variables:             variables,
 	}
 	ms := make([]metric, 0, len(fs))
 	errors := []error{}
@@ -47,7 +47,7 @@ func decodeMetricCalls(fs []*ast.CallExpr, metricsImportName, prometheusImportNa
 
 type metricDecoder struct {
 	kubeMetricsImportName string
-	prometheusImportName  string
+	variables             map[string]ast.Expr
 }
 
 func (c *metricDecoder) decodeNewMetricCall(fc *ast.CallExpr) (metric, error) {
@@ -130,10 +130,11 @@ func decodeLabels(expr ast.Expr) ([]string, error) {
 		if !ok {
 			return nil, newDecodeErrorf(bl, errLabels)
 		}
-		if bl.Kind != token.STRING {
-			return nil, newDecodeErrorf(bl, errLabels)
+		value, err := stringValue(bl)
+		if err != nil {
+			return nil, err
 		}
-		labels[i] = strings.Trim(bl.Value, `"`)
+		labels[i] = value
 	}
 	return labels, nil
 }
@@ -160,14 +161,30 @@ func (c *metricDecoder) decodeOpts(expr ast.Expr) (metric, error) {
 
 		switch key {
 		case "Namespace", "Subsystem", "Name", "Help", "DeprecatedVersion":
-			k, ok := kv.Value.(*ast.BasicLit)
-			if !ok {
+			var value string
+			var err error
+			switch v := kv.Value.(type) {
+			case *ast.BasicLit:
+				value, err = stringValue(v)
+				if err != nil {
+					return m, err
+				}
+			case *ast.Ident:
+				variableExpr, found := c.variables[v.Name]
+				if !found {
+					return m, newDecodeErrorf(expr, errBadVariableAttribute)
+				}
+				bl, ok := variableExpr.(*ast.BasicLit)
+				if !ok {
+					return m, newDecodeErrorf(expr, errNonStringAttribute)
+				}
+				value, err = stringValue(bl)
+				if err != nil {
+					return m, err
+				}
+			default:
 				return m, newDecodeErrorf(expr, errNonStringAttribute)
 			}
-			if k.Kind != token.STRING {
-				return m, newDecodeErrorf(expr, errNonStringAttribute)
-			}
-			value := strings.Trim(k.Value, `"`)
 			switch key {
 			case "Namespace":
 				m.Namespace = value
@@ -200,6 +217,13 @@ func (c *metricDecoder) decodeOpts(expr ast.Expr) (metric, error) {
 	return m, nil
 }
 
+func stringValue(bl *ast.BasicLit) (string, error) {
+	if bl.Kind != token.STRING {
+		return "", newDecodeErrorf(bl, errNonStringAttribute)
+	}
+	return strings.Trim(bl.Value, `"`), nil
+}
+
 func (c *metricDecoder) decodeBuckets(expr ast.Expr) ([]float64, error) {
 	switch v := expr.(type) {
 	case *ast.CompositeLit:
@@ -207,7 +231,7 @@ func (c *metricDecoder) decodeBuckets(expr ast.Expr) ([]float64, error) {
 	case *ast.SelectorExpr:
 		variableName := v.Sel.String()
 		importName, ok := v.X.(*ast.Ident)
-		if ok && importName.String() == c.prometheusImportName && variableName == "DefBuckets" {
+		if ok && importName.String() == c.kubeMetricsImportName && variableName == "DefBuckets" {
 			return metrics.DefBuckets, nil
 		}
 	case *ast.CallExpr:
@@ -220,7 +244,7 @@ func (c *metricDecoder) decodeBuckets(expr ast.Expr) ([]float64, error) {
 		if !ok {
 			return nil, newDecodeErrorf(v, errBuckets)
 		}
-		if functionImport.String() != c.prometheusImportName {
+		if functionImport.String() != c.kubeMetricsImportName {
 			return nil, newDecodeErrorf(v, errBuckets)
 		}
 		firstArg, secondArg, thirdArg, err := decodeBucketArguments(v)

test/instrumentation/error.go

@@ -29,6 +29,7 @@ const (
 	errStableSummary        = "Stable summary metric is not supported"
 	errInvalidNewMetricCall = "Invalid new metric call, please ensure code compiles"
 	errNonStringAttribute   = "Non string attribute it not supported"
+	errBadVariableAttribute = "Metric attribute was not correctly set. Please use only global consts in same file"
 	errFieldNotSupported    = "Field %s is not supported"
 	errBuckets              = "Buckets should be set to list of floats, result from function call of prometheus.LinearBuckets or prometheus.ExponentialBuckets"
 	errLabels               = "Labels were not set to list of strings"

test/instrumentation/main.go

@@ -35,9 +35,6 @@ const (
 	kubeMetricImportPath = `"k8s.io/component-base/metrics"`
 	// Should equal to final directory name of kubeMetricImportPath
 	kubeMetricsDefaultImportName = "metrics"
-	prometheusImportPath         = `"github.com/prometheus/client_golang/prometheus"`
-	// Should equal to final directory name of kubeMetricImportPath
-	prometheusDefaultImportName = "prometheus"
 )
 
 func main() {
@@ -121,13 +118,10 @@ func searchFileForStableMetrics(filename string, src interface{}) ([]metric, []e
 	if metricsImportName == "" {
 		return []metric{}, []error{}
 	}
-	prometheusImportName, err := getLocalNameOfImportedPackage(tree, prometheusImportPath, prometheusDefaultImportName)
-	if err != nil {
-		return []metric{}, addFileInformationToErrors([]error{err}, fileset)
-	}
+	variables := globalVariableDeclarations(tree)
 
 	stableMetricsFunctionCalls, errors := findStableMetricDeclaration(tree, metricsImportName)
-	metrics, es := decodeMetricCalls(stableMetricsFunctionCalls, metricsImportName, prometheusImportName)
+	metrics, es := decodeMetricCalls(stableMetricsFunctionCalls, metricsImportName, variables)
 	errors = append(errors, es...)
 	return metrics, addFileInformationToErrors(errors, fileset)
 }
@@ -157,3 +151,21 @@ func addFileInformationToErrors(es []error, fileset *token.FileSet) []error {
 	}
 	return es
 }
+
+func globalVariableDeclarations(tree *ast.File) map[string]ast.Expr {
+	consts := make(map[string]ast.Expr)
+	for _, d := range tree.Decls {
+		if gd, ok := d.(*ast.GenDecl); ok && (gd.Tok == token.CONST || gd.Tok == token.VAR) {
+			for _, spec := range gd.Specs {
+				if vspec, ok := spec.(*ast.ValueSpec); ok {
+					for _, name := range vspec.Names {
+						for _, value := range vspec.Values {
+							consts[name.Name] = value
+						}
+					}
+				}
+			}
+		}
+	}
+	return consts
+}

test/instrumentation/main_test.go

@@ -298,6 +298,85 @@ var _ = custom.NewCounter(
 			StabilityLevel: custom.STABLE,
 		},
 	)
+`},
+		{
+			testName: "Const",
+			metric: metric{
+				Name:           "metric",
+				StabilityLevel: "STABLE",
+				Type:           counterMetricType,
+			},
+			src: `
+package test
+import "k8s.io/component-base/metrics"
+const name = "metric"
+var _ = metrics.NewCounter(
+		&metrics.CounterOpts{
+			Name:           name,
+			StabilityLevel: metrics.STABLE,
+		},
+	)
+`},
+		{
+			testName: "Variable",
+			metric: metric{
+				Name:           "metric",
+				StabilityLevel: "STABLE",
+				Type:           counterMetricType,
+			},
+			src: `
+package test
+import "k8s.io/component-base/metrics"
+var name = "metric"
+var _ = metrics.NewCounter(
+		&metrics.CounterOpts{
+			Name:           name,
+			StabilityLevel: metrics.STABLE,
+		},
+	)
+`},
+		{
+			testName: "Multiple consts in block",
+			metric: metric{
+				Name:           "metric",
+				StabilityLevel: "STABLE",
+				Type:           counterMetricType,
+			},
+			src: `
+package test
+import "k8s.io/component-base/metrics"
+const (
+ unrelated1 = "unrelated1"
+ name = "metric"
+ unrelated2 = "unrelated2"
+)
+var _ = metrics.NewCounter(
+		&metrics.CounterOpts{
+			Name:           name,
+			StabilityLevel: metrics.STABLE,
+		},
+	)
+`},
+		{
+			testName: "Multiple variables in Block",
+			metric: metric{
+				Name:           "metric",
+				StabilityLevel: "STABLE",
+				Type:           counterMetricType,
+			},
+			src: `
+package test
+import "k8s.io/component-base/metrics"
+var (
+ unrelated1 = "unrelated1"
+ name = "metric"
+ _ = metrics.NewCounter(
+		&metrics.CounterOpts{
+			Name:           name,
+			StabilityLevel: metrics.STABLE,
+		},
+	)
+)
 `},
 		{
 			testName: "Histogram with linear buckets",
@@ -310,12 +389,11 @@ var _ = custom.NewCounter(
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-import "github.com/prometheus/client_golang/prometheus"
 var _ = metrics.NewHistogram(
 		&metrics.HistogramOpts{
 			Name: "histogram",
 			StabilityLevel: metrics.STABLE,
-			Buckets: prometheus.LinearBuckets(1, 1, 3),
+			Buckets: metrics.LinearBuckets(1, 1, 3),
 		},
 	)
 `},
@@ -330,12 +408,11 @@ var _ = metrics.NewHistogram(
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-import "github.com/prometheus/client_golang/prometheus"
 var _ = metrics.NewHistogram(
 		&metrics.HistogramOpts{
 			Name: "histogram",
 			StabilityLevel: metrics.STABLE,
-			Buckets: prometheus.ExponentialBuckets(1, 2, 3),
+			Buckets: metrics.ExponentialBuckets(1, 2, 3),
 		},
 	)
 `},
@@ -350,12 +427,11 @@ var _ = metrics.NewHistogram(
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-import "github.com/prometheus/client_golang/prometheus"
 var _ = metrics.NewHistogram(
 		&metrics.HistogramOpts{
 			Name: "histogram",
 			StabilityLevel: metrics.STABLE,
-			Buckets: prometheus.DefBuckets,
+			Buckets: metrics.DefBuckets,
 		},
 	)
 `},
@@ -397,15 +473,14 @@ var _ = metrics.NewSummary(
 	)
 `},
 		{
-			testName: "Fail on stable metric with attribute set to variable",
-			err:      fmt.Errorf("testdata/metric.go:7:4: Non string attribute it not supported"),
+			testName: "Fail on stable metric with attribute set to unknown variable",
+			err:      fmt.Errorf("testdata/metric.go:6:4: Metric attribute was not correctly set. Please use only global consts in same file"),
 			src: `
 package test
 import "k8s.io/component-base/metrics"
-const name = "metric"
 var _ = metrics.NewCounter(
 		&metrics.CounterOpts{
-			Name:           name,
+			Name:           unknownVariable,
 			StabilityLevel: metrics.STABLE,
 		},
 	)