labels: Fixed label matching using matchers + external labels; unify …

…tests and behaviour. (#4690)

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

pkg/promclient/promclient.go

@@ -704,15 +704,15 @@ func (c *Client) SeriesInGRPC(ctx context.Context, base *url.URL, matchers []*la
 	return m.Data, c.get2xxResultWithGRPCErrors(ctx, "/prom_series HTTP[client]", &u, &m)
 }
 
-// LabelNames returns all known label names constrained by the given matchers. It uses gRPC errors.
+// LabelNamesInGRPC returns all known label names constrained by the given matchers. It uses gRPC errors.
 // NOTE: This method is tested in pkg/store/prometheus_test.go against Prometheus.
-func (c *Client) LabelNamesInGRPC(ctx context.Context, base *url.URL, matchers []storepb.LabelMatcher, startTime, endTime int64) ([]string, error) {
+func (c *Client) LabelNamesInGRPC(ctx context.Context, base *url.URL, matchers []*labels.Matcher, startTime, endTime int64) ([]string, error) {
 	u := *base
 	u.Path = path.Join(u.Path, "/api/v1/labels")
 	q := u.Query()
 
 	if len(matchers) > 0 {
-		q.Add("match[]", storepb.MatchersToString(matchers...))
+		q.Add("match[]", storepb.PromMatchersToString(matchers...))
 	}
 	q.Add("start", formatTime(timestamp.Time(startTime)))
 	q.Add("end", formatTime(timestamp.Time(endTime)))
@@ -726,13 +726,13 @@ func (c *Client) LabelNamesInGRPC(ctx context.Context, base *url.URL, matchers [
 
 // LabelValuesInGRPC returns all known label values for a given label name. It uses gRPC errors.
 // NOTE: This method is tested in pkg/store/prometheus_test.go against Prometheus.
-func (c *Client) LabelValuesInGRPC(ctx context.Context, base *url.URL, label string, matchers []storepb.LabelMatcher, startTime, endTime int64) ([]string, error) {
+func (c *Client) LabelValuesInGRPC(ctx context.Context, base *url.URL, label string, matchers []*labels.Matcher, startTime, endTime int64) ([]string, error) {
 	u := *base
 	u.Path = path.Join(u.Path, "/api/v1/label/", label, "/values")
 	q := u.Query()
 
 	if len(matchers) > 0 {
-		q.Add("match[]", storepb.MatchersToString(matchers...))
+		q.Add("match[]", storepb.PromMatchersToString(matchers...))
 	}
 	q.Add("start", formatTime(timestamp.Time(startTime)))
 	q.Add("end", formatTime(timestamp.Time(endTime)))

pkg/store/acceptance_test.go

@@ -0,0 +1,239 @@
+// Copyright (c) The Thanos Authors.
+// Licensed under the Apache License 2.0.
+
+package store
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/pkg/errors"
+	"github.com/prometheus/prometheus/pkg/labels"
+	"github.com/prometheus/prometheus/pkg/timestamp"
+	"github.com/prometheus/prometheus/storage"
+	"github.com/thanos-io/thanos/pkg/store/storepb"
+	"github.com/thanos-io/thanos/pkg/testutil"
+)
+
+type labelNameCallCase struct {
+	matchers []storepb.LabelMatcher
+	start    int64
+	end      int64
+
+	expectedNames []string
+	expectErr     error
+}
+
+type labelValuesCallCase struct {
+	label string
+
+	matchers []storepb.LabelMatcher
+	start    int64
+	end      int64
+
+	expectedValues []string
+	expectErr      error
+}
+
+// testLabelAPIs tests labels methods from StoreAPI from closed box perspective.
+func testLabelAPIs(t *testing.T, startStore func(extLset labels.Labels, append func(app storage.Appender)) storepb.StoreServer) {
+	t.Helper()
+
+	now := time.Now()
+	extLset := labels.FromStrings("region", "eu-west")
+	for _, tc := range []struct {
+		desc             string
+		appendFn         func(app storage.Appender)
+		labelNameCalls   []labelNameCallCase
+		labelValuesCalls []labelValuesCallCase
+	}{
+		{
+			desc: "no label in tsdb, empty results",
+			labelNameCalls: []labelNameCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime)},
+			},
+			labelValuesCalls: []labelValuesCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), expectErr: errors.New("rpc error: code = InvalidArgument desc = label name parameter cannot be empty")},
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "foo"},
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "region", expectedValues: []string{"eu-west"}}, // External labels should be visible.
+			},
+		},
+		{
+			desc: "{foo=foovalue1} 1",
+			appendFn: func(app storage.Appender) {
+				_, err := app.Append(0, labels.FromStrings("foo", "foovalue1"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				testutil.Ok(t, app.Commit())
+			},
+			labelNameCalls: []labelNameCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), expectedNames: []string{"foo", "region"}},
+			},
+			labelValuesCalls: []labelValuesCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "foo", expectedValues: []string{"foovalue1"}},
+			},
+		},
+		{
+			desc: "{foo=foovalue2} 1 and {foo=foovalue2} 1",
+			appendFn: func(app storage.Appender) {
+				_, err := app.Append(0, labels.FromStrings("foo", "foovalue1"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				_, err = app.Append(0, labels.FromStrings("foo", "foovalue2"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				testutil.Ok(t, app.Commit())
+			},
+			labelNameCalls: []labelNameCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), expectedNames: []string{"foo", "region"}},
+			},
+			labelValuesCalls: []labelValuesCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "foo", expectedValues: []string{"foovalue1", "foovalue2"}},
+			},
+		},
+		{
+			desc: "{foo=foovalue1, bar=barvalue1} 1 and {foo=foovalue2} 1 and {foo=foovalue2} 1",
+			appendFn: func(app storage.Appender) {
+				_, err := app.Append(0, labels.FromStrings("foo", "foovalue1"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				_, err = app.Append(0, labels.FromStrings("foo", "foovalue2"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				_, err = app.Append(0, labels.FromStrings("foo", "foovalue1", "bar", "barvalue1"), timestamp.FromTime(now), 1)
+				testutil.Ok(t, err)
+				testutil.Ok(t, app.Commit())
+			},
+			labelNameCalls: []labelNameCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), expectedNames: []string{"bar", "foo", "region"}},
+				// Query range outside added samples timestamp.
+				// NOTE: Ideally we could do 'end: timestamp.FromTime(now.Add(-1 * time.Second))'. In practice however we index labels within block range, so we approximate label and label values to chunk of block time.
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(now.Add(-4 * time.Hour))},
+				// Matchers on normal series.
+				{
+					start:         timestamp.FromTime(minTime),
+					end:           timestamp.FromTime(maxTime),
+					expectedNames: []string{"bar", "foo", "region"},
+					matchers:      []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "bar", Value: "barvalue1"}},
+				},
+				{
+					start:         timestamp.FromTime(minTime),
+					end:           timestamp.FromTime(maxTime),
+					expectedNames: []string{"foo", "region"},
+					matchers:      []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "foo", Value: "foovalue2"}},
+				},
+				{
+					start:    timestamp.FromTime(minTime),
+					end:      timestamp.FromTime(maxTime),
+					matchers: []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "bar", Value: "different"}},
+				},
+				// Matchers on external labels.
+				{
+					start:         timestamp.FromTime(minTime),
+					end:           timestamp.FromTime(maxTime),
+					expectedNames: []string{"bar", "foo", "region"},
+					matchers:      []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "eu-west"}},
+				},
+				{
+					start:    timestamp.FromTime(minTime),
+					end:      timestamp.FromTime(maxTime),
+					matchers: []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "different"}},
+				},
+			},
+			labelValuesCalls: []labelValuesCallCase{
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "foo", expectedValues: []string{"foovalue1", "foovalue2"}},
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(maxTime), label: "bar", expectedValues: []string{"barvalue1"}},
+				// Query range outside added samples timestamp.
+				// NOTE: Ideally we could do 'end: timestamp.FromTime(now.Add(-1 * time.Second))'. In practice however we index labels within block range, so we approximate label and label values to chunk of block time.
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(now.Add(-4 * time.Hour)), label: "foo"},
+				{start: timestamp.FromTime(minTime), end: timestamp.FromTime(now.Add(-4 * time.Hour)), label: "bar"},
+				// Matchers on normal series.
+				{
+					start:          timestamp.FromTime(minTime),
+					end:            timestamp.FromTime(maxTime),
+					label:          "foo",
+					expectedValues: []string{"foovalue1"},
+					matchers:       []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "bar", Value: "barvalue1"}},
+				},
+				{
+					start:    timestamp.FromTime(minTime),
+					end:      timestamp.FromTime(maxTime),
+					label:    "foo",
+					matchers: []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "bar", Value: "different"}},
+				},
+				// Matchers on external labels.
+				{
+					start:          timestamp.FromTime(minTime),
+					end:            timestamp.FromTime(maxTime),
+					label:          "foo",
+					expectedValues: []string{"foovalue1", "foovalue2"},
+					matchers:       []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "eu-west"}},
+				},
+				{
+					start:          timestamp.FromTime(minTime),
+					end:            timestamp.FromTime(maxTime),
+					label:          "bar",
+					expectedValues: []string{"barvalue1"},
+					matchers:       []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "eu-west"}},
+				},
+				{
+					start:    timestamp.FromTime(minTime),
+					end:      timestamp.FromTime(maxTime),
+					label:    "foo",
+					matchers: []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "different"}},
+				},
+				{
+					start:    timestamp.FromTime(minTime),
+					end:      timestamp.FromTime(maxTime),
+					label:    "bar",
+					matchers: []storepb.LabelMatcher{{Type: storepb.LabelMatcher_EQ, Name: "region", Value: "different"}},
+				},
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			appendFn := tc.appendFn
+			if appendFn == nil {
+				appendFn = func(storage.Appender) {}
+			}
+			store := startStore(extLset, appendFn)
+			for _, c := range tc.labelNameCalls {
+				t.Run("label_names", func(t *testing.T) {
+					resp, err := store.LabelNames(context.Background(), &storepb.LabelNamesRequest{
+						Start:    c.start,
+						End:      c.end,
+						Matchers: c.matchers,
+					})
+					if c.expectErr != nil {
+						testutil.NotOk(t, err)
+						testutil.Equals(t, c.expectErr.Error(), err.Error())
+						return
+					}
+					testutil.Ok(t, err)
+					testutil.Equals(t, 0, len(resp.Warnings))
+					if len(resp.Names) == 0 {
+						resp.Names = nil
+					}
+					testutil.Equals(t, c.expectedNames, resp.Names)
+				})
+			}
+			for _, c := range tc.labelValuesCalls {
+				t.Run("label_name_values", func(t *testing.T) {
+					resp, err := store.LabelValues(context.Background(), &storepb.LabelValuesRequest{
+						Start:    c.start,
+						End:      c.end,
+						Label:    c.label,
+						Matchers: c.matchers,
+					})
+					if c.expectErr != nil {
+						testutil.NotOk(t, err)
+						testutil.Equals(t, c.expectErr.Error(), err.Error())
+						return
+					}
+					testutil.Ok(t, err)
+					testutil.Equals(t, 0, len(resp.Warnings))
+					if len(resp.Values) == 0 {
+						resp.Values = nil
+					}
+					testutil.Equals(t, c.expectedValues, resp.Values)
+				})
+			}
+		})
+	}
+}

pkg/store/prometheus.go

@@ -508,25 +508,24 @@ func (p *PrometheusStore) encodeChunk(ss []prompb.Sample) (storepb.Chunk_Encodin
 
 // LabelNames returns all known label names of series that match the given matchers.
 func (p *PrometheusStore) LabelNames(ctx context.Context, r *storepb.LabelNamesRequest) (*storepb.LabelNamesResponse, error) {
-	lnc := false
-	v := p.promVersion()
-	lbls := []string{}
+	extLset := p.externalLabelsFn()
 
-	version, err := semver.Parse(v)
-	if err == nil && version.GTE(baseVer) {
-		lnc = true
+	match, matchers, err := matchesExternalLabels(r.Matchers, extLset)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+	if !match {
+		return &storepb.LabelNamesResponse{Names: nil}, nil
 	}
 
-	if lnc || len(r.Matchers) == 0 {
-		lbls, err = p.client.LabelNamesInGRPC(ctx, p.base, r.Matchers, r.Start, r.End)
+	var lbls []string
+	version, parseErr := semver.Parse(p.promVersion())
+	if len(matchers) == 0 || (parseErr == nil && version.GTE(baseVer)) {
+		lbls, err = p.client.LabelNamesInGRPC(ctx, p.base, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err
 		}
 	} else {
-		matchers, err := storepb.MatchersToPromMatchers(r.Matchers...)
-		if err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
-		}
 		sers, err := p.client.SeriesInGRPC(ctx, p.base, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err
@@ -545,42 +544,49 @@ func (p *PrometheusStore) LabelNames(ctx context.Context, r *storepb.LabelNamesR
 		}
 	}
 
+	if len(lbls) > 0 {
+		for _, extLbl := range extLset {
+			lbls = append(lbls, extLbl.Name)
+		}
+		sort.Strings(lbls)
+	}
+
 	return &storepb.LabelNamesResponse{Names: lbls}, nil
 }
 
 // LabelValues returns all known label values for a given label name.
 func (p *PrometheusStore) LabelValues(ctx context.Context, r *storepb.LabelValuesRequest) (*storepb.LabelValuesResponse, error) {
-	externalLset := p.externalLabelsFn()
+	if r.Label == "" {
+		return nil, status.Error(codes.InvalidArgument, "label name parameter cannot be empty")
+	}
+
+	extLset := p.externalLabelsFn()
 
 	// First check for matching external label which has priority.
-	if l := externalLset.Get(r.Label); l != "" {
+	if l := extLset.Get(r.Label); l != "" {
 		return &storepb.LabelValuesResponse{Values: []string{l}}, nil
 	}
 
+	match, matchers, err := matchesExternalLabels(r.Matchers, extLset)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+	if !match {
+		return &storepb.LabelValuesResponse{Values: nil}, nil
+	}
+
 	var (
 		sers []map[string]string
-		err  error
+		vals []string
 	)
 
-	lvc := false // LabelValuesCall
-	vals := []string{}
-	v := p.promVersion()
-
-	version, err := semver.Parse(v)
-	if err == nil && version.GTE(baseVer) {
-		lvc = true
-	}
-
-	if len(r.Matchers) == 0 || lvc {
-		vals, err = p.client.LabelValuesInGRPC(ctx, p.base, r.Label, r.Matchers, r.Start, r.End)
+	version, parseErr := semver.Parse(p.promVersion())
+	if len(matchers) == 0 || (parseErr == nil && version.GTE(baseVer)) {
+		vals, err = p.client.LabelValuesInGRPC(ctx, p.base, r.Label, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err
 		}
 	} else {
-		matchers, err := storepb.MatchersToPromMatchers(r.Matchers...)
-		if err != nil {
-			return nil, status.Error(codes.Internal, err.Error())
-		}
 		sers, err = p.client.SeriesInGRPC(ctx, p.base, matchers, r.Start, r.End)
 		if err != nil {
 			return nil, err
@@ -597,6 +603,7 @@ func (p *PrometheusStore) LabelValues(ctx context.Context, r *storepb.LabelValue
 			vals = append(vals, key)
 		}
 	}
+
 	sort.Strings(vals)
 	return &storepb.LabelValuesResponse{Values: vals}, nil
 }