Advisor: Make evaluation interval and max history configurable (#100534)

apps/advisor/pkg/app/app.go

@@ -19,10 +19,11 @@ import (
 
 func New(cfg app.Config) (app.App, error) {
 	// Read config
-	checkRegistry, ok := cfg.SpecificConfig.(checkregistry.CheckService)
+	specificConfig, ok := cfg.SpecificConfig.(checkregistry.AdvisorAppConfig)
 	if !ok {
 		return nil, fmt.Errorf("invalid config type")
 	}
+	checkRegistry := specificConfig.CheckRegistry
 
 	// Prepare storage client
 	clientGenerator := k8s.NewClientRegistry(cfg.KubeConfig, k8s.ClientConfig{})

apps/advisor/pkg/app/checkregistry/checkregistry.go

@@ -57,3 +57,9 @@ func (s *Service) Checks() []checks.Check {
 		),
 	}
 }
+
+// AdvisorAppConfig is the configuration received from Grafana to run the app
+type AdvisorAppConfig struct {
+	CheckRegistry CheckService
+	PluginConfig  map[string]string
+}

apps/advisor/pkg/app/checkscheduler/checkscheduler.go

@@ -4,11 +4,13 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strconv"
 	"time"
 
 	"github.com/grafana/grafana-app-sdk/app"
 	"github.com/grafana/grafana-app-sdk/k8s"
 	"github.com/grafana/grafana-app-sdk/resource"
+	"github.com/grafana/grafana-plugin-sdk-go/backend/gtime"
 	advisorv0alpha1 "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1"
 	"github.com/grafana/grafana/apps/advisor/pkg/app/checkregistry"
 	"github.com/grafana/grafana/apps/advisor/pkg/app/checks"
@@ -16,24 +18,35 @@ import (
 	"k8s.io/klog/v2"
 )
 
-const evaluateChecksInterval = 24 * time.Hour
+const defaultEvaluationInterval = 24 * time.Hour
-const maxChecks = 10
+const defaultMaxHistory = 10
 
 // Runner is a "runnable" app used to be able to expose and API endpoint
 // with the existing checks types. This does not need to be a CRUD resource, but it is
 // the only way existing at the moment to expose the check types.
 type Runner struct {
-	checkRegistry checkregistry.CheckService
+	checkRegistry      checkregistry.CheckService
-	client        resource.Client
+	client             resource.Client
+	evaluationInterval time.Duration
+	maxHistory         int
 }
 
 // NewRunner creates a new Runner.
 func New(cfg app.Config) (app.Runnable, error) {
 	// Read config
-	checkRegistry, ok := cfg.SpecificConfig.(checkregistry.CheckService)
+	specificConfig, ok := cfg.SpecificConfig.(checkregistry.AdvisorAppConfig)
 	if !ok {
 		return nil, fmt.Errorf("invalid config type")
 	}
+	checkRegistry := specificConfig.CheckRegistry
+	evalInterval, err := getEvaluationInterval(specificConfig.PluginConfig)
+	if err != nil {
+		return nil, err
+	}
+	maxHistory, err := getMaxHistory(specificConfig.PluginConfig)
+	if err != nil {
+		return nil, err
+	}
 
 	// Prepare storage client
 	clientGenerator := k8s.NewClientRegistry(cfg.KubeConfig, k8s.ClientConfig{})
@@ -43,8 +56,10 @@ func New(cfg app.Config) (app.Runnable, error) {
 	}
 
 	return &Runner{
-		checkRegistry: checkRegistry,
+		checkRegistry:      checkRegistry,
-		client:        client,
+		client:             client,
+		evaluationInterval: evalInterval,
+		maxHistory:         maxHistory,
 	}, nil
 }
 
@@ -64,7 +79,7 @@ func (r *Runner) Run(ctx context.Context) error {
 		}
 	}
 
-	nextSendInterval := time.Until(lastCreated.Add(evaluateChecksInterval))
+	nextSendInterval := time.Until(lastCreated.Add(r.evaluationInterval))
 	if nextSendInterval < time.Minute {
 		nextSendInterval = 1 * time.Minute
 	}
@@ -85,8 +100,8 @@ func (r *Runner) Run(ctx context.Context) error {
 				klog.Error("Error cleaning up old check reports", "error", err)
 			}
 
-			if nextSendInterval != evaluateChecksInterval {
+			if nextSendInterval != r.evaluationInterval {
-				nextSendInterval = evaluateChecksInterval
+				nextSendInterval = r.evaluationInterval
 			}
 			ticker.Reset(nextSendInterval)
 		case <-ctx.Done():
@@ -155,7 +170,7 @@ func (r *Runner) cleanupChecks(ctx context.Context) error {
 	}
 
 	for _, checks := range checksByType {
-		if len(checks) > maxChecks {
+		if len(checks) > r.maxHistory {
 			// Sort checks by creation time
 			sort.Slice(checks, func(i, j int) bool {
 				ti := checks[i].GetCreationTimestamp().Time
@@ -163,7 +178,7 @@ func (r *Runner) cleanupChecks(ctx context.Context) error {
 				return ti.Before(tj)
 			})
 			// Delete the oldest checks
-			for i := 0; i < len(checks)-maxChecks; i++ {
+			for i := 0; i < len(checks)-r.maxHistory; i++ {
 				check := checks[i]
 				id := check.GetStaticMetadata().Identifier()
 				err := r.client.Delete(ctx, id, resource.DeleteOptions{})
@@ -176,3 +191,29 @@ func (r *Runner) cleanupChecks(ctx context.Context) error {
 
 	return nil
 }
+
+func getEvaluationInterval(pluginConfig map[string]string) (time.Duration, error) {
+	evaluationInterval := defaultEvaluationInterval
+	configEvaluationInterval, ok := pluginConfig["evaluation_interval"]
+	if ok {
+		var err error
+		evaluationInterval, err = gtime.ParseDuration(configEvaluationInterval)
+		if err != nil {
+			return 0, fmt.Errorf("invalid evaluation interval: %w", err)
+		}
+	}
+	return evaluationInterval, nil
+}
+
+func getMaxHistory(pluginConfig map[string]string) (int, error) {
+	maxHistory := defaultMaxHistory
+	configMaxHistory, ok := pluginConfig["max_history"]
+	if ok {
+		var err error
+		maxHistory, err = strconv.Atoi(configMaxHistory)
+		if err != nil {
+			return 0, fmt.Errorf("invalid max history: %w", err)
+		}
+	}
+	return maxHistory, nil
+}

apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go

@@ -135,8 +135,8 @@ func TestRunner_cleanupChecks_WithinMax(t *testing.T) {
 func TestRunner_cleanupChecks_ErrorOnDelete(t *testing.T) {
 	mockClient := &MockClient{
 		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
-			items := make([]advisorv0alpha1.Check, 0, maxChecks+1)
+			items := make([]advisorv0alpha1.Check, 0, defaultMaxHistory+1)
-			for i := 0; i < maxChecks+1; i++ {
+			for i := 0; i < defaultMaxHistory+1; i++ {
 				item := advisorv0alpha1.Check{}
 				item.ObjectMeta.SetLabels(map[string]string{
 					checks.TypeLabel: "mock",
@@ -153,7 +153,8 @@ func TestRunner_cleanupChecks_ErrorOnDelete(t *testing.T) {
 	}
 
 	runner := &Runner{
-		client: mockClient,
+		client:     mockClient,
+		maxHistory: defaultMaxHistory,
 	}
 	err := runner.cleanupChecks(context.Background())
 	assert.ErrorContains(t, err, "delete error")
@@ -161,8 +162,8 @@ func TestRunner_cleanupChecks_ErrorOnDelete(t *testing.T) {
 
 func TestRunner_cleanupChecks_Success(t *testing.T) {
 	itemsDeleted := []string{}
-	items := make([]advisorv0alpha1.Check, 0, maxChecks+1)
+	items := make([]advisorv0alpha1.Check, 0, defaultMaxHistory+1)
-	for i := 0; i < maxChecks+1; i++ {
+	for i := 0; i < defaultMaxHistory+1; i++ {
 		item := advisorv0alpha1.Check{}
 		item.ObjectMeta.SetName(fmt.Sprintf("check-%d", i))
 		item.ObjectMeta.SetLabels(map[string]string{
@@ -187,13 +188,54 @@ func TestRunner_cleanupChecks_Success(t *testing.T) {
 	}
 
 	runner := &Runner{
-		client: mockClient,
+		client:     mockClient,
+		maxHistory: defaultMaxHistory,
 	}
 	err := runner.cleanupChecks(context.Background())
 	assert.NoError(t, err)
 	assert.Equal(t, []string{"check-0"}, itemsDeleted)
 }
 
+func Test_getEvaluationInterval(t *testing.T) {
+	t.Run("default", func(t *testing.T) {
+		interval, err := getEvaluationInterval(map[string]string{})
+		assert.NoError(t, err)
+		assert.Equal(t, 24*time.Hour, interval)
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		interval, err := getEvaluationInterval(map[string]string{"evaluation_interval": "invalid"})
+		assert.Error(t, err)
+		assert.Zero(t, interval)
+	})
+
+	t.Run("custom", func(t *testing.T) {
+		interval, err := getEvaluationInterval(map[string]string{"evaluation_interval": "1h"})
+		assert.NoError(t, err)
+		assert.Equal(t, time.Hour, interval)
+	})
+}
+
+func Test_getMaxHistory(t *testing.T) {
+	t.Run("default", func(t *testing.T) {
+		history, err := getMaxHistory(map[string]string{})
+		assert.NoError(t, err)
+		assert.Equal(t, 10, history)
+	})
+
+	t.Run("invalid", func(t *testing.T) {
+		history, err := getMaxHistory(map[string]string{"max_history": "invalid"})
+		assert.Error(t, err)
+		assert.Zero(t, history)
+	})
+
+	t.Run("custom", func(t *testing.T) {
+		history, err := getMaxHistory(map[string]string{"max_history": "5"})
+		assert.NoError(t, err)
+		assert.Equal(t, 5, history)
+	})
+}
+
 type MockCheckService struct {
 	checks []checks.Check
 }

apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer.go

@@ -24,10 +24,11 @@ type Runner struct {
 // NewRunner creates a new Runner.
 func New(cfg app.Config) (app.Runnable, error) {
 	// Read config
-	checkRegistry, ok := cfg.SpecificConfig.(checkregistry.CheckService)
+	specificConfig, ok := cfg.SpecificConfig.(checkregistry.AdvisorAppConfig)
 	if !ok {
 		return nil, fmt.Errorf("invalid config type")
 	}
+	checkRegistry := specificConfig.CheckRegistry
 
 	// Prepare storage client
 	clientGenerator := k8s.NewClientRegistry(cfg.KubeConfig, k8s.ClientConfig{})

pkg/registry/apps/advisor/register.go

@@ -8,6 +8,7 @@ import (
 	advisorapp "github.com/grafana/grafana/apps/advisor/pkg/app"
 	"github.com/grafana/grafana/apps/advisor/pkg/app/checkregistry"
 	"github.com/grafana/grafana/pkg/services/apiserver/builder/runner"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 type AdvisorAppProvider struct {
@@ -16,13 +17,19 @@ type AdvisorAppProvider struct {
 
 func RegisterApp(
 	checkRegistry checkregistry.CheckService,
+	cfg *setting.Cfg,
 ) *AdvisorAppProvider {
 	provider := &AdvisorAppProvider{}
+	pluginConfig := cfg.PluginSettings["grafana-advisor-app"]
+	specificConfig := checkregistry.AdvisorAppConfig{
+		CheckRegistry: checkRegistry,
+		PluginConfig:  pluginConfig,
+	}
 	appCfg := &runner.AppBuilderConfig{
 		OpenAPIDefGetter: advisorv0alpha1.GetOpenAPIDefinitions,
 		ManagedKinds:     advisorapp.GetKinds(),
 		Authorizer:       advisorapp.GetAuthorizer(),
-		CustomConfig:     any(checkRegistry),
+		CustomConfig:     any(specificConfig),
 	}
 	provider.Provider = simple.NewAppProvider(apis.LocalManifest(), appCfg, advisorapp.New)
 	return provider