WIP: Remote write checkpointing by cstyan · Pull Request #8918 · prometheus/prometheus

cstyan · 2021-06-10T21:48:09Z

Still really rough, but working, implementation of #8809

cc: @rfratto

…te the segment record to a json file Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

…e, updated the file as well Signed-off-by: njingco <jingco.nicole@gmail.com>

…segment records from multiple endpoints Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

…tly update the json file every set time, only made queue_manager have an update function for the CheckpointRecord Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: njingco <jingco.nicole@gmail.com>

…reating the checkpoint file, added the context the timed checkpointing go thread Signed-off-by: njingco <jingco.nicole@gmail.com>

Signed-off-by: Callum Styan <callumstyan@gmail.com>

rfratto

(FYI, you probably didn't mean to check in the tsdb/tsdb or documentation/examples/remote_storage/example_write_adapter/server files)

rfratto · 2021-06-14T13:05:29Z

storage/remote/queue_manager.go

+	checkpointDir := filepath.Join(t.tsdbDir, "remote", t.client().Name())
+	err := os.MkdirAll(checkpointDir, 0777)
+	if err != nil {
+		level.Warn(t.logger).Log("msg", "error creating temporary remote write checkpoint directory", "error", err)


Should this return on error? Ditto with WriteFile/Replace failing.

rfratto · 2021-06-14T13:08:39Z

tsdb/wal/wal.go

+var (
+	castagnoliTable = crc32.MakeTable(crc32.Castagnoli)
+
+	MaxSegmentNumber = 99999999


Is math.MaxInt32/math.MaxInt64 more appropriate here?

yep, but these aren't being used anymore so I will remove them

rfratto · 2021-06-14T13:14:14Z

tsdb/wal/watcher.go

-func (w *Watcher) Run() error {
+// or an error case is hit. Use -1 for startSegment if the watcher should not
+// start tailing samples until after startTime.
+func (w *Watcher) Run(startSegment int) error {


Don't segment files start at 1? You might be able to use 0 instead of -1 here.

Nope, they start at 0

ls data/wal 00000000

👍 my bad :)

rfratto · 2021-06-14T13:16:36Z

tsdb/wal/watcher.go

+	for _, s := range segments {
+		if s == segmentNum {
+			return true, nil
+		}
+	}


Since the segments are sorted you may be able to do an early return here if s > segmentNum.

rfratto · 2021-06-14T13:21:28Z

tsdb/wal/watcher.go

 			}
 			for _, s := range samples {
+				w.samplesRead++
 				if s.T > w.startTimestamp {


Hmm, doesn't this have the same problem? Even if we start at a newer segment, we're still not going to queue any new samples until after time.Now() has passed.

It's smelly but at the moment we're only setting the start time if we didn't find/set the starting segment.

if startSegment == -1 { w.SetStartTime(time.Now()) }

rfratto · 2021-06-14T13:24:14Z

storage/remote/write.go

+	done chan struct{}
+	// Soft shutdown context will prevent new enqueues and deadlocks.
+	softShutdown chan struct{}
+
+	// Hard shutdown context is used to terminate outgoing HTTP connections
+	// after giving them a chance to terminate.
+	hardShutdown          context.CancelFunc
+	droppedOnHardShutdown atomic.Uint32


Are these being used anywhere? I see (at least) done being closed, but never read. Am I missing something?

No none of these were being used, don't think done was being closed either?

test. Signed-off-by: Callum Styan <callumstyan@gmail.com>

csmarchbanks

I don't love that the checkpoint logic, and things such as the tsdbDir and path, are spread across multiple packages. Would it be possible to keep most of the logic contained within watcher or queue_manager?

My one other thought is to make sure the checkpoint file format is expandable in the future for adding offsets. Do we want more structured data in that file?

csmarchbanks · 2021-06-14T19:05:23Z

tsdb/wal/watcher.go

 	metrics        *WatcherMetrics
 	readerMetrics  *LiveReaderMetrics

+	samplesSent int


Am I missing something, or are these unused other than to increment them?

csmarchbanks · 2021-06-14T19:15:25Z

tsdb/wal/watcher.go

 	// For testing, stop when we hit this segment.
 	MaxSegment int
+
+	mutex sync.RWMutex


Is this mutex used?

csmarchbanks · 2021-06-14T19:19:26Z

tsdb/wal/watcher.go

+	startSegment := -1
+
+	// look for a checkpoint file
+	fname := filepath.Join(w.tsdbDir, "remote", w.name, "checkpoint")


Putting remote into a wal specific file doesn't seem ideal. Could it just be watcher instead? Or specified when creating a Watcher?

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Agent / Remote Write code currently doesn't send the samples from the WAL on startup at all. We only send the new samples so replaying the WAL doesn't give us much. It does let us load series references from the WAL but we save much more by not reading the WAL at all. See: prometheus/prometheus#9848 and prometheus/prometheus#8918 Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

storage/remote to cache these segment offsets to disk. Deprecates prometheus#7710. Deprecates prometheus#8918. Closes prometheus#8809. Signed-off-by: Robert Fratto <robertfratto@gmail.com>

njingco and others added 12 commits May 31, 2021 21:40

Added SegmentFile data in the watcher structure, made function to wri…

27e1b52

…te the segment record to a json file Signed-off-by: njingco <jingco.nicole@gmail.com>

cleaned up code

a26027a

Signed-off-by: njingco <jingco.nicole@gmail.com>

add writing the remote write url to the remote SegmentRecord.json fil…

3a8f4f0

…e, updated the file as well Signed-off-by: njingco <jingco.nicole@gmail.com>

Added json error detection and handeling, added functionality to add …

fa2929a

…segment records from multiple endpoints Signed-off-by: njingco <jingco.nicole@gmail.com>

sync branch

c225540

Signed-off-by: njingco <jingco.nicole@gmail.com>

change the port number back to :1234

f5e3ea6

Signed-off-by: njingco <jingco.nicole@gmail.com>

removed offset element in the SegmentRecord.json

aec1e70

Signed-off-by: njingco <jingco.nicole@gmail.com>

added changes from PR review by Callum and Chris

8e01082

Signed-off-by: njingco <jingco.nicole@gmail.com>

moved json writing function to write.go, and added a timer to constan…

163d146

…tly update the json file every set time, only made queue_manager have an update function for the CheckpointRecord Signed-off-by: njingco <jingco.nicole@gmail.com>

moved the segment max and min variables to wal as per callums comment

59fe941

Signed-off-by: njingco <jingco.nicole@gmail.com>

added mutex to current Segment number, added the temporary file for c…

4b97556

…reating the checkpoint file, added the context the timed checkpointing go thread Signed-off-by: njingco <jingco.nicole@gmail.com>

Finish implementation of remote write checkpointing.

c5b6a9c

Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan requested a review from csmarchbanks June 10, 2021 21:48

cstyan requested review from bwplotka, codesome and tomwilkie as code owners June 10, 2021 21:48

cstyan removed request for bwplotka, codesome and tomwilkie June 10, 2021 21:52

rfratto reviewed Jun 14, 2021

View reviewed changes

Address review feedback + fix sample count access race in checkpoint

8fa2921

test. Signed-off-by: Callum Styan <callumstyan@gmail.com>

csmarchbanks reviewed Jun 14, 2021

View reviewed changes

More cleanup plus move watcher constructor args into a struct.

b44b545

Signed-off-by: Callum Styan <callumstyan@gmail.com>

rfratto mentioned this pull request Jul 21, 2021

Initial draft of prometheus-agent #8785

Merged

stale bot added the stale label Aug 28, 2021

gouthamve mentioned this pull request Nov 24, 2021

Remove WAL on startup grafana/agent#1132

Closed

3 tasks

rfratto mentioned this pull request Nov 24, 2021

tsdb/wal: add support for saving and restoring read segments #9862

Closed

cstyan closed this by deleting the head repository Aug 14, 2023

Conversation

cstyan commented Jun 10, 2021

Uh oh!

rfratto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csmarchbanks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

csmarchbanks left a comment •

edited

Loading