Skip to content

Commit 8785e74

Browse files
grafanabotchaudum
grafanabot
and
chaudum
committed
chore: [k200] fix(blooms): Fully deduplicate chunks from FilterChunkRef responses (#12835)
Backport a0f358f from #12807 Co-authored-by: Christian Haudum <[email protected]>
1 parent 22a8825 commit 8785e74

File tree

9 files changed

+263
-82
lines changed

9 files changed

+263
-82
lines changed

‎pkg/bloomgateway/bloomgateway.go

+8-1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ package bloomgateway
4444
import (
4545
"context"
4646
"fmt"
47+
"slices"
4748
"sort"
4849
"time"
4950

@@ -383,6 +384,10 @@ func (g *Gateway) consumeTask(ctx context.Context, task Task, tasksCh chan<- Tas
383384
case <-ctx.Done():
384385
// do nothing
385386
default:
387+
// chunks may not always be sorted
388+
if !slices.IsSortedFunc(res.Removals, func(a, b v1.ChunkRef) int { return a.Cmp(b) }) {
389+
slices.SortFunc(res.Removals, func(a, b v1.ChunkRef) int { return a.Cmp(b) })
390+
}
386391
task.responses = append(task.responses, res)
387392
}
388393
}
@@ -413,13 +418,14 @@ func orderedResponsesByFP(responses [][]v1.Output) v1.Iterator[v1.Output] {
413418
itrs = append(itrs, v1.NewPeekingIter(v1.NewSliceIter(r)))
414419
}
415420
return v1.NewHeapIterator[v1.Output](
416-
func(o1, o2 v1.Output) bool { return o1.Fp <= o2.Fp },
421+
func(o1, o2 v1.Output) bool { return o1.Fp < o2.Fp },
417422
itrs...,
418423
)
419424
}
420425

421426
// TODO(owen-d): improve perf. This can be faster with a more specialized impl
422427
// NB(owen-d): `req` is mutated in place for performance, but `responses` is not
428+
// Removals of the outputs must be sorted.
423429
func filterChunkRefs(req *logproto.FilterChunkRefRequest, responses [][]v1.Output) []*logproto.GroupedChunkRefs {
424430
res := make([]*logproto.GroupedChunkRefs, 0, len(req.Refs))
425431

@@ -433,6 +439,7 @@ func filterChunkRefs(req *logproto.FilterChunkRefRequest, responses [][]v1.Outpu
433439
// from
434440
v1.Identity[v1.Output],
435441
// merge two removal sets for the same series, deduping
442+
// requires that the removals of the outputs are sorted
436443
func(o1, o2 v1.Output) v1.Output {
437444
res := v1.Output{Fp: o1.Fp}
438445

‎pkg/bloomgateway/bloomgateway_test.go

+23-2
Original file line numberDiff line numberDiff line change
@@ -640,12 +640,12 @@ func TestFilterChunkRefs(t *testing.T) {
640640
{
641641
{fp: 0, checksums: []uint32{0, 1}},
642642
{fp: 0, checksums: []uint32{0, 1, 2}},
643-
{fp: 1, checksums: []uint32{1}},
643+
{fp: 1, checksums: []uint32{0, 2}},
644644
{fp: 2, checksums: []uint32{1}},
645645
},
646646
},
647647
expected: mkResult([]instruction{
648-
{fp: 1, checksums: []uint32{0, 2}},
648+
{fp: 1, checksums: []uint32{1}},
649649
{fp: 2, checksums: []uint32{0, 2}},
650650
{fp: 3, checksums: []uint32{0, 1, 2}},
651651
}),
@@ -670,6 +670,27 @@ func TestFilterChunkRefs(t *testing.T) {
670670
{fp: 3, checksums: []uint32{0, 1, 2}},
671671
}),
672672
},
673+
{
674+
desc: "unordered fingerprints",
675+
input: mkInput(4, 3),
676+
removals: [][]instruction{
677+
{
678+
{fp: 3, checksums: []uint32{2}},
679+
{fp: 0, checksums: []uint32{1, 2}},
680+
{fp: 2, checksums: []uint32{1, 2}},
681+
},
682+
{
683+
{fp: 1, checksums: []uint32{1}},
684+
{fp: 2, checksums: []uint32{0, 1}},
685+
{fp: 3, checksums: []uint32{0}},
686+
},
687+
},
688+
expected: mkResult([]instruction{
689+
{fp: 0, checksums: []uint32{0}},
690+
{fp: 1, checksums: []uint32{0, 2}},
691+
{fp: 3, checksums: []uint32{1}},
692+
}),
693+
},
673694
} {
674695
t.Run(tc.desc, func(t *testing.T) {
675696
res := filterChunkRefs(tc.input, mkRemovals(tc.removals))

‎pkg/bloomgateway/cache_test.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,10 @@ func TestMerge(t *testing.T) {
344344
m := newMerger()
345345
actual, err := m.MergeResponse(input...)
346346
require.NoError(t, err)
347-
require.Equal(t, tc.expected, actual)
347+
348+
resp, ok := actual.(*logproto.FilterChunkRefResponse)
349+
require.True(t, ok)
350+
require.Equal(t, tc.expected, resp)
348351
})
349352
}
350353
}

‎pkg/bloomgateway/client.go

+36-44
Original file line numberDiff line numberDiff line change
@@ -294,13 +294,12 @@ func mergeSeries(input [][]*logproto.GroupedChunkRefs, buf []*logproto.GroupedCh
294294

295295
iters := make([]v1.PeekingIterator[*logproto.GroupedChunkRefs], 0, len(input))
296296
for _, inp := range input {
297+
sort.Slice(inp, func(i, j int) bool { return inp[i].Fingerprint < inp[j].Fingerprint })
297298
iters = append(iters, v1.NewPeekingIter(v1.NewSliceIter(inp)))
298299
}
299300

300301
heapIter := v1.NewHeapIterator[*logproto.GroupedChunkRefs](
301-
func(a, b *logproto.GroupedChunkRefs) bool {
302-
return a.Fingerprint < b.Fingerprint
303-
},
302+
func(a, b *logproto.GroupedChunkRefs) bool { return a.Fingerprint < b.Fingerprint },
304303
iters...,
305304
)
306305

@@ -311,10 +310,17 @@ func mergeSeries(input [][]*logproto.GroupedChunkRefs, buf []*logproto.GroupedCh
311310
v1.Identity[*logproto.GroupedChunkRefs],
312311
// merge
313312
func(a, b *logproto.GroupedChunkRefs) *logproto.GroupedChunkRefs {
313+
// TODO(chaudum): Check if we can assume sorted shortrefs here
314+
if !slices.IsSortedFunc(a.Refs, func(a, b *logproto.ShortRef) int { return a.Cmp(b) }) {
315+
slices.SortFunc(a.Refs, func(a, b *logproto.ShortRef) int { return a.Cmp(b) })
316+
}
317+
if !slices.IsSortedFunc(b.Refs, func(a, b *logproto.ShortRef) int { return a.Cmp(b) }) {
318+
slices.SortFunc(b.Refs, func(a, b *logproto.ShortRef) int { return a.Cmp(b) })
319+
}
314320
return &logproto.GroupedChunkRefs{
315321
Fingerprint: a.Fingerprint,
316322
Tenant: a.Tenant,
317-
Refs: mergeChunks(a.Refs, b.Refs),
323+
Refs: mergeChunkSets(a.Refs, b.Refs),
318324
}
319325
},
320326
// iterator
@@ -324,51 +330,37 @@ func mergeSeries(input [][]*logproto.GroupedChunkRefs, buf []*logproto.GroupedCh
324330
return v1.CollectInto(dedupeIter, buf)
325331
}
326332

327-
func mergeChunks(inputs ...[]*logproto.ShortRef) []*logproto.ShortRef {
328-
if len(inputs) == 0 {
329-
return nil
330-
}
333+
// mergeChunkSets merges and deduplicates two sorted slices of shortRefs
334+
func mergeChunkSets(s1, s2 []*logproto.ShortRef) (result []*logproto.ShortRef) {
335+
var i, j int
336+
for i < len(s1) && j < len(s2) {
337+
a, b := s1[i], s2[j]
338+
339+
if a.Equal(b) {
340+
result = append(result, a)
341+
i++
342+
j++
343+
continue
344+
}
345+
346+
if a.Less(b) {
347+
result = append(result, a)
348+
i++
349+
continue
350+
}
331351

332-
if len(inputs) == 1 {
333-
slices.SortFunc(
334-
inputs[0],
335-
func(a, b *logproto.ShortRef) int {
336-
if a.Equal(b) {
337-
return 0
338-
}
339-
if a.From.Before(b.From) || (a.From.Equal(b.From) && a.Through.Before(b.Through)) {
340-
return -1
341-
}
342-
return 1
343-
},
344-
)
345-
return inputs[0]
352+
result = append(result, b)
353+
j++
346354
}
347355

348-
iters := make([]v1.PeekingIterator[*logproto.ShortRef], 0, len(inputs))
349-
for _, inp := range inputs {
350-
iters = append(iters, v1.NewPeekingIter(v1.NewSliceIter(inp)))
356+
if i < len(s1) {
357+
result = append(result, s1[i:]...)
358+
}
359+
if j < len(s2) {
360+
result = append(result, s2[j:]...)
351361
}
352362

353-
chunkDedupe := v1.NewDedupingIter[*logproto.ShortRef, *logproto.ShortRef](
354-
// eq
355-
func(a, b *logproto.ShortRef) bool { return a.Equal(b) },
356-
// from
357-
v1.Identity[*logproto.ShortRef],
358-
// merge
359-
func(a, b *logproto.ShortRef) *logproto.ShortRef { return a },
360-
// iterator
361-
v1.NewPeekingIter[*logproto.ShortRef](
362-
v1.NewHeapIterator[*logproto.ShortRef](
363-
func(a, b *logproto.ShortRef) bool {
364-
return a.From.Before(b.From) || (a.From.Equal(b.From) && a.Through.Before(b.Through))
365-
},
366-
iters...,
367-
),
368-
),
369-
)
370-
merged, _ := v1.Collect(chunkDedupe)
371-
return merged
363+
return result
372364
}
373365

374366
// doForAddrs sequetially calls the provided callback function fn for each

‎pkg/bloomgateway/client_test.go

+24
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,27 @@ func TestGatewayClient_MergeSeries(t *testing.T) {
7070
result, _ := mergeSeries(inputs, nil)
7171
require.Equal(t, expected, result)
7272
}
73+
74+
func TestGatewayClient_MergeChunkSets(t *testing.T) {
75+
inp1 := []*logproto.ShortRef{
76+
shortRef(1, 3, 1),
77+
shortRef(2, 3, 2),
78+
shortRef(4, 5, 3),
79+
}
80+
inp2 := []*logproto.ShortRef{
81+
shortRef(2, 3, 2),
82+
shortRef(3, 4, 4),
83+
shortRef(5, 6, 5),
84+
}
85+
86+
expected := []*logproto.ShortRef{
87+
shortRef(1, 3, 1),
88+
shortRef(2, 3, 2),
89+
shortRef(3, 4, 4),
90+
shortRef(4, 5, 3),
91+
shortRef(5, 6, 5),
92+
}
93+
94+
result := mergeChunkSets(inp1, inp2)
95+
require.Equal(t, expected, result)
96+
}

‎pkg/bloomgateway/querier.go

+31-33
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"github.com/go-kit/log"
77
"github.com/go-kit/log/level"
88
"github.com/opentracing/opentracing-go"
9+
"github.com/pkg/errors"
910
"github.com/prometheus/client_golang/prometheus"
1011
"github.com/prometheus/client_golang/prometheus/promauto"
1112
"github.com/prometheus/common/model"
@@ -99,9 +100,7 @@ func (bq *BloomQuerier) FilterChunkRefs(ctx context.Context, tenant string, from
99100
preFilterChunks := len(chunkRefs)
100101
preFilterSeries := len(grouped)
101102

102-
result := make([]*logproto.ChunkRef, 0, len(chunkRefs))
103-
seriesSeen := make(map[uint64]struct{}, len(grouped))
104-
103+
responses := make([][]*logproto.GroupedChunkRefs, 0, 2)
105104
// We can perform requests sequentially, because most of the time the request
106105
// only covers a single day, and if not, it's at most two days.
107106
for _, s := range partitionSeriesByDay(from, through, grouped) {
@@ -110,53 +109,52 @@ func (bq *BloomQuerier) FilterChunkRefs(ctx context.Context, tenant string, from
110109
if err != nil {
111110
return nil, err
112111
}
113-
var chunks int
114-
for i := range s.series {
115-
chunks += len(s.series[i].Refs)
116-
}
117-
sp.LogKV(
118-
"day", s.day.Time.Time(),
119-
"from", s.interval.Start.Time(),
120-
"through", s.interval.End.Time(),
121-
"series", len(s.series),
122-
"chunks", chunks,
123-
"blocks", len(blocks),
124-
"skipped", len(skipped),
125-
)
126112

127113
refs, err := bq.c.FilterChunks(ctx, tenant, s.interval, blocks, queryPlan)
128114
if err != nil {
129115
return nil, err
130116
}
131117

132118
// add chunk refs from series that were not mapped to any blocks
133-
refs = append(refs, skipped...)
119+
responses = append(responses, refs, skipped)
134120
bq.metrics.seriesSkipped.Add(float64(len(skipped)))
121+
}
122+
123+
deduped, err := mergeSeries(responses, nil)
124+
if err != nil {
125+
return nil, errors.Wrap(err, "failed to dedupe results")
126+
}
135127

136-
for i := range refs {
137-
seriesSeen[refs[i].Fingerprint] = struct{}{}
138-
for _, ref := range refs[i].Refs {
139-
result = append(result, &logproto.ChunkRef{
140-
Fingerprint: refs[i].Fingerprint,
141-
UserID: tenant,
142-
From: ref.From,
143-
Through: ref.Through,
144-
Checksum: ref.Checksum,
145-
})
146-
}
128+
result := make([]*logproto.ChunkRef, 0, len(chunkRefs))
129+
for i := range deduped {
130+
for _, ref := range deduped[i].Refs {
131+
result = append(result, &logproto.ChunkRef{
132+
Fingerprint: deduped[i].Fingerprint,
133+
UserID: tenant,
134+
From: ref.From,
135+
Through: ref.Through,
136+
Checksum: ref.Checksum,
137+
})
147138
}
148139
}
149140

141+
postFilterChunks := len(result)
142+
postFilterSeries := len(deduped)
143+
150144
level.Debug(bq.logger).Log(
145+
"operation", "bloomquerier.FilterChunkRefs",
146+
"tenant", tenant,
147+
"from", from.Time(),
148+
"through", through.Time(),
149+
"responses", len(responses),
151150
"preFilterChunks", preFilterChunks,
152-
"postFilterChunks", len(result),
151+
"postFilterChunks", postFilterChunks,
152+
"filteredChunks", preFilterChunks-postFilterChunks,
153153
"preFilterSeries", preFilterSeries,
154-
"postFilterSeries", len(seriesSeen),
154+
"postFilterSeries", postFilterSeries,
155+
"filteredSeries", preFilterSeries-postFilterSeries,
155156
)
156157

157-
postFilterChunks := len(result)
158-
postFilterSeries := len(seriesSeen)
159-
160158
bq.metrics.chunksTotal.Add(float64(preFilterChunks))
161159
bq.metrics.chunksFiltered.Add(float64(preFilterChunks - postFilterChunks))
162160
bq.metrics.seriesTotal.Add(float64(preFilterSeries))

0 commit comments

Comments
 (0)