Write chunks via queue, predicting the refs by replay · Pull Request #10051 · prometheus/prometheus

replay · 2021-12-20T19:50:36Z

Our load tests have shown that there is a latency spike in the
remote write handler whenever the head chunks need to be written,
because chunkDiskMapper.WriteChunk() blocks until the chunks are written
to disk.

This adds a queue to the chunk disk mapper which makes the WriteChunk()
method non-blocking unless the queue is full. Reads can still be served
from the queue.

Our load tests have shown that there is a latency spike in the remote write handler whenever the head chunks need to be written, because chunkDiskMapper.WriteChunk() blocks until the chunks are written to disk. This adds a queue to the chunk disk mapper which makes the WriteChunk() method non-blocking unless the queue is full. Reads can still be served from the queue. Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

tsdb/chunks/chunk_write_queue.go

tsdb/chunks/head_chunks.go

tsdb/chunks/chunk_write_queue.go

tsdb/chunks/head_chunks.go

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

tsdb/chunks/chunk_write_queue.go

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

tsdb/chunks/chunk_write_queue.go

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

codesome

I am currently reviewing this, but there is one common comment for entire change: code comments should start with capital letters (except when referencing a variable that starts with lower case), and end with a period ..

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay · 2022-01-05T14:07:50Z

code comments should start with capital letters (except when referencing a variable that starts with lower case), and end with a period ..

Thanks, I fixed it.

codesome

Looking good with some nits! Let's run prombench till tomorrow on this while I finish reviewing some unit test changes.

tsdb/chunks/chunk_write_queue.go

tsdb/chunks/chunk_write_queue_test.go

tsdb/chunks/head_chunks.go

codesome · 2022-01-05T14:52:31Z

/prombench main

prombot · 2022-01-05T14:52:32Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-10051 and main

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

codesome · 2022-01-06T08:49:23Z

/prombench cancel

All looks good

prombot · 2022-01-06T08:49:25Z

Benchmark cancel is in progress.

codesome

Some comments about changed test semantics, should be good to merge after fixing those

tsdb/chunks/head_chunks_test.go

codesome · 2022-01-06T09:22:37Z

tsdb/chunks/head_chunks_test.go

 	// though files 4 and 6 are empty.
 	file2Maxt := hrw.mmappedChunkFiles[2].maxt
 	require.NoError(t, hrw.Truncate(file2Maxt+1))
-	// As 6 was empty, it should not create another file.


We need to test this. We should add a chunk and see that 3,4,5,6 is present and no 7. And then truncate and add a chunk to create the file 7 (we need this truncate to make sure again that it preserves sequence of files).

is this what you had in mind? 04e94b9

Not exactly, we should be truncating file2Maxt+1 like before. Because the bug was the it was deleting empty files from later files.

tsdb/chunks/head_chunks_test.go

tsdb/head_test.go

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

Co-authored-by: Ganesh Vernekar <15064823+codesome@users.noreply.github.com> Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay · 2022-01-06T15:52:59Z

I also replaced most occurrences of callbackWg in tsdb/chunks/chunk_write_queue_test.go with awaitCb (6bd1f8e). But there was one test where this wasn't possible because it's critical that we can wait for the callback after the call to addChunk, so I left the callbackWg there: TestChunkWriteQueue_WrappingAroundSizeLimit

tsdb/chunks/head_chunks_test.go

codesome

Thanks!

* Write chunks via queue, predicting the refs Our load tests have shown that there is a latency spike in the remote write handler whenever the head chunks need to be written, because chunkDiskMapper.WriteChunk() blocks until the chunks are written to disk. This adds a queue to the chunk disk mapper which makes the WriteChunk() method non-blocking unless the queue is full. Reads can still be served from the queue. Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * address PR feeddback Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * initialize metrics without .Add(0) Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * change isRunningMtx to normal lock Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * do not re-initialize chunkrefmap Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * update metric outside of lock scope Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * add benchmark for adding job to chunk write queue Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * remove unnecessary "success" var Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * gofumpt -extra Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * avoid WithLabelValues call in addJob Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * format comments Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * addressing PR feedback Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * rename cutExpectRef to cutAndExpectRef Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * use head.Init() instead of .initTime() Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * address PR feedback Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * PR feedback Co-authored-by: Ganesh Vernekar <15064823+codesome@users.noreply.github.com> Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * update test according to PR feedback Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * replace callbackWg -> awaitCb Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * better test of truncation with empty files Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> * replace callbackWg -> awaitCb Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com> Co-authored-by: Ganesh Vernekar <15064823+codesome@users.noreply.github.com> Signed-off-by: suyashtava <suyashtava@gmail.com>

replay marked this pull request as ready for review December 20, 2021 20:24

replay requested a review from codesome as a code owner December 20, 2021 20:24

replay mentioned this pull request Dec 21, 2021

Ingesters latency spike caused by mmap-ed chunks grafana/mimir#470

Closed

colega reviewed Jan 3, 2022

View reviewed changes

address PR feeddback

3e9160c

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the chunk_write_queue branch from fa52843 to 3e9160c Compare January 4, 2022 14:17

colega reviewed Jan 4, 2022

View reviewed changes

tsdb/chunks/chunk_write_queue.go Outdated Show resolved Hide resolved

initialize metrics without .Add(0)

210f9fa

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the chunk_write_queue branch from 3f1544c to 210f9fa Compare January 4, 2022 17:11

replay added 3 commits January 4, 2022 17:21

change isRunningMtx to normal lock

de725f3

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

do not re-initialize chunkrefmap

450e9bd

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

update metric outside of lock scope

855f156

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

colega reviewed Jan 5, 2022

View reviewed changes

tsdb/chunks/chunk_write_queue.go Outdated Show resolved Hide resolved

replay added 4 commits January 5, 2022 12:49

add benchmark for adding job to chunk write queue

ba8df03

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

remove unnecessary "success" var

673040b

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

gofumpt -extra

b4e587b

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

avoid WithLabelValues call in addJob

e0c1ca3

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

codesome reviewed Jan 5, 2022

View reviewed changes

format comments

c468827

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

codesome reviewed Jan 5, 2022

View reviewed changes

prombot added the prombench label Jan 5, 2022

replay added 2 commits January 5, 2022 22:18

addressing PR feedback

07b70c8

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

rename cutExpectRef to cutAndExpectRef

544c2ca

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the chunk_write_queue branch from cbefe75 to 544c2ca Compare January 5, 2022 22:24

codesome reviewed Jan 6, 2022

View reviewed changes

replay and others added 3 commits January 6, 2022 12:58

use head.Init() instead of .initTime()

92ec6df

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

address PR feedback

3e624ef

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

PR feedback

3954160

Co-authored-by: Ganesh Vernekar <15064823+codesome@users.noreply.github.com> Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the chunk_write_queue branch from f3802ad to 07ce3f9 Compare January 6, 2022 13:23

update test according to PR feedback

4d3d594

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the chunk_write_queue branch from 07ce3f9 to 4d3d594 Compare January 6, 2022 13:24

replay added 3 commits January 6, 2022 13:32

replace callbackWg -> awaitCb

575ff12

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

better test of truncation with empty files

04e94b9

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replace callbackWg -> awaitCb

6bd1f8e

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

codesome reviewed Jan 10, 2022

View reviewed changes

tsdb/chunks/head_chunks_test.go Show resolved Hide resolved

replay force-pushed the chunk_write_queue branch from 69db63b to 6bd1f8e Compare January 10, 2022 13:21

codesome approved these changes Jan 10, 2022

View reviewed changes

codesome enabled auto-merge (squash) January 10, 2022 13:36

codesome merged commit 0df3489 into prometheus:main Jan 10, 2022

replay deleted the chunk_write_queue branch January 10, 2022 13:45

replay mentioned this pull request Jan 12, 2022

Nits after PR 10051 merge #10159

Merged

roidelapluie mentioned this pull request Feb 16, 2022

panic: expected newly cut file to have sequence:offset #10291

Closed

codesome mentioned this pull request Feb 18, 2022

Panic after failed m-map file replay due to corruption #10315

Closed

prymitive mentioned this pull request Mar 2, 2022

Rule evaluation & scrapes are frequently blocked or delayed with Prometheus >= v2.33.0 #10377

Closed

codesome mentioned this pull request Jun 4, 2022

TSDB append latency spike when chunks are being m-mapped #9869

Closed

prymitive mentioned this pull request Jun 4, 2022

Write & mmap chunks in the background #10709

Closed

This was referenced Jun 16, 2022

Reduce chunk write queue memory usage 1 #10873

Merged

Reduce chunk write queue memory usage 2 #10874

Merged

philipgough mentioned this pull request Aug 3, 2022

receive: Expose write chunk queue as flag thanos-io/thanos#5566

Merged

2 tasks

Conversation

replay commented Dec 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

replay commented Jan 5, 2022

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codesome commented Jan 5, 2022

Uh oh!

prombot commented Jan 5, 2022

Uh oh!

codesome commented Jan 6, 2022

Uh oh!

prombot commented Jan 6, 2022

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codesome Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

replay Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

codesome Jan 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

replay commented Jan 6, 2022

Uh oh!

Uh oh!

codesome left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants