pkg/naming: shorten regexp when matching many similar names

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

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
 			{

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,25 @@ 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
+		}
+	}
+	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
+}

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(`<<index .LabelValuesByName "resource">>`, 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]]"),
 			),
 		},
 	}