Introduce TSDB changes for appending metadata to the WAL by tpaschalis · Pull Request #10972 · prometheus/prometheus

tpaschalis · 2022-07-04T12:19:02Z

This is a rehash of the recent PR #10312 for appending metadata to the WAL, but focusing on the TSDB-related changes only.

It includes a new storage format in the WAL, the changes to the Appender interface in the Storage package, stubbing all the implementations, as well testing that the encoding/decoding functionality works as intended.

codesome

This is a first pass re-review of some parts. Mostly nits. I am left with the checkpoint code and the unit tests to review. I think we can remove it out of the draft.

codesome · 2022-07-12T12:49:42Z

storage/interface.go

+// MetadataAppender provides an interface for associating metadata to stored series.
+type MetadataAppender interface {
+	// AppendMetadata appends a metadata entry for the given series and labels.
+	// A series reference number is returned which can be used to modify the
+	// metadata of the given series in the same or later transactions.
+	// Returned reference numbers are ephemeral and may be rejected in calls
+	// to AppendMetadata() at any point. If the series does not exist, AppendMetadata
+	// returns an error.
+	// If the reference is 0 it must not be used for caching.
+	AppendMetadata(ref SeriesRef, l labels.Labels, m metadata.Metadata) (SeriesRef, error)


How about this? Because we are not appending metadata and storing a history of it. We are just updating it in-place.

Suggested change

// MetadataAppender provides an interface for associating metadata to stored series.

type MetadataAppender interface {

// AppendMetadata appends a metadata entry for the given series and labels.

// A series reference number is returned which can be used to modify the

// metadata of the given series in the same or later transactions.

// Returned reference numbers are ephemeral and may be rejected in calls

// to AppendMetadata() at any point. If the series does not exist, AppendMetadata

// returns an error.

// If the reference is 0 it must not be used for caching.

AppendMetadata(ref SeriesRef, l labels.Labels, m metadata.Metadata) (SeriesRef, error)

// MetadataUpdater provides an interface for associating metadata to stored series.

type MetadataUpdater interface {

// UpdateMetadata updates the metadata entry for the given series and labels.

// A series reference number is returned which can be used to modify the

// metadata of the given series in the same or later transactions.

// Returned reference numbers are ephemeral and may be rejected in calls

// to AppendMetadata() at any point. If the series does not exist, AppendMetadata

// returns an error.

// If the reference is 0 it must not be used for caching.

UpdateMetadata(ref SeriesRef, l labels.Labels, m metadata.Metadata) (SeriesRef, error)

Hmm, there's the same argument to be made about Exemplars as well right? I can get behind this change if you feel strongly about it, but (personally) I prefer how the current name works to extend the Appender interface.

You actually do append exemplars and don't update it in-place. You can have exemplars over time, not just the last one.

I thought about it but couldn't think of a better name, so let's go with your idea ^^

~~FYI~~ FWIW the same name is also used in the pre-existing code on the remote package, for the current implementation of sending metadata via the QueueManager

https://github.com/prometheus/prometheus/blob/main/storage/remote/metadata_watcher.go#L28-L31

Implemented in d86d5f7 :)

tsdb/agent/db.go

tsdb/docs/format/wal.md

tsdb/head_wal.go

tsdb/record/record.go

codesome

Nice work! While reading the test in db_test.go I realised that the behaviour of AppendMetadata when we use it to update the metadata of the same series more than once in the same transaction can be non-deterministic (like we can tell how it works, for a user it is non-deterministic). Should we document that?

Like if a series s1 had m1 meta already. And then in a transaction we do 3 updates m3 m2 m1 in order in the same transaction (i.e. before commit), then the meta m2 will be attached to s1 and not m1, while the user would expect m1.

tsdb/wal/checkpoint.go

codesome · 2022-07-13T07:46:08Z

tsdb/wal/checkpoint.go

+			repl := 0
+			for _, m := range metadata {
+				if keep(m.Ref) {
+					latestMetadataMap[m.Ref] = m


Note for future: once we have plugged in the scraping, we should run prombench on it and see if there are memory usage spikes after compactions when this checkpoint runs.

tsdb/wal/checkpoint.go

tsdb/wal/checkpoint_test.go

tsdb/db_test.go

tsdb/record/record.go