From 4ef98449fff08a82cf3390020ee06b9b48a93712 Mon Sep 17 00:00:00 2001
From: Giuseppe Guerra <giuseppe.guerra@grafana.com>
Date: Mon, 28 Aug 2023 11:34:05 +0200
Subject: [PATCH] Plugins: Dynamic angular patterns: Add random offset to GCOM
 API calls, handle HTTP errors (#73494)

* Plugins: Dynamic angular patterns: Return error for != 2xx status code

* Add test for status code check

* Plugins: Dynamic angular patterns: Add random skew to periocic GCOM api calls

* Add test for random skew

* Changed randomSkew signature, ensure it is always positive
---
 .../angulardetectorsprovider/dynamic.go       | 22 ++++++++++-
 .../angulardetectorsprovider/dynamic_test.go  | 39 ++++++++++++++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic.go b/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic.go
index d9e272553e4..969dd6c2380 100644
--- a/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic.go
+++ b/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"math/rand"
 	"net"
 	"net/http"
 	"net/url"
@@ -117,6 +118,9 @@ func (d *Dynamic) fetch(ctx context.Context) (GCOMPatterns, error) {
 			d.log.Error("Response body close error", "error", err)
 		}
 	}()
+	if resp.StatusCode/100 != 2 {
+		return nil, fmt.Errorf("bad status code: %d", resp.StatusCode)
+	}
 	var out GCOMPatterns
 	if err := json.NewDecoder(resp.Body).Decode(&out); err != nil {
 		return nil, fmt.Errorf("json decode: %w", err)
@@ -184,6 +188,12 @@ func (d *Dynamic) IsDisabled() bool {
 	return !d.features.IsEnabled(featuremgmt.FlagPluginsDynamicAngularDetectionPatterns)
 }
 
+// randomSkew returns a random time.Duration between 0 and maxSkew.
+// This can be added to backgroundJobInterval to skew it by a random amount.
+func (d *Dynamic) randomSkew(maxSkew time.Duration) time.Duration {
+	return time.Duration(rand.Float64() * float64(maxSkew))
+}
+
 // Run is the function implementing the background service and updates the detectors periodically.
 func (d *Dynamic) Run(ctx context.Context) error {
 	d.log.Debug("Started background service")
@@ -193,8 +203,18 @@ func (d *Dynamic) Run(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("get last updated: %w", err)
 	}
-	nextRunUntil := time.Until(lastUpdate.Add(backgroundJobInterval))
 
+	// Offset the background job interval a bit to skew GCOM calls from all instances,
+	// so GCOM is not overwhelmed with lots of requests all at the same time.
+	// Important when lots of HG instances restart at the same time.
+	skew := d.randomSkew(backgroundJobInterval / 4)
+	backgroundJobInterval += skew
+	d.log.Debug(
+		"Applied background job skew",
+		"skew", backgroundJobInterval, "interval", backgroundJobInterval,
+	)
+
+	nextRunUntil := time.Until(lastUpdate.Add(backgroundJobInterval))
 	ticker := time.NewTicker(backgroundJobInterval)
 	defer ticker.Stop()
 
diff --git a/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic_test.go b/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic_test.go
index 0179c292553..fef12455270 100644
--- a/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic_test.go
+++ b/pkg/services/pluginsintegration/angulardetectorsprovider/dynamic_test.go
@@ -108,6 +108,20 @@ func TestDynamicAngularDetectorsProvider(t *testing.T) {
 			require.False(t, gcom.httpCalls.called(), "gcom api should not be called")
 			require.Empty(t, svc.ProvideDetectors(context.Background()))
 		})
+
+		t.Run("returns error if status code is outside 2xx range", func(t *testing.T) {
+			errScenario := &gcomScenario{httpHandlerFunc: func(w http.ResponseWriter, req *http.Request) {
+				// Return a valid json response so json.Unmarshal succeeds
+				// but still return 500 status code
+				w.WriteHeader(http.StatusInternalServerError)
+				_, _ = w.Write([]byte("[]"))
+			}}
+			errSrv := errScenario.newHTTPTestServer()
+			t.Cleanup(errSrv.Close)
+			svc := provideDynamic(t, errSrv.URL)
+			_, err := svc.fetch(context.Background())
+			require.Error(t, err)
+		})
 	})
 
 	t.Run("updateDetectors", func(t *testing.T) {
@@ -283,7 +297,7 @@ func TestDynamicAngularDetectorsProviderBackgroundService(t *testing.T) {
 			done := make(chan struct{})
 			gcom := newDefaultGCOMScenario(func(_ http.ResponseWriter, _ *http.Request) {
 				now := time.Now()
-				assert.WithinDuration(t, now, lastJobTime, jobInterval+jobInterval/2)
+				assert.WithinDuration(t, now, lastJobTime, jobInterval*2)
 				lastJobTime = now
 
 				jobCalls.inc()
@@ -316,6 +330,29 @@ func TestDynamicAngularDetectorsProviderBackgroundService(t *testing.T) {
 	})
 }
 
+func TestRandomSkew(t *testing.T) {
+	const runs = 100
+
+	gcom := newDefaultGCOMScenario()
+	srv := gcom.newHTTPTestServer()
+	t.Cleanup(srv.Close)
+	svc := provideDynamic(t, srv.URL)
+	const ttl = time.Hour * 1
+	const skew = ttl / 4
+	var different bool
+	var previous time.Duration
+	for i := 0; i < runs; i++ {
+		v := svc.randomSkew(skew)
+		require.True(t, v >= 0 && v <= skew, "returned skew must be within ttl and +ttl/4")
+		if i == 0 {
+			previous = v
+		} else if !different {
+			different = float64(previous) != float64(v)
+		}
+	}
+	require.True(t, different, "must not always return the same value")
+}
+
 var mockGCOMResponse = []byte(`[{
 	"name": "PanelCtrl",
 	"type": "contains",