Distributor pushes to ingesters through a goroutines pool #6660

colega · 2023-11-14T17:42:51Z

What this PR does

This updates dskit to include grafana/dskit#431 and takes advantage of that to introduce a new flag distributor.ingester-push-worker-goroutines that can be configured to use a worker goroutines pool when sending push requests from the distributors to the ingesters, instead of creating new goroutines all the time.

This should reduce the amount of stack allocations happening, which take around 6% of distributor CPU time:

Which issue(s) this PR fixes or relates to

Ref: golang/go#18138

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

duricanikolic · 2023-11-14T17:56:37Z

pkg/distributor/distributor_test.go

@@ -292,6 +293,22 @@ func TestDistributor_Push(t *testing.T) {
 				cortex_distributor_latest_seen_sample_timestamp_seconds{user="user"} 123456789.009
 			`,
 		},
+		"A push to 3 happy ingesters using batch worker gouroutines should succeed": {


WDYT if we add another test in which we show, similarly to what is done in dskit, that setting cfg.IngesterPushWorkerGoroutines to a positive value, actually uses that number of workers?

I don't like the idea of counting goroutines here, with all the services and stuff going on, that can easily break. However, I've added another test below that wraps the workers with a function that counts the calls and checks the counter. This also served me to see that service wasn't properly created, this test wasn't calling configure, and workers weren't actually used 🤦

Thank you for adding this test.

duricanikolic · 2023-11-14T18:07:25Z

pkg/distributor/distributor.go

@@ -205,6 +212,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
 	f.DurationVar(&cfg.RemoteTimeout, "distributor.remote-timeout", 2*time.Second, "Timeout for downstream ingesters.")
 	f.BoolVar(&cfg.WriteRequestsBufferPoolingEnabled, "distributor.write-requests-buffer-pooling-enabled", false, "Enable pooling of buffers used for marshaling write requests.")
 	f.BoolVar(&cfg.LimitInflightRequestsUsingGrpcMethodLimiter, "distributor.limit-inflight-requests-using-grpc-method-limiter", false, "Use experimental method of limiting push requests.")
+	f.IntVar(&cfg.IngesterPushWorkerGoroutines, "distributor.ingester-push-worker-goroutines", 0, "Number of pre-allocated worker goroutines used to forward push requests to the ingesters. If 0, no workers will be used and a new goroutine will be spawned for each ingester push request. If not enough workers available, new goroutine will be spawned.")


-distributor.ingester-push-worker-goroutines sounds like "I am going to use at most this number of goroutines for ingester push requests", whereas it means "I will reuse this number of goroutines, and spawn new ones if it is needed".
WDYT if we call this -distributor.reusable-ingester-push-workers or -distributor.reusable-ingester-push-worker-goroutines?

distributor.reusable-ingester-push-workers or distributor.ingester-push-workers is my preference. I don't really like including "goroutines" in the name of the option, seems like it leaks implementation details.

Renamed to distributor.reusable-ingester-push-workers in 690cc0c

Thank you 🙏

56quarters

I'm curious about the behavior of this option. Right now, if there are no workers available we spawn new goroutines. It seems like that if there are no workers available, the distributor is under load and so spawning new goroutines would further increase the load.

Edit: Nevermind, this is a performance improvement, not intended for load shedding or something like that.

56quarters · 2023-11-14T18:56:48Z

pkg/distributor/distributor.go

@@ -205,6 +212,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
 	f.DurationVar(&cfg.RemoteTimeout, "distributor.remote-timeout", 2*time.Second, "Timeout for downstream ingesters.")
 	f.BoolVar(&cfg.WriteRequestsBufferPoolingEnabled, "distributor.write-requests-buffer-pooling-enabled", false, "Enable pooling of buffers used for marshaling write requests.")
 	f.BoolVar(&cfg.LimitInflightRequestsUsingGrpcMethodLimiter, "distributor.limit-inflight-requests-using-grpc-method-limiter", false, "Use experimental method of limiting push requests.")
+	f.IntVar(&cfg.IngesterPushWorkerGoroutines, "distributor.ingester-push-worker-goroutines", 0, "Number of pre-allocated worker goroutines used to forward push requests to the ingesters. If 0, no workers will be used and a new goroutine will be spawned for each ingester push request. If not enough workers available, new goroutine will be spawned.")


distributor.reusable-ingester-push-workers or distributor.ingester-push-workers is my preference. I don't really like including "goroutines" in the name of the option, seems like it leaks implementation details.

pkg/distributor/distributor.go

56quarters · 2023-11-14T19:01:23Z

pkg/distributor/distributor.go

+	// ingesterDoBatchPushGo is the Go function passed to ring.DoBatchWithOptions.
+	// It can be nil, in which case a simple `go f()` will be used.
+	// See Config.IngesterPushWorkerGoroutines on how to configure this.
+	ingesterDoBatchPushGo func(func())


nit: Can we use something else besides Go here? I'm struggling to understand what this is/does.

Renamed to ingesterDoBatchPushWorkers, does that work?
I called it Go because that's how I called the option in dskit, I'm also open to suggestion on naming there.

Since the new CLI flag is called distributor.reusable-ingester-push-workers, my suggestion is:

replace
ingesterDoBatchPushWorkers func(func())
with
reusableIngesterPushWorkersPool *concurrency.ReusableGoroutinesPool

instantiate it as
if cfg.ReusableIngesterPushWorkers > 0 { d.reusableIngesterPushWorkersPool := concurrency.NewReusableGoroutinesPool(cfg.ReusableIngesterPushWorkers) ... }

and use it as
ring.DoBatchOptions{ Cleanup: func() { pushReq.CleanUp(); cancel() }, IsClientError: isClientError, Go: d.reusableIngesterPushWorkersPool.Go, }

It is more intuitive what the type *concurrency.ReusableGoroutinesPool is comparing to just func(func()).

But then I need to add extra logic on each call checking whether there's a pool or not.

I don't want to block you on this. I approved your PR.
If we don't manage to find a better attribute name, let's merge it like this.

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

…th-goroutines-pool Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega · 2023-11-15T09:00:10Z

Thank you for your reviews, @duricanikolic, @56quarters, I've addressed your feedback, please take a look when possible 🙏.

duricanikolic

LGTM
Thank you

colega · 2023-11-16T08:45:34Z

I'm going to close this PR and perform a different approach described in golang/go#18138: just growing the stack in the distributor call.

That seems less complex, more local, and easier to revert in the long term.

colega · 2023-11-17T09:23:15Z

The growStack approach didn't work, so I'm continuing the work on this.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega · 2023-11-17T09:49:24Z

@56quarters merging this as I don't see any blocking feedback, I'll be happy to apply any changes afterwards if you find something.

colega added 3 commits November 14, 2023 18:17

Update dskit

89bdea7

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add support for workers when pushing to ingesters

52796f9

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Update CHANGELOG.md and about-versioning.md

dd99cae

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega marked this pull request as ready for review November 14, 2023 17:46

colega requested review from grafanabot and a team as code owners November 14, 2023 17:46

duricanikolic reviewed Nov 14, 2023

View reviewed changes

56quarters reviewed Nov 14, 2023

View reviewed changes

colega and others added 5 commits November 15, 2023 09:17

Update pkg/distributor/distributor.go

8404c87

Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com>

Rename to distributor.reusable-ingester-push-workers

690cc0c

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

s/ingesterDoBatchPushGo/ingesterDoBatchPushWorkers/

784de7f

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Test that workers are called, call them, fix svc

2c8bc90

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Merge remote-tracking branch 'origin/main' into distributor-pushes-wi…

cf06766

…th-goroutines-pool Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

duricanikolic approved these changes Nov 15, 2023

View reviewed changes

colega marked this pull request as draft November 16, 2023 11:38

colega marked this pull request as ready for review November 17, 2023 09:23

Merge branch 'main' into distributor-pushes-with-goroutines-pool

c117b5a

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega merged commit 1280630 into main Nov 17, 2023
28 checks passed

colega deleted the distributor-pushes-with-goroutines-pool branch November 17, 2023 09:49

colega mentioned this pull request Nov 29, 2023

Reduce stack allocations by reusing goroutines when pushing series from the distributors to the ingesters #6776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distributor pushes to ingesters through a goroutines pool #6660

Distributor pushes to ingesters through a goroutines pool #6660

colega commented Nov 14, 2023 •

edited

Loading

duricanikolic Nov 14, 2023

colega Nov 15, 2023

duricanikolic Nov 15, 2023

duricanikolic Nov 14, 2023

56quarters Nov 14, 2023

colega Nov 15, 2023

duricanikolic Nov 15, 2023

56quarters left a comment •

edited

Loading

56quarters Nov 14, 2023

56quarters Nov 14, 2023

colega Nov 15, 2023

duricanikolic Nov 15, 2023

colega Nov 15, 2023

duricanikolic Nov 15, 2023

colega commented Nov 15, 2023

duricanikolic left a comment

colega commented Nov 16, 2023

colega commented Nov 17, 2023

colega commented Nov 17, 2023

Distributor pushes to ingesters through a goroutines pool #6660

Distributor pushes to ingesters through a goroutines pool #6660

Conversation

colega commented Nov 14, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

56quarters left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colega commented Nov 15, 2023

duricanikolic left a comment

Choose a reason for hiding this comment

colega commented Nov 16, 2023

colega commented Nov 17, 2023

colega commented Nov 17, 2023

colega commented Nov 14, 2023 •

edited

Loading

56quarters left a comment •

edited

Loading