Repository: cloudprober/cloudprober

Issue:
[prober] Spread low frequency probes better
We limit the inter-probe interval to 2s:
https://github.com/cloudprober/cloudprober/blob/24200671b86da6763fcb176ac9a974f42f699d06/prober/prober.go#L156

That works for high frequency and low resource overhead probes like HTTP. For something like a browser probe, where typical intervals are in minutes (2-15 min) and probes are resource heavy, it's not a good strategy. We need to improve this logic.


Discussion:
@manugarg: To keep it simple, I am in favor of raising the max inter-probe interval to 5m. Maybe we should allow each probe to specify the initial delay.

Solution patch:
diff --git a/prober/prober.go b/prober/prober.go
index 0273d344..d5346688 100644
--- a/prober/prober.go
+++ b/prober/prober.go
@@ -139,22 +139,33 @@ func (pr *Prober) startProbe(name string) {
 	go pr.Probes[name].Start(probeCtx, pr.dataChan)
 }
 
-func randomDuration(duration, ceiling time.Duration) time.Duration {
+func randomDuration(duration time.Duration) time.Duration {
 	if duration == 0 {
 		return 0
 	}
-	if duration > ceiling {
-		duration = ceiling
+	if duration > time.Minute {
+		duration = time.Minute
 	}
 	return time.Duration(randGenerator.Int63n(duration.Milliseconds())) * time.Millisecond
 }
 
-// interProbeWait returns the wait time between probes. It's not beneficial for
-// this interval to be too large, so we cap it at 2 seconds.
-func interProbeWait(interval time.Duration, numProbes int) time.Duration {
+// interProbeGap returns the wait time between probes that are in the same
+// interval bucket.
+//
+// Overall goal is to start all probes by the end of 1st interval. But to avoid
+// waiting for too long for large intervals, we cap the gap at 1 minute.
+// Some examples:
+//
+//	30 probes in 30s interval, gap=1s, last probe starts at 29s
+//	12 probes in 1m interval, gap=5s, last probe starts at 55s
+//	12 probes in 5m interval, gap=25s, last probe starts at 4m35s
+//	8 probes in 10m interval, gap=1m, last probe starts at 7m
+//
+// TODO(manugarg): Allow this to be overridden with config.
+func interProbeGap(interval time.Duration, numProbes int) time.Duration {
 	d := interval / time.Duration(numProbes)
-	if d > 2*time.Second {
-		return 2 * time.Second
+	if d > 1*time.Minute {
+		return 1 * time.Minute
 	}
 	return d
 }
@@ -181,18 +192,23 @@ func (pr *Prober) startProbesWithJitter() {
 	}
 
 	iter := 0
+
 	for interval, probeInfos := range intervalBuckets {
 		// Note that we introduce jitter within the goroutine instead of in the
 		// loop here. This is to make sure this function returns quickly.
 		go func(interval time.Duration, probeInfos []*probes.ProbeInfo, iter int) {
 			if iter > 0 {
-				time.Sleep(randomDuration(interval, 10*time.Second))
+				// Except for the first interval bucket, we sleep before
+				// starting probes in this bucket. This sleep is for a random
+				// duration <= min(interval, 1min). Note that unlike inter-probe
+				// gap this one doesn't add up.
+				time.Sleep(randomDuration(interval))
 			}
 
 			for _, p := range probeInfos {
 				pr.l.Info("Starting probe: ", p.Name)
 				go pr.startProbe(p.Name)
-				time.Sleep(interProbeWait(interval, len(probeInfos)))
+				time.Sleep(interProbeGap(interval, len(probeInfos)))
 			}
 		}(interval, probeInfos, iter)
 		iter++



Test patch:
diff --git a/prober/prober_test.go b/prober/prober_test.go
index 3ef45844..79cf4599 100644
--- a/prober/prober_test.go
+++ b/prober/prober_test.go
@@ -33,27 +33,33 @@ import (
 
 func TestRandomDuration(t *testing.T) {
 	tests := []struct {
-		duration time.Duration
-		ceiling  time.Duration
+		duration        time.Duration
+		wantMaxDuration time.Duration
 	}{
 		{
-			duration: 0,
-			ceiling:  10 * time.Second,
+			duration:        0,
+			wantMaxDuration: 0,
 		},
 		{
-			duration: 5 * time.Second,
-			ceiling:  10 * time.Second,
+			duration:        5 * time.Second,
+			wantMaxDuration: 5 * time.Second,
 		},
 		{
-			duration: 30 * time.Second,
-			ceiling:  10 * time.Second,
+			duration:        30 * time.Second,
+			wantMaxDuration: 30 * time.Second,
+		},
+		{
+			duration:        10 * time.Minute,
+			wantMaxDuration: 1 * time.Minute,
+		},
+		{
+			duration:        1 * time.Hour,
+			wantMaxDuration: 1 * time.Minute,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%v", tt), func(t *testing.T) {
-			got := randomDuration(tt.duration, tt.ceiling)
-			assert.LessOrEqual(t, got, tt.duration)
-			assert.LessOrEqual(t, got, tt.ceiling)
+			assert.LessOrEqual(t, randomDuration(tt.duration), tt.wantMaxDuration)
 		})
 	}
 }
@@ -62,22 +68,42 @@ func TestInterProbeWait(t *testing.T) {
 	tests := []struct {
 		interval  time.Duration
 		numProbes int
-		want      time.Duration
+		wantGap   time.Duration
 	}{
 		{
-			interval:  2 * time.Second,
-			numProbes: 16,
-			want:      125 * time.Millisecond,
+			interval:  5 * time.Second,
+			numProbes: 20,
+			wantGap:   250 * time.Millisecond, // Last at 4.75s
 		},
 		{
 			interval:  30 * time.Second,
 			numProbes: 12,
-			want:      2 * time.Second,
+			wantGap:   2500 * time.Millisecond, // Last at 29.5s
+		},
+		{
+			interval:  5 * time.Minute,
+			numProbes: 4,
+			wantGap:   1 * time.Minute, // Last at 3m
+		},
+		{
+			interval:  10 * time.Minute,
+			numProbes: 6,
+			wantGap:   1 * time.Minute, // Last at 5m
+		},
+		{
+			interval:  1 * time.Hour,
+			numProbes: 12,
+			wantGap:   1 * time.Minute, // Last at 11m
+		},
+		{
+			interval:  24 * time.Hour,
+			numProbes: 3,
+			wantGap:   1 * time.Minute,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%s:%d", tt.interval, tt.numProbes), func(t *testing.T) {
-			assert.Equal(t, tt.want, interProbeWait(tt.interval, tt.numProbes))
+			assert.Equal(t, tt.wantGap, interProbeGap(tt.interval, tt.numProbes))
 		})
 	}
 }