From 4dd546cf2cc410e6601d5d69d2f751d3de09c15f Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Fri, 25 Oct 2019 18:49:47 +0200 Subject: [PATCH] Allow usage of consts and variables for stable metrics in static analysis --- test/instrumentation/decode_metric.go | 52 +++++++++++---- test/instrumentation/error.go | 1 + test/instrumentation/main.go | 28 +++++--- test/instrumentation/main_test.go | 95 ++++++++++++++++++++++++--- 4 files changed, 144 insertions(+), 32 deletions(-) diff --git a/test/instrumentation/decode_metric.go b/test/instrumentation/decode_metric.go index 7c0f5a5d63c..721ab9738b1 100644 --- a/test/instrumentation/decode_metric.go +++ b/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) diff --git a/test/instrumentation/error.go b/test/instrumentation/error.go index 81ea88e8ba6..bc3942522d8 100644 --- a/test/instrumentation/error.go +++ b/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" diff --git a/test/instrumentation/main.go b/test/instrumentation/main.go index 96d9655bfc9..0e5739a8426 100644 --- a/test/instrumentation/main.go +++ b/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 +} diff --git a/test/instrumentation/main_test.go b/test/instrumentation/main_test.go index 4e856b4f385..33e833e9699 100644 --- a/test/instrumentation/main_test.go +++ b/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, }, )