wlog: Optimized and refactored watcher code. by bwplotka · Pull Request #16182 · prometheus/prometheus

bwplotka · 2025-03-07T09:03:58Z

This cleans up the watcher structure and tests. No functionality should be changed.

Changes:

Correctly reuse Ref slices. Seems by accident they were shadowing variables.
Add zeropools for even more performance, ensured interface WriteTo is compatible (it allows pooling).
Simplified Watcher public methods.
Removed unnecessary leaked abstractions and test code in production struct e.g. starttime, max segment, read timeout, eofNonErr
Made the tailing/reading series less confusing. TIL watcher reads only from time.Now after retry and ONLY from the latest segment (: Shouldn't it read from the beginning?
Removed obsolete queue_manager benchmark.

I noticed the need of this in #16046 (profiling suggesting watcher decoding being a significant overhead no matter of sample type).

storage/remote/queue_manager_test.go

bboreham · 2025-03-10T10:05:06Z

Shouldn't it read from the beginning

Beginning is hours ago, possibly tens of hours. See #8809 for another idea.

bboreham

Scary to change this code, but the shadowing is a great catch.
I haven't done a full review, just a quick initial scan.

tsdb/wlog/watcher.go

bwplotka · 2025-03-10T13:52:37Z

Thanks for a quick review @bboreham - I was fighting with the super confusing behaviour of LiveReader (the eofNonErr testing flag in production code....). Proposed much cleaner approach (IMO). Addressed the initial comments too. Will work on the benchmark results now, but marking as good for review.

@bwplotka

The `:=` causes new variables to be created, which means the outer slice stays at nil, and new memory is allocated every time round the loop. Extracted from prometheus#16182 Credit to @bwplotka. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

bboreham · 2025-03-10T17:56:17Z

I'm excited about the pessimisation you found - extracted #16197 in the hope that we can merge it faster than this 900-line PR.

@bwplotka

The `:=` causes new variables to be created, which means the outer slice stays at nil, and new memory is allocated every time round the loop. Extracted from #16182 Credit to @bwplotka. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

Signed-off-by: bwplotka <bwplotka@gmail.com> # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher.go

bwplotka · 2025-03-11T13:12:03Z

Should be ready for review @bboreham PTAL

@bwplotka

The `:=` causes new variables to be created, which means the outer slice stays at nil, and new memory is allocated every time round the loop. Extracted from prometheus#16182 Credit to @bwplotka. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in productio

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in production

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in production Signed-off-by: Adam Bernot <bernot@google.com>

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in production Signed-off-by: Adam Bernot <bernot@google.com> Signed-off-by: perebaj <perebaj@gmail.com>

bboreham · 2025-10-28T11:33:37Z

Hello from the bug-scrub! I will take another look.

bwplotka · 2025-11-03T14:22:19Z

This needs a rebase, will do soon

bboreham · 2025-11-11T09:09:53Z

I'm looking at this PR; I've done the rebase locally, if you are brave enough to let me push it.

bboreham

Generally fine, though I would like to resolve the TODOs before merging.
I didn't follow all the test changes; perhaps describe them more fully in the PR?

bboreham · 2025-11-11T09:14:20Z

tsdb/wlog/watcher.go

+// We do this here rather than in the constructor because of the ordering of
+// creating Queue Managers's, stopping them, and then starting new ones in
+// storage/remote/storage.go ApplyConfig.
+func (w *Watcher) initMetrics() {


startMetrics in comment vs initMetrics in code

bboreham · 2025-11-11T09:21:01Z

tsdb/wlog/watcher.go

+
+// readSegment reads all known records into w.writer from a segment.
+// It returns the EOF error if the segment is corrupted or partially written.
+func (w *Watcher) readSegment(r *LiveReader, startT int64, segmentNum int) (err error) {


This might be clearer if named more like readSegmentAllData, since the counterpart is readSegmentSeries.

bboreham · 2025-11-11T09:22:13Z

tsdb/wlog/watcher.go

+		// One table per WAL segment means it won't grow indefinitely.
+		dec = record.NewDecoder(labels.NewSymbolTable())
+
+		// TODO(bwplotka): Consider zeropools.


Pool should be worse, since we are already reusing memory and there is only ever zero or one slice of each type in use.

bboreham · 2025-11-11T09:23:03Z

tsdb/wlog/watcher.go

+				if h.T > startT {
+					if !w.replayDone {
+						w.replayDone = true
+						w.logger.Info("Done replaying WAL", "duration", time.Since(timestamp.Time(startT)))


Could this be a function, to avoid repeating it three times?

bboreham · 2025-11-11T09:23:56Z

tsdb/wlog/watcher.go

 				break
 			}
-			metadata, err = dec.Metadata(rec, metadata[:0])
+			meta, err := dec.Metadata(rec, metadata[:0])


This is an accident, I think, reverting part of the optimisation from #16197.

bboreham · 2025-11-11T09:55:43Z

tsdb/wlog/watcher.go

+	// TODO(bwplotka): Checking every 100ms feels too frequent. It might be enough
+	// to check on notify AND with emergency 15s read only.


I agree that scanning the whole WAL directory every 100ms is too much.
However, this is the only way that watchSegment returns (except for errors), and hence the only way we move to the next segment. We cannot wait 15s for that.
Possibly we could extend Notify to include the information that there is a new segment?
Maybe we can observe that we got notified and no new data was in the file, so we should look at that point for a new segment?

bwplotka requested a review from bboreham March 7, 2025 09:07

bwplotka force-pushed the watcher-opt branch 2 times, most recently from d8d616f to 2ee34a1 Compare March 7, 2025 09:14

bwplotka marked this pull request as ready for review March 7, 2025 09:14

bwplotka requested a review from jesusvazquez as a code owner March 7, 2025 09:14

bwplotka force-pushed the watcher-opt branch from 2ee34a1 to 38ce1a3 Compare March 7, 2025 09:16

bwplotka requested review from cstyan and tomwilkie as code owners March 7, 2025 09:16

bwplotka marked this pull request as draft March 7, 2025 09:34

bwplotka mentioned this pull request Mar 7, 2025

ct: Support CTs in WAL; change sample record; use in PRW 2.0 #16046

Draft

bwplotka commented Mar 10, 2025

View reviewed changes

storage/remote/queue_manager_test.go Show resolved Hide resolved

bboreham reviewed Mar 10, 2025

View reviewed changes

tsdb/wlog/watcher.go Outdated Show resolved Hide resolved

tsdb/wlog/watcher.go Show resolved Hide resolved

tsdb/wlog/watcher.go Outdated Show resolved Hide resolved

tsdb/wlog/watcher.go Outdated Show resolved Hide resolved

bwplotka force-pushed the watcher-opt branch from 38ce1a3 to dd25b6f Compare March 10, 2025 13:48

bwplotka marked this pull request as ready for review March 10, 2025 13:50

bwplotka force-pushed the watcher-opt branch from dd25b6f to f939bf6 Compare March 10, 2025 13:50

This comment was marked as resolved.

Sign in to view

bwplotka force-pushed the watcher-opt branch 3 times, most recently from 47cf03b to efd6289 Compare March 10, 2025 14:41

bwplotka mentioned this pull request Mar 10, 2025

WAL Watcher: refactor reading WAL segments #14439

Closed

bwplotka force-pushed the watcher-opt branch 3 times, most recently from eca1e09 to c76a5e3 Compare March 10, 2025 16:00

bboreham mentioned this pull request Mar 10, 2025

[PERF] Remote-write: re-use memory to read WAL data #16197

Merged

wlog: Optimized and refactored watcher code.

2df21da

Signed-off-by: bwplotka <bwplotka@gmail.com> # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher.go

bwplotka force-pushed the watcher-opt branch 3 times, most recently from b96b8f8 to e325164 Compare March 11, 2025 12:12

wlog: Optimized and refactored watcher code.

dd9d853

Signed-off-by: bwplotka <bwplotka@gmail.com> # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher_test.go # Conflicts: # tsdb/wlog/watcher.go

bwplotka force-pushed the watcher-opt branch from e325164 to dd9d853 Compare March 11, 2025 12:44

github-actions bot added the stale label May 16, 2025

bernot-dev added a commit to bernot-dev/prometheus that referenced this pull request Aug 12, 2025

test: remove obsolete test

2279077

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in productio

bernot-dev added a commit to bernot-dev/prometheus that referenced this pull request Aug 12, 2025

test: remove obsolete test

e13514e

As mentioned in prometheus#16182, the BenchmarkStartup test for the queue manager covers an old API and uses settings that will not occur in production

bernot-dev mentioned this pull request Aug 12, 2025

test: remove obsolete queue manager test #17041

Merged

bboreham self-assigned this Oct 28, 2025

github-actions bot removed the stale label Oct 28, 2025

bboreham reviewed Nov 11, 2025

View reviewed changes

bwplotka mentioned this pull request Feb 3, 2026

tsdb(wal): st-per-sample initial code and benchmarks #17671

Merged

austindyoung mentioned this pull request Feb 26, 2026

[mirror] prometheus/prometheus#16182 wlog: Optimized and refactored watcher code. austindyoung/prometheus-driftfence-mirror-fork#222

Open

		// TODO(bwplotka): Checking every 100ms feels too frequent. It might be enough
		// to check on notify AND with emergency 15s read only.

Conversation

bwplotka commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bboreham commented Mar 10, 2025

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka commented Mar 10, 2025

Uh oh!

This comment was marked as resolved.

bboreham commented Mar 10, 2025

Uh oh!

bwplotka commented Mar 11, 2025

Uh oh!

bboreham commented Oct 28, 2025

Uh oh!

bwplotka commented Nov 3, 2025

Uh oh!

bboreham commented Nov 11, 2025

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

bboreham Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bwplotka commented Mar 7, 2025 •

edited

Loading