From 7d895374b01c23dd6ae7837fe6bcb8f62951f26d Mon Sep 17 00:00:00 2001 From: Andy Bursavich Date: Mon, 8 Jul 2024 17:30:12 -0700 Subject: [PATCH 1/2] pkg/naming: shorten regexp when matching many similar names --- docs/config.md | 4 +- pkg/naming/metrics_query.go | 91 +++++++++++++++++++------------- pkg/naming/metrics_query_test.go | 33 +++++++++--- 3 files changed, 81 insertions(+), 47 deletions(-) diff --git a/docs/config.md b/docs/config.md index 9d97c98a4..3e5cd0c35 100644 --- a/docs/config.md +++ b/docs/config.md @@ -196,8 +196,8 @@ Additionally, there are two advanced fields that are "raw" forms of other fields: - `LabelValuesByName`: a map mapping the labels and values from the - `LabelMatchers` field. The values are pre-joined by `|` - (for used with the `=~` matcher in Prometheus). + `LabelMatchers` field. The values are in regular expression format + (for use with the `=~` matcher in Prometheus). - `GroupBySlice`: the slice form of `GroupBy`. In general, you'll probably want to use the `Series`, `LabelMatchers`, and diff --git a/pkg/naming/metrics_query.go b/pkg/naming/metrics_query.go index 13b0bf03b..e9d7069b3 100644 --- a/pkg/naming/metrics_query.go +++ b/pkg/naming/metrics_query.go @@ -20,8 +20,11 @@ import ( "bytes" "errors" "fmt" + "regexp" + "regexp/syntax" "strings" "text/template" + "unicode/utf8" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" @@ -123,26 +126,25 @@ func (q *metricsQuery) Build(series string, resource schema.GroupResource, names }) } - exprs, valuesByName, err := q.processQueryParts(queryParts) + resourceLbl, err := q.resConverter.LabelForResource(resource) if err != nil { return "", err } + resourceOp := selection.DoesNotExist + if len(names) > 0 { + resourceOp = selection.Equals + } + queryParts = append(queryParts, queryPart{ + labelName: string(resourceLbl), + values: names, + operator: resourceOp, + }) - resourceLbl, err := q.resConverter.LabelForResource(resource) + exprs, valuesByName, err := q.processQueryParts(queryParts) if err != nil { return "", err } - matcher := prom.LabelEq - targetValue := strings.Join(names, "|") - - if len(names) > 1 { - matcher = prom.LabelMatches - } - - exprs = append(exprs, matcher(string(resourceLbl), targetValue)) - valuesByName[string(resourceLbl)] = targetValue - groupBy := make([]string, 0, len(extraGroupBy)+1) groupBy = append(groupBy, string(resourceLbl)) groupBy = append(groupBy, extraGroupBy...) @@ -167,17 +169,14 @@ func (q *metricsQuery) Build(series string, resource schema.GroupResource, names } func (q *metricsQuery) BuildExternal(seriesName string, namespace string, groupBy string, groupBySlice []string, metricSelector labels.Selector) (prom.Selector, error) { - queryParts := []queryPart{} - // Build up the query parts from the selector. - queryParts = append(queryParts, q.createQueryPartsFromSelector(metricSelector)...) + queryParts := q.createQueryPartsFromSelector(metricSelector) if q.namespaced && namespace != "" { namespaceLbl, err := q.resConverter.LabelForResource(NsGroupResource) if err != nil { return "", err } - queryParts = append(queryParts, queryPart{ labelName: string(namespaceLbl), values: []string{namespace}, @@ -187,7 +186,6 @@ func (q *metricsQuery) BuildExternal(seriesName string, namespace string, groupB // Convert our query parts into the types we need for our template. exprs, valuesByName, err := q.processQueryParts(queryParts) - if err != nil { return "", err } @@ -215,25 +213,15 @@ func (q *metricsQuery) BuildExternal(seriesName string, namespace string, groupB func (q *metricsQuery) createQueryPartsFromSelector(metricSelector labels.Selector) []queryPart { requirements, _ := metricSelector.Requirements() - selectors := []queryPart{} - for i := 0; i < len(requirements); i++ { - selector := q.convertRequirement(requirements[i]) - - selectors = append(selectors, selector) - } - - return selectors -} - -func (q *metricsQuery) convertRequirement(requirement labels.Requirement) queryPart { - labelName := requirement.Key() - values := requirement.Values().List() - - return queryPart{ - labelName: labelName, - values: values, - operator: requirement.Operator(), + var parts []queryPart + for _, req := range requirements { + parts = append(parts, queryPart{ + labelName: req.Key(), + values: req.Values().List(), + operator: req.Operator(), + }) } + return parts } func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[string]string, error) { @@ -264,7 +252,6 @@ func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[ } matcher, err := q.selectMatcher(qPart.operator, qPart.values) - if err != nil { return nil, nil, err } @@ -274,9 +261,9 @@ func (q *metricsQuery) processQueryParts(queryParts []queryPart) ([]string, map[ return nil, nil, err } + valuesByName[qPart.labelName] = targetValue expression := matcher(qPart.labelName, targetValue) exprs = append(exprs, expression) - valuesByName[qPart.labelName] = strings.Join(qPart.values, "|") } return exprs, valuesByName, nil @@ -355,7 +342,7 @@ func (q *metricsQuery) selectTargetValue(operator selection.Operator, values []s // They might choose to send an "IN" request and give a list of static values // or they could send a single value that's a regex, giving them a passthrough // for their label selector. - return strings.Join(values, "|"), nil + return compactRegexp(values) case selection.Exists, selection.DoesNotExist: return "", ErrQueryUnsupportedValues } @@ -367,3 +354,31 @@ func (q *metricsQuery) selectTargetValue(operator selection.Operator, values []s func (q *metricsQuery) operatorIsSupported(operator selection.Operator) bool { return operator != selection.GreaterThan && operator != selection.LessThan } + +// compactRegexp returns a regular expression that matches the given values. +// It makes a reasonable effort to return a short string by letting the +// regexp/syntax package simplify the naive expression. +func compactRegexp(values []string) (string, error) { + for _, v := range values { + if !utf8.ValidString(v) || regexp.QuoteMeta(v) != v { + return "", ErrMalformedQuery + } + } + if len(values) == 0 { + return "", nil + } + if len(values) == 1 { + return values[0], nil + } + expr := strings.Join(values, "|") + re, err := syntax.Parse(expr, syntax.POSIX) + if err != nil { + return "", ErrMalformedQuery + } + short := re.Simplify().String() + short = strings.ReplaceAll(short, "(?:", "(") // Remove non-capturing group prefix. + if len(expr) <= len(short) { + return expr, nil + } + return short, nil +} diff --git a/pkg/naming/metrics_query_test.go b/pkg/naming/metrics_query_test.go index 122d87c70..dd1f54a9b 100644 --- a/pkg/naming/metrics_query_test.go +++ b/pkg/naming/metrics_query_test.go @@ -17,6 +17,7 @@ limitations under the License. package naming import ( + "errors" "fmt" "testing" @@ -29,6 +30,8 @@ import ( pmodel "github.com/prometheus/common/model" ) +var errUnknownResource = errors.New("unknown resource") + type resourceConverterMock struct { namespaced bool } @@ -42,6 +45,9 @@ func (rcm *resourceConverterMock) ResourcesForSeries(series prom.Series) (res [] // 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) { + if gr.Resource == "" { + return "", errUnknownResource + } return pmodel.LabelName(gr.Resource), nil } @@ -106,10 +112,23 @@ func TestBuildSelector(t *testing.T) { check checkFunc }{ + { + name: "unknown resource", + + mq: mustNewQuery(`series <<.Series>>`, false), + metricSelector: labels.NewSelector(), + series: "foo", + + check: checks( + hasError(errUnknownResource), + ), + }, + { name: "series", mq: mustNewQuery(`series <<.Series>>`, false), + resource: schema.GroupResource{Group: "group", Resource: "resource"}, metricSelector: labels.NewSelector(), series: "foo", @@ -129,7 +148,7 @@ func TestBuildSelector(t *testing.T) { check: checks( hasError(nil), - hasSelector(`resource=~"bar|baz"`), + hasSelector(`resource=~"ba[rz]"`), ), }, @@ -183,11 +202,11 @@ func TestBuildSelector(t *testing.T) { mq: mustNewQuery(`<>`, false), metricSelector: labels.NewSelector(), resource: schema.GroupResource{Group: "group", Resource: "resource"}, - names: []string{"bar", "baz"}, + names: []string{"bar", "baz", "foo-26jf7", "foo-2hqnf", "foo-8dddc", "foo-kwlkg"}, check: checks( hasError(nil), - hasSelector("bar|baz"), + hasSelector("ba[rz]|foo-(2(6jf7|hqnf)|8dddc|kwlkg)"), ), }, @@ -198,11 +217,11 @@ func TestBuildSelector(t *testing.T) { metricSelector: labels.NewSelector(), resource: schema.GroupResource{Group: "group", Resource: "resource"}, namespace: "default", - names: []string{"bar", "baz"}, + names: []string{"bar", "baz", "foo-26jf7", "foo-2hqnf", "foo-8dddc", "foo-kwlkg"}, check: checks( hasError(nil), - hasSelector("default bar|baz"), + hasSelector("default ba[rz]|foo-(2(6jf7|hqnf)|8dddc|kwlkg)"), ), }, @@ -409,7 +428,7 @@ func TestBuildExternalSelector(t *testing.T) { check: checks( hasError(nil), - hasSelector(`foo="bar",qux=~"bar|baz"`), + hasSelector(`foo="bar",qux=~"ba[rz]"`), ), }, { @@ -435,7 +454,7 @@ func TestBuildExternalSelector(t *testing.T) { check: checks( hasError(nil), - hasSelector("map[foo:bar|baz]"), + hasSelector("map[foo:ba[rz]]"), ), }, } From 69ae2d12109ac90fb4cb9a753e133e04e79fd39b Mon Sep 17 00:00:00 2001 From: Andy Bursavich Date: Thu, 25 Jul 2024 18:24:57 -0700 Subject: [PATCH 2/2] pkg/custom-provider: update expected test queries --- pkg/custom-provider/series_registry_test.go | 6 +++--- pkg/naming/metrics_query.go | 6 ------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/custom-provider/series_registry_test.go b/pkg/custom-provider/series_registry_test.go index 09e440a7f..7dd5352d2 100644 --- a/pkg/custom-provider/series_registry_test.go +++ b/pkg/custom-provider/series_registry_test.go @@ -164,7 +164,7 @@ var _ = Describe("Series Registry", func() { resourceNames: []string{"somepod1", "somepod2"}, metricSelector: labels.Everything(), - expectedQuery: "sum(container_some_usage{namespace=\"somens\",pod=~\"somepod1|somepod2\",container!=\"POD\"}) by (pod)", + expectedQuery: "sum(container_some_usage{namespace=\"somens\",pod=~\"somepod[12]\",container!=\"POD\"}) by (pod)", }, { title: "container metrics counter", @@ -173,7 +173,7 @@ var _ = Describe("Series Registry", func() { resourceNames: []string{"somepod1", "somepod2"}, metricSelector: labels.Everything(), - expectedQuery: "sum(rate(container_some_count_total{namespace=\"somens\",pod=~\"somepod1|somepod2\",container!=\"POD\"}[1m])) by (pod)", + expectedQuery: "sum(rate(container_some_count_total{namespace=\"somens\",pod=~\"somepod[12]\",container!=\"POD\"}[1m])) by (pod)", }, { title: "container metrics seconds counter", @@ -182,7 +182,7 @@ var _ = Describe("Series Registry", func() { resourceNames: []string{"somepod1", "somepod2"}, metricSelector: labels.Everything(), - expectedQuery: "sum(rate(container_some_time_seconds_total{namespace=\"somens\",pod=~\"somepod1|somepod2\",container!=\"POD\"}[1m])) by (pod)", + expectedQuery: "sum(rate(container_some_time_seconds_total{namespace=\"somens\",pod=~\"somepod[12]\",container!=\"POD\"}[1m])) by (pod)", }, // namespaced metrics { diff --git a/pkg/naming/metrics_query.go b/pkg/naming/metrics_query.go index e9d7069b3..b6447a362 100644 --- a/pkg/naming/metrics_query.go +++ b/pkg/naming/metrics_query.go @@ -364,12 +364,6 @@ func compactRegexp(values []string) (string, error) { return "", ErrMalformedQuery } } - if len(values) == 0 { - return "", nil - } - if len(values) == 1 { - return values[0], nil - } expr := strings.Join(values, "|") re, err := syntax.Parse(expr, syntax.POSIX) if err != nil {