From 9e072b2b5769324efd6f82da7536a596ea719a7f Mon Sep 17 00:00:00 2001 From: Sergiusz Urbaniak Date: Tue, 7 May 2019 14:04:29 +0200 Subject: [PATCH] pkg/naming: fix LabelValuesByName rendering According to documentation LabelValuesByName is supposed to render "|" separated values. Currently it is returned as a slice per label name. This fixes it Fixes #191 --- pkg/naming/metrics_query.go | 22 +-- pkg/naming/metrics_query_test.go | 234 +++++++++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 10 deletions(-) create mode 100644 pkg/naming/metrics_query_test.go diff --git a/pkg/naming/metrics_query.go b/pkg/naming/metrics_query.go index 07a7e084..a38e6785 100644 --- a/pkg/naming/metrics_query.go +++ b/pkg/naming/metrics_query.go @@ -74,7 +74,7 @@ type metricsQuery struct { type queryTemplateArgs struct { Series string LabelMatchers string - LabelValuesByName map[string][]string + LabelValuesByName map[string]string GroupBy string GroupBySlice []string } @@ -87,7 +87,7 @@ type queryPart struct { func (q *metricsQuery) Build(series string, resource schema.GroupResource, namespace string, extraGroupBy []string, names ...string) (prom.Selector, error) { var exprs []string - valuesByName := map[string][]string{} + valuesByName := map[string]string{} if namespace != "" { namespaceLbl, err := q.resConverter.LabelForResource(NsGroupResource) @@ -95,21 +95,23 @@ func (q *metricsQuery) Build(series string, resource schema.GroupResource, names return "", err } exprs = append(exprs, prom.LabelEq(string(namespaceLbl), namespace)) - valuesByName[string(namespaceLbl)] = []string{namespace} + valuesByName[string(namespaceLbl)] = namespace } resourceLbl, err := q.resConverter.LabelForResource(resource) if err != nil { return "", err } + matcher := prom.LabelEq - targetValue := names[0] + targetValue := strings.Join(names, "|") + if len(names) > 1 { matcher = prom.LabelMatches - targetValue = strings.Join(names, "|") } + exprs = append(exprs, matcher(string(resourceLbl), targetValue)) - valuesByName[string(resourceLbl)] = names + valuesByName[string(resourceLbl)] = targetValue groupBy := make([]string, 0, len(extraGroupBy)+1) groupBy = append(groupBy, string(resourceLbl)) @@ -191,7 +193,7 @@ func (q *metricsQuery) convertRequirement(requirement labels.Requirement) queryP } } -func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[string][]string, error) { +func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[string]string, error) { // We've take the approach here that if we can't perfectly map their query into a Prometheus // query that we should abandon the effort completely. // The concern is that if we don't get a perfect match on their query parameters, the query result @@ -203,8 +205,8 @@ func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[ var exprs []string // Contains the list of label values we're targeting, by namespace. - // e.g. "some_label" => ["value-one", "value-two"] - valuesByName := map[string][]string{} + // e.g. "some_label" => "value-one|value-two" + valuesByName := map[string]string{} // Convert our query parts into template arguments. for _, qPart := range queryParts { @@ -231,7 +233,7 @@ func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[ expression := matcher(qPart.labelName, targetValue) exprs = append(exprs, expression) - valuesByName[qPart.labelName] = qPart.values + valuesByName[qPart.labelName] = strings.Join(qPart.values, "|") } return exprs, valuesByName, nil diff --git a/pkg/naming/metrics_query_test.go b/pkg/naming/metrics_query_test.go new file mode 100644 index 00000000..8213f2bf --- /dev/null +++ b/pkg/naming/metrics_query_test.go @@ -0,0 +1,234 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package naming + +import ( + "fmt" + "testing" + + prom "github.com/directxman12/k8s-prometheus-adapter/pkg/client" + pmodel "github.com/prometheus/common/model" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type resourceConverterMock struct { + namespaced bool +} + +// ResourcesForSeries is a mock that returns a single group resource, +// namely the series as a resource itself. +func (rcm *resourceConverterMock) ResourcesForSeries(series prom.Series) (res []schema.GroupResource, namespaced bool) { + return []schema.GroupResource{{Resource: series.Name}}, false +} + +// LabelForResource is a mock that returns the label name, +// simply by taking the given resource. +func (rcm *resourceConverterMock) LabelForResource(gr schema.GroupResource) (pmodel.LabelName, error) { + return pmodel.LabelName(gr.Resource), nil +} + +func TestBuildSelector(t *testing.T) { + type checkFunc func(prom.Selector, error) error + + hasError := func(want error) checkFunc { + return func(_ prom.Selector, got error) error { + if want != got { + return fmt.Errorf("got error %v, want %v", got, want) + } + return nil + } + } + + hasSelector := func(want string) checkFunc { + return func(got prom.Selector, _ error) error { + if prom.Selector(want) != got { + return fmt.Errorf("got selector %q, want %q", got, want) + } + return nil + } + } + + checks := func(cs ...checkFunc) checkFunc { + return func(s prom.Selector, e error) error { + for _, c := range cs { + if err := c(s, e); err != nil { + return err + } + } + return nil + } + } + + mustNewQuery := func(queryTemplate string, namespaced bool) MetricsQuery { + mq, err := NewMetricsQuery(queryTemplate, &resourceConverterMock{namespaced}) + if err != nil { + t.Fatal(err) + } + return mq + } + + tests := []struct { + name string + mq MetricsQuery + + series string + resource schema.GroupResource + namespace string + extraGroupBy []string + names []string + + check checkFunc + }{ + { + name: "series", + + mq: mustNewQuery(`series <<.Series>>`, false), + series: "foo", + + check: checks( + hasError(nil), + hasSelector("series foo"), + ), + }, + + { + name: "multiple LabelMatchers values", + + mq: mustNewQuery(`<<.LabelMatchers>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + names: []string{"bar", "baz"}, + + check: checks( + hasError(nil), + hasSelector(`resource=~"bar|baz"`), + ), + }, + + { + name: "single LabelMatchers value", + + mq: mustNewQuery(`<<.LabelMatchers>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + names: []string{"bar"}, + + check: checks( + hasError(nil), + hasSelector(`resource="bar"`), + ), + }, + + { + name: "single LabelValuesByName value", + + mq: mustNewQuery(`<>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + names: []string{"bar"}, + + check: checks( + hasError(nil), + hasSelector("bar"), + ), + }, + + { + name: "multiple LabelValuesByName values", + + mq: mustNewQuery(`<>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + names: []string{"bar", "baz"}, + + check: checks( + hasError(nil), + hasSelector("bar|baz"), + ), + }, + + { + name: "multiple LabelValuesByName values with namespace", + + mq: mustNewQuery(`<> <>`, true), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + namespace: "default", + names: []string{"bar", "baz"}, + + check: checks( + hasError(nil), + hasSelector("default bar|baz"), + ), + }, + + { + name: "single GroupBy value", + + mq: mustNewQuery(`<<.GroupBy>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + + check: checks( + hasError(nil), + hasSelector("resource"), + ), + }, + + { + name: "multiple GroupBySlice values", + + mq: mustNewQuery(`<<.GroupBy>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + extraGroupBy: []string{"extra", "groups"}, + + check: checks( + hasError(nil), + hasSelector("resource,extra,groups"), + ), + }, + + { + name: "single GroupBySlice value", + + mq: mustNewQuery(`<<.GroupBySlice>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + + check: checks( + hasError(nil), + hasSelector("[resource]"), + ), + }, + + { + name: "multiple GroupBy values", + + mq: mustNewQuery(`<<.GroupBySlice>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, + extraGroupBy: []string{"extra", "groups"}, + + check: checks( + hasError(nil), + hasSelector("[resource extra groups]"), + ), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + selector, err := tc.mq.Build(tc.series, tc.resource, tc.namespace, tc.extraGroupBy, tc.names...) + + if err := tc.check(selector, err); err != nil { + t.Error(err) + } + }) + } +}