Skip to content

Commit

Permalink
LabelValuesExcluding: Fix corner case
Browse files Browse the repository at this point in the history
Signed-off-by: Arve Knudsen <[email protected]>
  • Loading branch information
aknuds1 committed Jan 25, 2024
1 parent 694524d commit f630927
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 10 deletions.
6 changes: 3 additions & 3 deletions storage/labelvalues_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
func TestListLabelValues(t *testing.T) {
t.Run("lets you traverse a slice of label values", func(t *testing.T) {
input := []string{"a", "b", "c", "d"}
it := NewListLabelValues(input)
it := NewListLabelValues(input, nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand All @@ -25,7 +25,7 @@ func TestListLabelValues(t *testing.T) {
})

t.Run("can be initialized with an empty slice", func(t *testing.T) {
it := NewListLabelValues([]string{})
it := NewListLabelValues([]string{}, nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand All @@ -36,7 +36,7 @@ func TestListLabelValues(t *testing.T) {
})

t.Run("can be initialized with a nil slice", func(t *testing.T) {
it := NewListLabelValues(nil)
it := NewListLabelValues(nil, nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand Down
13 changes: 13 additions & 0 deletions tsdb/block_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,10 @@ func TestLabelValuesStream_WithMatchers(t *testing.T) {
"unique", fmt.Sprintf("value%d", i),
), []chunks.Sample{sample{100, 0, nil, nil}}))
}
// Add another series with an overlapping unique label, but leaving out the tens label
seriesEntries = append(seriesEntries, storage.NewListSeries(labels.FromStrings(
"unique", "value99",
), []chunks.Sample{sample{100, 0, nil, nil}}))

ctx := context.Background()

Expand Down Expand Up @@ -371,6 +375,15 @@ func TestLabelValuesStream_WithMatchers(t *testing.T) {
labels.MustNewMatcher(labels.MatchNotEqual, "tens", ""),
},
expectedValues: uniqueWithout30s,
}, {
// In this case, we query for the "unique" label where the "tens" label is absent.
// We have one series where "tens" is empty (unique="value99"), but also another with
// the same value for "unique" and "tens" present. Make sure that unique="value99" is
// still found.
name: "get unique ID where tens is empty",
labelName: "unique",
matchers: []*labels.Matcher{labels.MustNewMatcher(labels.MatchEqual, "tens", "")},
expectedValues: []string{"value99"},
},
}
for _, tt := range testCases {
Expand Down
14 changes: 14 additions & 0 deletions tsdb/head_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2851,6 +2851,11 @@ func TestHeadLabelValuesStream_WithMatchers(t *testing.T) {
), 100, 0)
require.NoError(t, err)
}
// Add another series with an overlapping unique label, but leaving out the tens label
_, err := app.Append(0, labels.FromStrings(
"unique", "value99",
), 100, 0)
require.NoError(t, err)
require.NoError(t, app.Commit())

indexReader := head.indexRange(0, 200)
Expand Down Expand Up @@ -2904,6 +2909,15 @@ func TestHeadLabelValuesStream_WithMatchers(t *testing.T) {
labelName: "tens",
matchers: []*labels.Matcher{labels.MustNewMatcher(labels.MatchEqual, "unique", "")},
expectedValues: nil,
}, {
// In this case, we query for the "unique" label where the "tens" label is absent.
// We have one series where "tens" is empty (unique="value99"), but also another with
// the same value for "unique" and "tens" present. Make sure that unique="value99" is
// still found.
name: "get unique ID where tens is empty",
labelName: "unique",
matchers: []*labels.Matcher{labels.MustNewMatcher(labels.MatchEqual, "tens", "")},
expectedValues: []string{"value99"},
},
}
for _, tt := range testCases {
Expand Down
38 changes: 35 additions & 3 deletions tsdb/index/labelvalues.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ func (r *Reader) labelValuesForV1(postings Postings, name string, includeMatches
slices.Sort(vals)
return &intersectLabelValuesV1{
postingOffsets: e,
values: storage.NewListLabelValues(vals),
values: storage.NewListLabelValues(vals, nil),
postings: NewPostingsCloner(postings),
b: r.b,
dec: r.dec,
Expand Down Expand Up @@ -494,7 +494,40 @@ func (p *MemPostings) labelValuesFor(postings Postings, name string, includeMatc
}

slices.Sort(vals)
return storage.NewListLabelValues(vals)
return storage.NewListLabelValues(vals, nil)
}

// LabelValuesFromSeries returns all unique label values from r for given label name from supplied series. Values are not sorted.
// buf is space for holding the result (if it isn't big enough, it will be ignored), may be nil.
func LabelValuesFromSeries(lg LabelsGetter, labelName string, refs []storage.SeriesRef, buf []string) ([]string, error) {
values := make(map[string]struct{}, len(buf))

var builder labels.ScratchBuilder
for _, ref := range refs {
err := lg.Labels(ref, &builder)
// Postings may be stale. Skip if no underlying series exists.
if errors.Is(err, storage.ErrNotFound) {
continue
}
if err != nil {
return nil, fmt.Errorf("label values for label %s: %w", labelName, err)
}

v := builder.Labels().Get(labelName)
if v != "" {
values[v] = struct{}{}
}
}

if cap(buf) >= len(values) {
buf = buf[:0]
} else {
buf = make([]string, 0, len(values))
}
for v := range values {
buf = append(buf, v)
}
return buf, nil
}

// intersect returns whether p1 and p2 have at least one series in common.
Expand Down Expand Up @@ -598,4 +631,3 @@ func (p *MemPostings) PostingsForRegexp(ctx context.Context, m *labels.Matcher)

return Merge(ctx, its...)
}

8 changes: 4 additions & 4 deletions tsdb/index/labelvalues_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ func TestMemPostings_LabelValuesFor(t *testing.T) {
t.Run("filtering based on non-empty postings", func(t *testing.T) {
p := mp.Get("a", "1")

it := mp.LabelValuesFor(p, "b")
it := mp.LabelValuesFor(p, "b", nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand All @@ -248,7 +248,7 @@ func TestMemPostings_LabelValuesFor(t *testing.T) {
t.Run("requesting a non-existent label value", func(t *testing.T) {
p := mp.Get("a", "1")

it := mp.LabelValuesFor(p, "c")
it := mp.LabelValuesFor(p, "c", nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand All @@ -259,7 +259,7 @@ func TestMemPostings_LabelValuesFor(t *testing.T) {
})

t.Run("filtering based on empty postings", func(t *testing.T) {
it := mp.LabelValuesFor(EmptyPostings(), "a")
it := mp.LabelValuesFor(EmptyPostings(), "a", nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand All @@ -272,7 +272,7 @@ func TestMemPostings_LabelValuesFor(t *testing.T) {
t.Run("filtering based on a postings set missing the label", func(t *testing.T) {
p := mp.Get("d", "1")

it := mp.LabelValuesFor(p, "a")
it := mp.LabelValuesFor(p, "a", nil)
t.Cleanup(func() {
require.NoError(t, it.Close())
})
Expand Down

0 comments on commit f630927

Please sign in to comment.