move WAL tailing code from Prometheus to TSDB WAL package #606

cstyan · 2019-05-17T06:12:45Z

This is probably a bit of a pain to review, but moving the WAL watcher code from Prometheus to TSDB (into the WAL package) requires moving some code from the root tsdb package. Specifically the Watcher uses RefSeries and RefSample. Moving those requires a bunch of other code to be moved as well.

If anyone knows a nicer way to make the RefSeries/RefSamples change or has thoughts about the new package organization let me know, none of this is set in stone.

cc @tomwilkie @gouthamve

csmarchbanks

Generally I think moving all the record related code to a record package makes a lot of sense. I would try to limit code in that package to just types/methods directly interacting with a record though. Left a couple more specific comments around things you might be able to move out.

record/internal.go

record/tombstones.go

csmarchbanks · 2019-06-06T03:12:37Z

Thanks for all the responses, they helped me get my head around all this! As you said, the amount of code in the root package can be problematic to wrap your head around.

My main concern with the current state of this PR is that the record package appears to be random types with no obvious relation. I don't think it will help the code being more isolated/easier to wrap your head around.

After thinking about this for a couple hours:

I am pretty against memSeries being moved away from head.
- Perhaps have a slice of *memSeries next to the slice of RefSamples in headAppender instead of having RefSample to depend on memSeries?
Should the record code (and RefSample/RefSeries) live in wal? Are records used anywhere else? Otherwise I like them living in their own record package.
Perhaps tombstones should be in their own package? I don't feel too strongly about this either, just having Stone live wherever RefSample does would be fine by me too.

I am definitely curious what you and the maintainers think!

codesome · 2019-06-06T07:11:57Z

As I am not much familiar with WAL tailing yet, I am still wrapping my head about what the record package should be. But I agree with some points which @csmarchbanks said

memSeries looks closely tied with Head and would be good to keep it there.
If there are only a few things used from tombstones.go in the record, then duplicate only those constructs and not the entire file. Else tombstones as a separate package would make more sense than being in inside record. (I support the former)

I will take a better look before the weekend hits.

cstyan · 2019-06-07T22:16:47Z

I've moved the tombstones code to it's own package. Unfortunately keeping memSeries in head and have the records package RefSample not contain memSeries is going to be pretty messy.

cstyan · 2019-06-10T17:16:16Z

cc @csmarchbanks @codesome

csmarchbanks · 2019-06-10T18:41:05Z

I like the tombstone move. I think you could remove the memSeries from RefSample without it becoming too messy by keeping two slices in sync. What do you think of
memSeries.patch.txt? You would have to apply to master, not your branch.

krasi-georgiev · 2019-06-10T19:34:51Z

What is the problem you are trying to solve with this PR?

cstyan · 2019-06-10T20:29:42Z

@krasi-georgiev the code for tailing/watching the WAL currently lives in the remote write package in prometheus/prometheus. We think it makes more sense for it to live in the WAL package, which is in tsdb repo right now. Unfortunately the WAL tailing code relies on a couple of types that live in the root tsdb package, which would end up causing some circular dependency issues.

krasi-georgiev · 2019-06-12T12:39:10Z

hm , I see.
In that case need to spend some more time to look into this.

csmarchbanks

Did another pass tonight, generally I am liking the refactor and beginnings of pulling things out of the base package.

record/internal.go

wal/wal_watcher.go

wal/checkpoint.go

cstyan · 2019-07-05T15:25:51Z

@csmarchbanks thanks for the review and for the sync.Pool suggestion #606 (comment)

codesome

Not familiar with WAL Watcher, but other changes look fine.

head.go

record/internal.go

wal/watcher.go

csmarchbanks

Starting to look good from my perspective! Left another handful of comments, mostly pretty minor.

head.go

record/internal.go

wal/watcher.go

wal/watcher_test.go

cstyan · 2019-07-19T21:02:27Z

@csmarchbanks @codesome could use another review when you have some time

csmarchbanks

A couple more comments.

csmarchbanks · 2019-07-19T21:52:25Z

wal/watcher.go

+	SeriesReset(int)
+}
+
+type watcherMetrics struct {


I would prefer if this were WatcherMetrics to make it easier to pass around elsewhere.

csmarchbanks · 2019-07-19T21:53:26Z

wal/watcher.go

+}
+
+// NewWatcher creates a new WAL watcher for a given WriteTo.
+func NewWatcher(reg prometheus.Registerer, logger log.Logger, name string, writer WriteTo, walDir string) *Watcher {


This should take a WatcherMetrics, not a registerer, otherwise creating two watchers with the same registerer would panic.

Both NewWatcherMetrics and NewLiveReaderMetrics call reg.Register, not reg.MustRegister. But I guess the real question here is whether it's reasonable for consumers of the WAL Watcher to have to create and pass the metrics struct rather than just passing a Registerer.

True, reg.Register won't panic, but it also won't register the new metrics, so we would be blind for those queues/after a reload.

I think it is reasonable to have a consumer create and pass the metrics struct, just like NewLiveReader.

csmarchbanks · 2019-07-19T22:21:09Z

wal/watcher.go

+}
+
+// NewWatcher creates a new WAL watcher for a given WriteTo.
+func NewWatcher(reg prometheus.Registerer, logger log.Logger, name string, writer WriteTo, walDir string) *Watcher {


True, reg.Register won't panic, but it also won't register the new metrics, so we would be blind for those queues/after a reload.

I think it is reasonable to have a consumer create and pass the metrics struct, just like NewLiveReader.

csmarchbanks · 2019-07-19T22:24:31Z

wal/watcher.go

+	maxSegment int
+}
+
+func NewWatcherMetrics(reg prometheus.Registerer) *watcherMetrics {


Should these take a consumer and provide an Unregister so that if a consumer disappears it can be removed from the metrics?

We can't Unregister as all the Watcher metrics are under the same metric but just WithLabels. Both QueueManager and Watcher Stop delete their metrics via DeleteLabelValues.

codesome

LGTM. I hope I did not miss anything, PR is big.

tombstones/tombstones.go

wal/reader_test.go

codesome · 2019-07-26T11:22:18Z

Forgot to mention I haven't reviewed wal/watcher.go assuming that it was already reviewed when it was added to Prometheus.