Bloom-Gateway cache #11380

salvacorts · 2023-12-04T15:11:41Z

What this PR does / why we need it:

This PR adds caching to the bloom-gateway client. It uses the result cache from #11343.

Here's how we:

Merge responses: group all chunks by FP and remove duplicated chunk checksums.
Extract responses based on time span: For all chunks in each FP, add to the extracted response only the chunks that overlaps with the desired start and end time.

github-actions · 2023-12-04T15:13:43Z

Trivy scan found the following vulnerabilities:

HIGH openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
HIGH openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0

pkg/bloomgateway/cache.go

pkg/logproto/compat.go

chaudum · 2023-12-05T07:38:30Z

pkg/logproto/compat.go

+
+// GetQuery returns the query of the request.
+// The query is the hash for the input chunks refs and the filter expressions.
+func (m *FilterChunkRefRequest) GetQuery() string {


This function is really expensive. Is it used as part of the cache key?

Yes, it's used to compute the key. I agree it's probably too expensive.

pkg/bloomgateway/cache.go

chaudum · 2023-12-05T08:19:21Z

pkg/bloomgateway/cache.go

+	sort.Slice(chunkRefs, func(i, j int) bool {
+		return chunkRefs[i].Fingerprint < chunkRefs[j].Fingerprint
+	})
+	return slices.CompactFunc(chunkRefs, func(next, prev *logproto.GroupedChunkRefs) bool {


TIL: There is a slices.CompactFunc :)

hm ;) great function ;) previously I would do it manually ;))))

it looks like it will join the chunks only for the first neighbor elements with the same fingerprint. if we have more than 2 neighbors with the same fingerprint, we will just lose chunk ids...
I am not sure if it's expected right now to have more than 2 elements with the same fingerprint but I would not leave this function because in the future somebody will spend days trying to find this bug.

look at this code snippet, https://go.dev/play/p/c_bt6c1HNUj?v= ,
I created a snippet with users, the first 6 users have ID a , and the next 4 users have ID b. if we use CompactFunc to aggregate all the scores of the users, we won't have the expected results...

compacted 2 &{id:a scores:1} &{id:b scores:13}

but it has to be:

compacted 2 &{id:a scores:15} &{id:b scores:30}

~~Yes! It's quite useful to remove / aggregate duplicated items in a sorted list~~ As Vlad mentioned, this is buggy.

You are right @vlad-diachenko good catch. I pushed a new commit with a test for this and a fix.

docs/sources/configure/_index.md

pkg/bloomgateway/cache.go

vlad-diachenko

reviewed partially and left some comments, I believe Chirstian has more context here, so completely rely on his review

vlad-diachenko · 2023-12-05T08:35:50Z

pkg/bloomgateway/cache.go

+		for _, ref := range chunkRef.Refs {
+			if model.Time(end) < ref.From || ref.Through <= model.Time(start) {
+				continue
+			}
+			refs = append(refs, ref)
+		}


but sorting also requires the same count of iterations to ensure each item is in order. and after this, we will do a binary search. so, I believe it's better to leave it as is. wdyt?

vlad-diachenko · 2023-12-05T08:40:28Z

pkg/bloomgateway/cache.go

+	sort.Slice(chunkRefs, func(i, j int) bool {
+		return chunkRefs[i].Fingerprint < chunkRefs[j].Fingerprint
+	})
+	return slices.CompactFunc(chunkRefs, func(next, prev *logproto.GroupedChunkRefs) bool {


hm ;) great function ;) previously I would do it manually ;))))

vlad-diachenko · 2023-12-05T09:20:15Z

pkg/bloomgateway/cache.go

+	sort.Slice(chunkRefs, func(i, j int) bool {
+		return chunkRefs[i].Fingerprint < chunkRefs[j].Fingerprint
+	})
+	return slices.CompactFunc(chunkRefs, func(next, prev *logproto.GroupedChunkRefs) bool {


it looks like it will join the chunks only for the first neighbor elements with the same fingerprint. if we have more than 2 neighbors with the same fingerprint, we will just lose chunk ids...
I am not sure if it's expected right now to have more than 2 elements with the same fingerprint but I would not leave this function because in the future somebody will spend days trying to find this bug.

look at this code snippet, https://go.dev/play/p/c_bt6c1HNUj?v= ,
I created a snippet with users, the first 6 users have ID a , and the next 4 users have ID b. if we use CompactFunc to aggregate all the scores of the users, we won't have the expected results...

compacted 2 &{id:a scores:1} &{id:b scores:13}

but it has to be:

compacted 2 &{id:a scores:15} &{id:b scores:30}

pkg/bloomgateway/client.go

chaudum · 2023-12-05T13:27:51Z

pkg/bloomgateway/client.go

@@ -109,9 +116,25 @@ func (i *ClientConfig) RegisterFlags(f *flag.FlagSet) {
 // RegisterFlagsWithPrefix registers flags for the Bloom Gateway client configuration with a common prefix.
 func (i *ClientConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	i.GRPCClientConfig.RegisterFlagsWithPrefix(prefix+"grpc", f)
+	i.Cache.RegisterFlagsWithPrefix(f, prefix+"cache.")


This is a bit of a trap:

You call RegisterFlagsWithPrefix on the inlined object resultscache.Config. Therefore not all flags are registered.

Fixed by implementing RegisterFlagsWithPrefix in CacheConfig

chaudum · 2023-12-05T13:37:52Z

pkg/logproto/compat.go

+		h := xxhash.New()
+		for _, ref := range m.Refs {
+			encodeBuf = encodeBuf[:0]
+			_, _ = h.Write(fmt.Appendf(encodeBuf, "%d", ref.Fingerprint))


Alternative that does not require string->[]byte conversions:

Suggested change

_, _ = h.Write(fmt.Appendf(encodeBuf, "%d", ref.Fingerprint))

_, _ = h.Write(binary.LittleEndian.AppendUint64(encodeBuf[:0], ref.Fingerprint))

Nice!, leaving fmt.Appendf for the operator since we need to be able to print the operator id.

vlad-diachenko

lgtm

**What this PR does / why we need it**: This PR adds caching to the bloom-gateway client. It uses the result cache from grafana#11343. Here's how we: - Merge responses: group all chunks by FP and remove duplicated chunk checksums. - Extract responses based on time span: For all chunks in each FP, add to the extracted response only the chunks that overlaps with the desired start and end time.

pull-request-size bot added the size/XL label Dec 4, 2023

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 4, 2023

Bloom-Gateway cache

957ce11

salvacorts force-pushed the salvacorts/bloom-gateway-cache branch from 035e329 to 957ce11 Compare December 4, 2023 15:25

chaudum reviewed Dec 5, 2023

View reviewed changes

salvacorts added 4 commits December 5, 2023 10:44

CR feedback (1)

b3780b3

CR Feedback (2): Forward GRPC call opts

4aab4b1

CR Feedback (3): WithStartEndForCache remove chunks outside range

a3d8bf6

Update docs

859de06

vlad-diachenko reviewed Dec 5, 2023

View reviewed changes

salvacorts added 3 commits December 5, 2023 11:36

Validate config

cdad32d

Fix bug with compact

58d1bfa

Do not format json

6e05bf6

chaudum reviewed Dec 5, 2023

View reviewed changes

salvacorts added 2 commits December 5, 2023 14:30

Reuse buffer for str fmt

80ea4d8

Fix register with flags

50fb9b0

chaudum reviewed Dec 5, 2023

View reviewed changes

CR

ded6905

salvacorts marked this pull request as ready for review December 5, 2023 13:51

salvacorts requested a review from a team as a code owner December 5, 2023 13:51

chaudum approved these changes Dec 5, 2023

View reviewed changes

paul1r approved these changes Dec 5, 2023

View reviewed changes

vlad-diachenko approved these changes Dec 5, 2023

View reviewed changes

salvacorts merged commit a0b462d into main Dec 5, 2023
6 checks passed

salvacorts deleted the salvacorts/bloom-gateway-cache branch December 5, 2023 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bloom-Gateway cache #11380

Bloom-Gateway cache #11380

salvacorts commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023

chaudum Dec 5, 2023

salvacorts Dec 5, 2023

chaudum Dec 5, 2023

vlad-diachenko Dec 5, 2023

vlad-diachenko Dec 5, 2023

salvacorts Dec 5, 2023 •

edited

Loading

salvacorts Dec 5, 2023

vlad-diachenko left a comment

vlad-diachenko Dec 5, 2023

vlad-diachenko Dec 5, 2023

vlad-diachenko Dec 5, 2023

chaudum Dec 5, 2023

salvacorts Dec 5, 2023

chaudum Dec 5, 2023 •

edited

Loading

salvacorts Dec 5, 2023

vlad-diachenko left a comment

	_, _ = h.Write(fmt.Appendf(encodeBuf, "%d", ref.Fingerprint))
	_, _ = h.Write(binary.LittleEndian.AppendUint64(encodeBuf[:0], ref.Fingerprint))

Bloom-Gateway cache #11380

Bloom-Gateway cache #11380

Conversation

salvacorts commented Dec 4, 2023 • edited Loading

github-actions bot commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvacorts Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaudum Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

salvacorts commented Dec 4, 2023 •

edited

Loading

salvacorts Dec 5, 2023 •

edited

Loading

chaudum Dec 5, 2023 •

edited

Loading