Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(blooms): compute chunks once #12664

Merged
merged 27 commits into from
Apr 29, 2024
Merged

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Apr 17, 2024

Computes chunks once during planning and passes them to the queriers, reducing a lot of calls to the index.

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
…ks-once

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
…ks-once

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d requested a review from a team as a code owner April 17, 2024 19:51
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
…ks-once

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d force-pushed the blooms/compute-chunks-once branch from c88b9f4 to a764c11 Compare April 18, 2024 23:20
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
…ns + new override

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
…ks-once

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Apr 23, 2024
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d force-pushed the blooms/compute-chunks-once branch from 53d4a46 to a3bd99c Compare April 24, 2024 16:23
@@ -176,6 +203,16 @@ func (s Shard) Ptr() *Shard {
return &s
}

func (s Shard) Bind(chunks *logproto.ChunkRefGroup) *ShardWithChunkRefs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function replaces Prt(), right? Can Ptr() be removed then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be, but is still used in a bunch of tests I'm not inclined to update here

Comment on lines 207 to 213
res := &ShardWithChunkRefs{
Shard: s,
}
if chunks != nil {
res.chunks = chunks
}
return res
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be the same as

Suggested change
res := &ShardWithChunkRefs{
Shard: s,
}
if chunks != nil {
res.chunks = chunks
}
return res
return &ShardWithChunkRefs{
Shard: s,
chunks: chunks,
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call 👍

@@ -355,48 +352,11 @@ func (s *GatewayClient) GetShards(
return errCt <= maxErrs
},
); err != nil {
if isUnimplementedCallError(err) {
Copy link
Contributor

@salvacorts salvacorts Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC we are removing this since we already rolled out index-gws that support the get shards method, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No (the OSS repo is independent from our internal deployments at Grafana Labs). This is because the new bounded shards implementation is an intentional choice that needs to be turned on (default=power_of_two), so I removed the fallback for simplicity's sake.

filtered := refs

// 2) filter via blooms if enabled
if g.bloomQuerier != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check here if the AST has any filtering expression:

if g.bloomQuerier != nil && syntax.ExtractLineFilters(p.Plan().AST)) != 0 {

res := &ShardWithChunkRefs{
Shard: s,
}
if chunks != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe it's not necessary but since elsewhere we are checking res.Chunks != nil to check if we are passing chunks in the shards, I think we should only set this if len(chunks.refs) > 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that there's a distinction -- nil indicates there is no override, but a zero-length chunk override is still valid and can happen when we lookup labels which don't map to chunks.

// in case of multiple tenants, we need to filter the store chunks by tenant if they are provided
storeOverridesByTenant := make(map[string][]*logproto.ChunkRef)
if overrides := params.GetStoreChunks(); overrides != nil {
storeOverridesByTenant = partitionChunkRefsByTenant(params.GetStoreChunks().Refs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

storeOverridesByTenant = partitionChunkRefsByTenant(overrides.Refs)

func partitionChunkRefsByTenant(refs []*logproto.ChunkRef) map[string][]*logproto.ChunkRef {
filtered := make(map[string][]*logproto.ChunkRef)
for _, ref := range refs {
filtered[ref.UserID] = append(filtered[ref.UserID], ref)
Copy link
Contributor

@salvacorts salvacorts Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we preallocate the user refs slice with some capacity?

Copy link
Member Author

@owen-d owen-d Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably skip this until I see it being a problem (or happy to see it in a followup PR)

@@ -2125,7 +2086,81 @@ func mergeLokiResponse(responses ...queryrangebase.Response) *LokiResponse {
}
}

// In some other world LabelRequest could implement queryrangebase.Request.
func parseRangeQueryWithStoreChunksExtension(r *http.Request) (*LokiRequest, error) {
Copy link
Contributor

@salvacorts salvacorts Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this method being called conditionally of having chunks. I think this as well as the one for instant queries should be renamed to parseRangeQuery and parseInstantQuery

@@ -265,37 +269,43 @@ func (r *dynamicShardResolver) ShardingRanges(expr syntax.Expr, targetBytesPerSh
if resp, ok := httpgrpc.HTTPResponseFromError(err); ok && (resp.Code == http.StatusNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this fallback for ingesters, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably remove this code for consistency with removing the same in gatewayclient

from,
through model.Time,
predicate chunk.Predicate,
storeChunksOverride *logproto.ChunkRefGroup,
Copy link
Contributor

@salvacorts salvacorts Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I understand GetChunks does some extra filtering to the storeChunksOverride (validates and filters by the time range and fetcher). Still I think we should avoid calling GetChunk refs at all instead of passing the storeChunksOverride around. What about having a GetFetcherForChunks that returns grouped chunks by fetchers.

@@ -411,7 +417,7 @@ func (s *LokiStore) lazyChunks(ctx context.Context, from, through model.Time, pr
stats := stats.FromContext(ctx)

start := time.Now()
chks, fetchers, err := s.GetChunks(ctx, userID, from, through, predicate)
chks, fetchers, err := s.GetChunks(ctx, userID, from, through, predicate, storeChunksOverride)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Here's where I think we should do something like:

var chunks [][]chunkRefs
var fetchers []fetchers
if storeChunksOverride > 0 {
   chunks, fetchers := s.GetFetcherForChunks(from, through, storeChunksOverride)
} else {
   chunks, fetchers := s.GetChunks(ctx, userID, from, through, predicate)
}

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d merged commit bc78d13 into grafana:main Apr 29, 2024
60 checks passed
shantanualsi pushed a commit that referenced this pull request May 6, 2024
Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants