chore(blooms)!: Introduce a new block schema (V3) #14038

chaudum · 2024-09-04T11:35:05Z

What this PR does / why we need it:

A new schema version V3 for bloom blocks that is incompatible with old schemas.
It simplifies some parts of the code and makes the schema more extensible, laying the groundwork for indexing structured metadata fields into bloom filters.

⚠️ This PR breaks the bloom filter functionality! Until the read path is implemented, bloom blocks created with the new schema cannot be queries in the bloom gateways.

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
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
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

Nice, I was able to follow along here fairly well. I didn't spot any major issues, so my comments are mainly questions rather than anything blocking.

rfratto · 2024-09-04T18:03:46Z

pkg/bloomgateway/processor.go

@@ -145,6 +145,11 @@ func (p *processor) processBlock(_ context.Context, bq *bloomshipper.CloseableBl
 		return err
 	}

+	// We require V3 schema
+	if !schema.IsCurrentSchema() {
+		return v1.ErrUnsupportedSchemaVersion


I noticed that processor.processTasks aborts after the first task group where p.processTasksForDay returns an error. Is that something we want to keep?

Especially with this most recent change, I believe that means if any of the existing blocks aren't the current schema version, then we don't process any of newer ones, even if they're valid.

Yeah, good point. We could ignore incompatible blocks, rather than fail.
But for the sake of simplicity, I would cancel a request with the first incompatible block.

Sounds good to me. If this turns out to be an issue in the future we can replace it with multierror instead of returning immediately.

pkg/storage/bloom/v1/builder_test.go

pkg/bloomgateway/processor.go

pkg/storage/bloom/v1/index.go

pkg/storage/bloom/v1/util.go

rfratto

Looks good from my side, Salva might see something I don't

salvacorts

LGTM 👏

salvacorts · 2024-09-05T14:18:45Z

pkg/storage/bloom/v1/bloom_builder.go

@@ -46,6 +46,8 @@ func (b *BloomBlockBuilder) Append(bloom *Bloom) (BloomOffset, error) {
 		}
 	}

+	// version := b.opts.Schema.version


Can we remove this?

salvacorts · 2024-09-05T14:19:10Z

pkg/storage/bloom/v1/builder.go

@@ -66,9 +66,21 @@ func (b BlockOptions) Encode(enc *encoding.Encbuf) {
 	enc.PutBE64(b.BlockSize)
 }

+// func NewDefaultBlockOptions(maxBlockSizeBytes, maxBloomSizeBytes uint64) BlockOptions {


salvacorts · 2024-09-05T14:26:07Z

pkg/storage/bloom/v1/schema.go

+	// V2 supports single series blooms encoded over multiple pages
+	// to accommodate larger single series
+	V2
+	// V2 indicated schema for indexed structured metadata


Suggested change

// V2 indicated schema for indexed structured metadata

// V3 indicated schema for indexed structured metadata

salvacorts · 2024-09-05T14:27:11Z

pkg/storage/bloom/v1/schema.go

+type Schema struct {
+	version                Version
+	encoding               chunkenc.Encoding
+	nGramLength, nGramSkip uint64


IIUC, we are going to remove this once we remove the ngrams, right? But we will keep V3

Yeah, we will remove them later on

salvacorts · 2024-09-05T14:31:31Z

pkg/storage/bloom/v1/versioned_builder.go

 		opts:   opts,
 		writer: writer,
 		index:  NewIndexBuilder(opts, index),
 		blooms: NewBloomBlockBuilder(opts, blooms),
 	}, nil
 }

-func (b *V2Builder) BuildFrom(itr iter.Iterator[SeriesWithBlooms]) (uint32, error) {
+func (b *V3Builder) BuildFrom(itr iter.Iterator[SeriesWithBlooms]) (uint32, error) {


We have two builders: the merge builder and the regular builder. But IIUC, we only use the BuildFrom from the merge builder, right? If so, we should remove this method (in a separate PR probably).

BuildFrom is actually only used in tests for building "literal blooms".

salvacorts · 2024-09-05T14:34:33Z

pkg/storage/bloom/v1/versioned_builder.go


-type V2Builder struct {


Will we support building older versions of blocks? If not, does it make sense to keep V3Builder in case we add a V4Builder in the future? Or should we name this Builder right away and keep changing it as we add versions?

At some point when the feature is GA, yes. For now we can make all changes to the V3Builder.

Got rid of backwards compatibility shenanigans and introduced a new version that includes the indexed fields. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

pull-request-size bot added the size/XL label Sep 4, 2024

chaudum force-pushed the chaudum/block-schema branch from 1378b11 to 854e418 Compare September 4, 2024 15:54

rfratto reviewed Sep 4, 2024

View reviewed changes

chaudum marked this pull request as ready for review September 5, 2024 07:27

chaudum requested a review from a team as a code owner September 5, 2024 07:27

chaudum changed the title ~~chore(blooms): Introduce a new block schema (V3)~~ chore(blooms)!: Introduce a new block schema (V3) Sep 5, 2024

rfratto approved these changes Sep 5, 2024

View reviewed changes

salvacorts approved these changes Sep 5, 2024

View reviewed changes

chaudum added 10 commits September 5, 2024 16:54

Introduce new binary format for bloom index

5f00be8

Got rid of backwards compatibility shenanigans and introduced a new version that includes the indexed fields. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove unnecessary PeekingEntryIter adapter

6294628

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Check block schema compatibility on query path

8fed713

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Extract NewBloom() function from tokenizer

7017ee6

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Rename DefaultSchemaVersion to CurrentSchemaVersion

6e133fa

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove unused import

538d838

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Remove incorrect comment

b8ee038

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Make linter happy

5808546

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Introduce Set type for encoding structured metadata fields

a7c0a5e

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

Apply changes from code review

1eb3b56

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>

chaudum force-pushed the chaudum/block-schema branch from ea8a541 to 1eb3b56 Compare September 5, 2024 14:54

chaudum merged commit 5395daf into main Sep 5, 2024
60 checks passed

chaudum deleted the chaudum/block-schema branch September 5, 2024 16:04

loki-gh-app bot mentioned this pull request Sep 23, 2024

chore(k221): release 3.2.0 #14214

Open

loki-gh-app bot mentioned this pull request Sep 30, 2024

chore(k222): release 3.2.0 #14305

Open

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

chore(k223): release 3.2.0 #14402

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(blooms)!: Introduce a new block schema (V3) #14038

chore(blooms)!: Introduce a new block schema (V3) #14038

chaudum commented Sep 4, 2024 •

edited

Loading

rfratto left a comment

rfratto Sep 4, 2024

chaudum Sep 5, 2024

rfratto Sep 5, 2024

rfratto left a comment

salvacorts left a comment

salvacorts Sep 5, 2024

chaudum Sep 5, 2024

salvacorts Sep 5, 2024

chaudum Sep 5, 2024

salvacorts Sep 5, 2024

salvacorts Sep 5, 2024

chaudum Sep 5, 2024

salvacorts Sep 5, 2024

chaudum Sep 5, 2024

salvacorts Sep 5, 2024

chaudum Sep 5, 2024

	// V2 indicated schema for indexed structured metadata
	// V3 indicated schema for indexed structured metadata

chore(blooms)!: Introduce a new block schema (V3) #14038

chore(blooms)!: Introduce a new block schema (V3) #14038

Conversation

chaudum commented Sep 4, 2024 • edited Loading

rfratto 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

rfratto left a comment

Choose a reason for hiding this comment

salvacorts 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

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 commented Sep 4, 2024 •

edited

Loading