fix(blooms): Exclude label filters where label name is part of the series labels. #14661

salvacorts · 2024-10-30T10:47:58Z

What this PR does / why we need it:

Before this PR, running this would return no data:

{env=~".+"} | env="prod"

This is because env is an stream label, not structured metadata. If we not exclude it from the checks, it would not match any bloom filter and therefore all chunks would be discarded erroneously.

This PR fixes this by passing down the series labels to the FilterChunkRefs method of the bloom gateway. The series labels are passed (along the FP) via the logproto.GroupedChunkRefs struct. We pass the series labels to v1.ExtractTestableLabelMatchers which will exclude any matcher with a label name present in the series labels.

Note that we only pass the series to v1.ExtractTestableLabelMatchers deep in the bloom gateway as filtering series is relatively expensive. Upper in the filtering stack we call v1.ExtractTestableLabelMatchers w/o the series (just to return early if there are not filters at all).

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

rfratto

I'm glad we're getting this issue sorted out!

rfratto · 2024-10-30T12:48:28Z

pkg/indexgateway/gateway.go

+	series, err := g.indexQuerier.GetSeries(ctx, instanceID, req.From, req.Through, matchers...)
+	if err != nil {
+		return nil, err
+	}


In the call to GetChunks on 223, the returned chunks matrix has a field called Metric labels.Labels alongside ChunkRef.

I think that might be the labels of the series (though it's named in a way that makes me less sure). But if it is the labels, we can remove this call in favour of the existing GetChunks call.

AFAICT from looking at the code, looks like it does contain the stream labels

rfratto · 2024-10-30T12:49:15Z

pkg/indexgateway/gateway.go

@@ -58,7 +58,7 @@ type IndexClientWithRange struct {
 }

 type BloomQuerier interface {
-	FilterChunkRefs(ctx context.Context, tenant string, from, through model.Time, chunks []*logproto.ChunkRef, plan plan.QueryPlan) ([]*logproto.ChunkRef, error)
+	FilterChunkRefs(ctx context.Context, tenant string, from, through model.Time, series []labels.Labels, chunks []*logproto.ChunkRef, plan plan.QueryPlan) ([]*logproto.ChunkRef, error)


nit: it's a little unfortunate we have to pass series and chunks separately instead of passing through GroupedChunkRefs. I'm not sure it's worth fixing this though, so I don't mind it staying this way.

pkg/logproto/bloomgateway.proto

rfratto · 2024-10-30T12:51:22Z

pkg/storage/bloom/v1/ast_extractor.go

 //
 // Unsupported LabelFilterExprs map to an UnsupportedLabelMatcher, for which
 // bloom tests should always pass.
-func ExtractTestableLabelMatchers(expr syntax.Expr) []LabelMatcher {
+func ExtractTestableLabelMatchers(expr syntax.Expr, series ...labels.Labels) []LabelMatcher {


It's probably ok to have this logic here, but I've never been happy with the cognitive overhead in bloom_tester.go and I suspect it might be easier to follow if we moved the series knowledge checks down to the bloom test itself.

WDYT?

If we do move the logic that far down, we'll need to update other functions to make sure the labels get propagated through properly, like inside partitionRequest.

Done. This way we can also filter out

{app="fake"} | app="noexist" {app="fake"} | app=""

rfratto

Mostly LGTM! I was doing some final checks of other instances where we construct logproto.GroupedChunkRefs to see if labels are being lost anywhere important.

I found these:

I don't know if any of these matter, but all three look easy to fix and so might be worth updating anyway.

I'm also worried about what's happening in BloomQuerier.FilterChunkRefs; are we losing label information after we call bq.BlockResolver.Resolve? Is that potentially a problem?

pkg/storage/bloom/v1/bloom_tester.go

rfratto

LGTM after last two comments! Thank you for fixing this!

pkg/storage/bloom/v1/bloom_tester.go

rfratto · 2024-10-31T12:17:45Z

pkg/storage/bloom/v1/bloom_tester_test.go

+		// We cannot support this test case until we can forward a list of structured metadata fields.
+		// We cannot check if the key is structured metadata using the bloom because these are probabilistic
+		// E.g. bloom.Test("env") may return true even if env is not structured metadata.
+		//{
+		//	name:  "filter label from series overridden by structured metadata",
+		//	query: `{app="fake"} | app="fake"`, // app is set to other in the structured metadata
+		//	match: false,
+		//},


Hm, I'm trying to understand this.

Shouldn't {app="fake"} | app="fake" only return false if every single log line in a chunk has app as structured metadata (and none of them are fake)? I'm not sure knowing the list of fields alone would be enough to ensure we're not filtering out chunks incorrectly.

Yes, you are right. I overthought this one too much.

Pull request #14661 added the series labels to the FilterChunkRefs request in order to be able to filter out full series that do not contain the filter key when using bloom filters. However, this chage incorrectly assumed that `Chunk.Metric` from the `GetChunks()` call contains the labels of the stream. This information is only populated once the chunk is fetched, though. Therefore the list of labels was empty, and no chunk refs were filtered. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…ries labels. (#14661)

Pull request #14661 added the series labels to the FilterChunkRefs request in order to be able to filter out full series that do not contain the filter key when using bloom filters. However, this chage incorrectly assumed that `Chunk.Metric` from the `GetChunks()` call contains the labels of the stream. This information is only populated once the chunk is fetched, though. Therefore the list of labels was empty, and no chunk refs were filtered. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…ered (#14807) Pull request #14661 added the series labels to the `FilterChunkRefs` request in order to be able to filter out full series that do not contain the filter key when using bloom filters. However, this change incorrectly assumed that `Chunk.Metric` from the `GetChunks()` call contains the labels of the stream. This information is only populated once the chunk is fetched, though. Therefore the list of labels was empty, and no chunk refs were filtered. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

…ered (#14807) Pull request #14661 added the series labels to the `FilterChunkRefs` request in order to be able to filter out full series that do not contain the filter key when using bloom filters. However, this change incorrectly assumed that `Chunk.Metric` from the `GetChunks()` call contains the labels of the stream. This information is only populated once the chunk is fetched, though. Therefore the list of labels was empty, and no chunk refs were filtered. Signed-off-by: Christian Haudum <christian.haudum@gmail.com> (cherry picked from commit 3b20cb0)

pull-request-size bot added the size/L label Oct 30, 2024

Filter out label filters that are part of the streams

8c0116d

salvacorts force-pushed the salvacorts/fix-blooms-filter-series-label branch from 6efff01 to 8c0116d Compare October 30, 2024 10:57

salvacorts marked this pull request as ready for review October 30, 2024 11:56

salvacorts requested a review from a team as a code owner October 30, 2024 11:56

rfratto reviewed Oct 30, 2024

View reviewed changes

salvacorts added 2 commits October 30, 2024 17:08

CR feedback

20ab9b3

fix tests

45a8156

rfratto reviewed Oct 30, 2024

View reviewed changes

pkg/storage/bloom/v1/bloom_tester.go Outdated Show resolved Hide resolved

pkg/storage/bloom/v1/bloom_tester.go Outdated Show resolved Hide resolved

salvacorts added 3 commits October 31, 2024 09:21

fix tests

8e04c4c

Fix bloom checker

f0209fd

Add todo

23c1cc2

rfratto approved these changes Oct 31, 2024

View reviewed changes

salvacorts added 2 commits October 31, 2024 13:49

Add log line and remove comment

2369415

lint

0548062

salvacorts mentioned this pull request Oct 31, 2024

feat(blooms): Only write key and key=value to blooms #14686

Merged

6 tasks

salvacorts merged commit d1668f6 into main Oct 31, 2024
59 checks passed

salvacorts deleted the salvacorts/fix-blooms-filter-series-label branch October 31, 2024 14:51

chaudum mentioned this pull request Nov 4, 2024

Bloom filters based on structured metadata search chunks even if key does not exists #14614

Closed

chaudum pushed a commit that referenced this pull request Nov 6, 2024

fix(blooms): Exclude label filters where label name is part of the se…

1de99ee

…ries labels. (#14661)

chaudum mentioned this pull request Nov 7, 2024

chore: Fix regression in bloom gateway that caused nothing to be filtered #14807

Merged

6 tasks

loki-gh-app bot mentioned this pull request Nov 7, 2024

chore: Fix regression in bloom gateway that caused nothing to be filtered (backport k227) #14816

Merged

6 tasks

loki-gh-app bot mentioned this pull request Nov 14, 2024

chore(k227): release 3.3.0 #14750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(blooms): Exclude label filters where label name is part of the series labels. #14661

fix(blooms): Exclude label filters where label name is part of the series labels. #14661

salvacorts commented Oct 30, 2024 •

edited

Loading

rfratto left a comment

rfratto Oct 30, 2024

salvacorts Oct 30, 2024

rfratto Oct 30, 2024

rfratto Oct 30, 2024

salvacorts Oct 30, 2024

rfratto left a comment

rfratto left a comment

rfratto Oct 31, 2024

salvacorts Oct 31, 2024

fix(blooms): Exclude label filters where label name is part of the series labels. #14661

fix(blooms): Exclude label filters where label name is part of the series labels. #14661

Conversation

salvacorts commented Oct 30, 2024 • edited Loading

rfratto left a comment

Choose a reason for hiding this comment

rfratto Oct 30, 2024

Choose a reason for hiding this comment

salvacorts Oct 30, 2024

Choose a reason for hiding this comment

rfratto Oct 30, 2024

Choose a reason for hiding this comment

rfratto Oct 30, 2024

Choose a reason for hiding this comment

salvacorts Oct 30, 2024

Choose a reason for hiding this comment

rfratto left a comment

Choose a reason for hiding this comment

rfratto left a comment

Choose a reason for hiding this comment

rfratto Oct 31, 2024

Choose a reason for hiding this comment

salvacorts Oct 31, 2024

Choose a reason for hiding this comment

salvacorts commented Oct 30, 2024 •

edited

Loading