Repository: prometheus/prometheus (language: go)

Issue:
Remote-write: threshold to skip resharding should be higher
I saw a lot of log lines like this:

```
ts=2024-05-02T14:02:48.270112953Z level=warn msg="Skipping resharding, last successful send was beyond threshold" [...] lastSendTimestamp=1714658566 minSendTimestamp=1714658568
```

Context was that we wanted to feed data in a timely manner, so `BatchSendDeadline` had been reduced to 100ms.

The code that generates the message: 
https://github.com/prometheus/prometheus/blob/94c81bba419b6c8ad993a182f357d722c78cd9c1/storage/remote/queue_manager.go#L1021-L1026

is called every 10s (hard-coded), so if `BatchSendDeadline` is any less than 5s we stand some chance that we didn't even try to send within that interval.

### Proposal

I suggest the check should be within `2 * time.Duration(t.cfg.BatchSendDeadline) + shardUpdateDuration`.


Discussion / hints:
I notice in a similar previous issue @csmarchbanks said https://github.com/prometheus/prometheus/issues/7124#issuecomment-613634182

> At very low remote write volumes it is very easy to go through multiple batch send durations without new samples coming in

Which matches the situation in this case (volume was about 500 series, scraped every 5s).
Anyway this judgement seems highly dependent on the value of `BatchSendDeadline`; is it unreasonable to set it to 100ms?

> low volumes are unlikely to ever reshard above the minimum anyway

More context: the machine was occasionally under heavy CPU load; I believe this generated a backlog on the send queue.
(Sadly I don't have metrics to confirm this)

is this good @bboreham or do we need to wait for others review .
Best to comment on the PR within the PR itself.

okay @bboreham . 
> Which matches the situation in this case (volume was about 500 series, scraped every 5s).
> Anyway this judgement seems highly dependent on the value of `BatchSendDeadline`; is it unreasonable to set it to 100ms?

I would have suggested/assumed people would drop the batch size `max_samples_per_send` a lot lower before dropping the `BatchSendDeadline` that low.
I don't think that helps.  In my example, Prometheus scraped 509 series every 5 seconds; I wanted it to send those 509 series without waiting 5 seconds.
If I reduce `max_samples_per_send` from 2000 default to 100, say, it will send 500 of them, but I still want it to send all 509 series.
Personally  I would still lower the batch size before the send deadline timeout, but even so I think guarding against excessive resharding checks is a valid change. Reviewing the PR again today.
I don't think I am understanding your point.  What would you lower `max_samples_per_send` to, given my example?
> I don't think I am understanding your point. What would you lower `max_samples_per_send` to, given my example?

Something below `509`? Or even just `1000` or so and lower the send deadline to ~1s. I don't know exactly what your use case is but scraping a small amount of samples and then always sending _all_ of them via remote write ASAP isn't really a situation we've designed for. Setting the send deadline to 100ms is just a workaround that's worked in your case.

This is separate from the issue of the the resharding check happening too often when the send deadline is < 5s, which  I don't have any issue with merging a fix for.
> lower the send deadline to ~1s

OK, that case still shows the issue I am talking about, because two times 1 second is way less than the 10s interval it checks at.

> always sending all of them via remote write ASAP

That isn't what I asked for; I asked for:

> > in a timely manner

and

> > without waiting 5 seconds

> Setting the send deadline to 100ms is just a workaround that's worked in your case.

I disagree, it matches what I wanted.

Bryan
Bryan and I discussed on Slack; over text we'd misunderstood each other. His config changes were definitely valid given the low scrape load he had. Remote write has some gaps when it comes to handling timely sending of data in that kind of a scenario, the hard coding of the reshard check ticker is just one of those gaps.

I'll be opening a few issues soon for some things we can try out, there are a number of people interested in taking on some smaller tasks in RW and those could be good first issues for them.
yeah after seeing and understanding code @bboreham  assumption here is valid as we dont need to wait for 5 seconds , and yeah as @bboreham  also asked for feeding data timely manner  .  and also we need to remove that hard coding of the resharding check ticker .  
Hey, I want to contribute to cloud native projects, I am quite starting my open source joruney,
Can anyone tell how to understand all this? Where to start from? It would be a lot of help if someone could point out in the right direction or point to a resource @bboreham I currently know Java, can I contribute with it or GO is required?
Hi @CodexRaunak and thanks for your interest.
Prometheus is written in Go and TypeScript, no Java.

A great first step would be to get Prometheus running, and have it monitor something of interest to you.
This will be useful even if you find a different project to contribute to.
There are lots of blog posts and videos how to get started.

On the coding side, get enough of a dev environment working to run the unit tests.
See if you can make one of them break by editing the code described in this issue.

Fix patch:
diff --git a/storage/remote/queue_manager.go b/storage/remote/queue_manager.go
index fb13da70da7..d47be3731a6 100644
--- a/storage/remote/queue_manager.go
+++ b/storage/remote/queue_manager.go
@@ -1108,9 +1108,9 @@ func (t *QueueManager) shouldReshard(desiredShards int) bool {
 	if desiredShards == t.numShards {
 		return false
 	}
-	// We shouldn't reshard if Prometheus hasn't been able to send to the
-	// remote endpoint successfully within some period of time.
-	minSendTimestamp := time.Now().Add(-2 * time.Duration(t.cfg.BatchSendDeadline)).Unix()
+	// We shouldn't reshard if Prometheus hasn't been able to send
+	// since the last time it checked if it should reshard.
+	minSendTimestamp := time.Now().Add(-1 * shardUpdateDuration).Unix()
 	lsts := t.lastSendTimestamp.Load()
 	if lsts < minSendTimestamp {
 		level.Warn(t.logger).Log("msg", "Skipping resharding, last successful send was beyond threshold", "lastSendTimestamp", lsts, "minSendTimestamp", minSendTimestamp)


Test patch:
diff --git a/storage/remote/queue_manager_test.go b/storage/remote/queue_manager_test.go
index 9ab563edab7..6a9e727be4e 100644
--- a/storage/remote/queue_manager_test.go
+++ b/storage/remote/queue_manager_test.go
@@ -711,32 +711,35 @@ func TestShouldReshard(t *testing.T) {
 		startingShards                           int
 		samplesIn, samplesOut, lastSendTimestamp int64
 		expectedToReshard                        bool
+		sendDeadline                             model.Duration
 	}
 	cases := []testcase{
 		{
-			// Resharding shouldn't take place if the last successful send was > batch send deadline*2 seconds ago.
+			// resharding shouldn't take place if we haven't successfully sent
+			// since the last shardUpdateDuration, even if the send deadline is very low
 			startingShards:    10,
 			samplesIn:         1000,
 			samplesOut:        10,
-			lastSendTimestamp: time.Now().Unix() - int64(3*time.Duration(config.DefaultQueueConfig.BatchSendDeadline)/time.Second),
+			lastSendTimestamp: time.Now().Unix() - int64(shardUpdateDuration),
 			expectedToReshard: false,
+			sendDeadline:      model.Duration(100 * time.Millisecond),
 		},
 		{
-			startingShards:    5,
+			startingShards:    10,
 			samplesIn:         1000,
 			samplesOut:        10,
 			lastSendTimestamp: time.Now().Unix(),
 			expectedToReshard: true,
+			sendDeadline:      config.DefaultQueueConfig.BatchSendDeadline,
 		},
 	}
 
 	for _, c := range cases {
-		_, m := newTestClientAndQueueManager(t, defaultFlushDeadline, config.RemoteWriteProtoMsgV1)
+		_, m := newTestClientAndQueueManager(t, time.Duration(c.sendDeadline), config.RemoteWriteProtoMsgV1)
 		m.numShards = c.startingShards
 		m.dataIn.incr(c.samplesIn)
 		m.dataOut.incr(c.samplesOut)
 		m.lastSendTimestamp.Store(c.lastSendTimestamp)
-
 		m.Start()
 
 		desiredShards := m.calculateDesiredShards()