diff --git a/staging/src/k8s.io/component-base/metrics/collector.go b/staging/src/k8s.io/component-base/metrics/collector.go index 113b903ec27..0718b6e1358 100644 --- a/staging/src/k8s.io/component-base/metrics/collector.go +++ b/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, these are collected from DescribeWithStability(). registerable map[string]*Desc // stores registerable descriptors by pair, 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() diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 9d359d6acb6..49d2d40bbf7 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/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" diff --git a/staging/src/k8s.io/component-base/metrics/opts_test.go b/staging/src/k8s.io/component-base/metrics/opts_test.go index 14e20f4bff4..46d5bade21f 100644 --- a/staging/src/k8s.io/component-base/metrics/opts_test.go +++ b/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", - inputValue: ALPHA, - expectValue: ALPHA, + name: "INTERNAL remain unchanged", + inputValue: INTERNAL, + expectValue: INTERNAL, expectPanic: false, }, { diff --git a/test/instrumentation/decode_metric.go b/test/instrumentation/decode_metric.go index fd770d69a0c..8150e1f6336 100644 --- a/test/instrumentation/decode_metric.go +++ b/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 { - return nil, newDecodeErrorf(expr, errInvalidNewMetricCall) + 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 } diff --git a/test/instrumentation/find_stable_metric.go b/test/instrumentation/find_stable_metric.go index 46605024a39..03605017946 100644 --- a/test/instrumentation/find_stable_metric.go +++ b/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: diff --git a/test/instrumentation/main_test.go b/test/instrumentation/main_test.go index bdb75b4b428..29f38475b4e 100644 --- a/test/instrumentation/main_test.go +++ b/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) diff --git a/test/instrumentation/testdata/pkg/kubelet/metrics/metrics.go b/test/instrumentation/testdata/pkg/kubelet/metrics/metrics.go index 7f95c87a32d..335e2961d62 100644 --- a/test/instrumentation/testdata/pkg/kubelet/metrics/metrics.go +++ b/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"}) +} diff --git a/test/instrumentation/testdata/test-stable-metrics-list.yaml b/test/instrumentation/testdata/test-stable-metrics-list.yaml index 3f9c1a7610e..94360c39c19 100644 --- a/test/instrumentation/testdata/test-stable-metrics-list.yaml +++ b/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