Alerting: Refactor annotation historian to isolate dashboard service dependency (#71689)

* Refactor annotation historian to isolate dashboard service dependency

* Export PanelKey

* Don't export parsePanelKey

* Remove commented out code

pkg/services/ngalert/ngalert.go

@@ -420,7 +420,8 @@ func configureHistorianBackend(ctx context.Context, cfg setting.UnifiedAlertingS
 		return historian.NewMultipleBackend(primary, secondaries...), nil
 	}
 	if backend == historian.BackendTypeAnnotations {
-		return historian.NewAnnotationBackend(ar, ds, rs, met), nil
+		store := historian.NewAnnotationStore(ar, ds, met)
+		return historian.NewAnnotationBackend(store, rs, met), nil
 	}
 	if backend == historian.BackendTypeLoki {
 		lcfg, err := historian.NewLokiConfig(cfg)

pkg/services/ngalert/state/historian/annotation.go

@@ -16,7 +16,6 @@ import (
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/services/annotations"
-	"github.com/grafana/grafana/pkg/services/dashboards"
 	"github.com/grafana/grafana/pkg/services/ngalert/eval"
 	"github.com/grafana/grafana/pkg/services/ngalert/metrics"
 	ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
@@ -26,12 +25,11 @@ import (
 
 // AnnotationBackend is an implementation of state.Historian that uses Grafana Annotations as the backing datastore.
 type AnnotationBackend struct {
-	annotations AnnotationStore
+	store   AnnotationStore
-	dashboards  *dashboardResolver
+	rules   RuleStore
-	rules       RuleStore
+	clock   clock.Clock
-	clock       clock.Clock
+	metrics *metrics.Historian
-	metrics     *metrics.Historian
+	log     log.Logger
-	log         log.Logger
 }
 
 type RuleStore interface {
@@ -40,18 +38,17 @@ type RuleStore interface {
 
 type AnnotationStore interface {
 	Find(ctx context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error)
-	SaveMany(ctx context.Context, items []annotations.Item) error
+	Save(ctx context.Context, panel *PanelKey, annotations []annotations.Item, orgID int64, logger log.Logger) error
 }
 
-func NewAnnotationBackend(annotations AnnotationStore, dashboards dashboards.DashboardService, rules RuleStore, metrics *metrics.Historian) *AnnotationBackend {
+func NewAnnotationBackend(annotations AnnotationStore, rules RuleStore, metrics *metrics.Historian) *AnnotationBackend {
 	logger := log.New("ngalert.state.historian", "backend", "annotations")
 	return &AnnotationBackend{
-		annotations: annotations,
+		store:   annotations,
-		dashboards:  newDashboardResolver(dashboards, defaultDashboardCacheExpiry),
+		rules:   rules,
-		rules:       rules,
+		clock:   clock.New(),
-		clock:       clock.New(),
+		metrics: metrics,
-		metrics:     metrics,
+		log:     logger,
-		log:         logger,
 	}
 }
 
@@ -83,7 +80,7 @@ func (h *AnnotationBackend) Record(ctx context.Context, rule history_model.RuleM
 		defer close(errCh)
 		logger := h.log.FromContext(ctx)
 
-		errCh <- h.recordAnnotations(ctx, panel, annotations, rule.OrgID, logger)
+		errCh <- h.store.Save(ctx, panel, annotations, rule.OrgID, logger)
 	}(writeCtx)
 	return errCh
 }
@@ -118,7 +115,7 @@ func (h *AnnotationBackend) Query(ctx context.Context, query ngmodels.HistoryQue
 		To:           query.To.Unix(),
 		SignedInUser: query.SignedInUser,
 	}
-	items, err := h.annotations.Find(ctx, &q)
+	items, err := h.store.Find(ctx, &q)
 	if err != nil {
 		return nil, fmt.Errorf("failed to query annotations for state history: %w", err)
 	}
@@ -198,34 +195,6 @@ func buildAnnotations(rule history_model.RuleMeta, states []state.StateTransitio
 	return items
 }
 
-func (h *AnnotationBackend) recordAnnotations(ctx context.Context, panel *panelKey, annotations []annotations.Item, orgID int64, logger log.Logger) error {
-	if panel != nil {
-		dashID, err := h.dashboards.getID(ctx, panel.orgID, panel.dashUID)
-		if err != nil {
-			logger.Error("Error getting dashboard for alert annotation", "dashboardUID", panel.dashUID, "error", err)
-			dashID = 0
-		}
-
-		for i := range annotations {
-			annotations[i].DashboardID = dashID
-			annotations[i].PanelID = panel.panelID
-		}
-	}
-
-	org := fmt.Sprint(orgID)
-	h.metrics.WritesTotal.WithLabelValues(org, "annotations").Inc()
-	h.metrics.TransitionsTotal.WithLabelValues(org).Add(float64(len(annotations)))
-	if err := h.annotations.SaveMany(ctx, annotations); err != nil {
-		logger.Error("Error saving alert annotation batch", "error", err)
-		h.metrics.WritesFailed.WithLabelValues(org, "annotations").Inc()
-		h.metrics.TransitionsFailed.WithLabelValues(org).Add(float64(len(annotations)))
-		return fmt.Errorf("error saving alert annotation batch: %w", err)
-	}
-
-	logger.Debug("Done saving alert annotation batch")
-	return nil
-}
-
 func buildAnnotationTextAndData(rule history_model.RuleMeta, currentState *state.State) (string, *simplejson.Json) {
 	jsonData := simplejson.New()
 	var value string

pkg/services/ngalert/state/historian/annotation_store.go

@@ -0,0 +1,62 @@
+package historian
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/services/annotations"
+	"github.com/grafana/grafana/pkg/services/dashboards"
+	"github.com/grafana/grafana/pkg/services/ngalert/metrics"
+)
+
+type AnnotationService interface {
+	Find(ctx context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error)
+	SaveMany(ctx context.Context, items []annotations.Item) error
+}
+
+type AnnotationServiceStore struct {
+	svc        AnnotationService
+	dashboards *dashboardResolver
+	metrics    *metrics.Historian
+}
+
+func NewAnnotationStore(svc AnnotationService, dashboards dashboards.DashboardService, metrics *metrics.Historian) *AnnotationServiceStore {
+	return &AnnotationServiceStore{
+		svc:        svc,
+		dashboards: newDashboardResolver(dashboards, defaultDashboardCacheExpiry),
+		metrics:    metrics,
+	}
+}
+
+func (s *AnnotationServiceStore) Save(ctx context.Context, panel *PanelKey, annotations []annotations.Item, orgID int64, logger log.Logger) error {
+	if panel != nil {
+		dashID, err := s.dashboards.getID(ctx, panel.orgID, panel.dashUID)
+		if err != nil {
+			logger.Error("Error getting dashboard for alert annotation", "dashboardUID", panel.dashUID, "error", err)
+			dashID = 0
+		}
+
+		for i := range annotations {
+			annotations[i].DashboardID = dashID
+			annotations[i].PanelID = panel.panelID
+		}
+	}
+
+	org := fmt.Sprint(orgID)
+	s.metrics.WritesTotal.WithLabelValues(org, "annotations").Inc()
+	s.metrics.TransitionsTotal.WithLabelValues(org).Add(float64(len(annotations)))
+	if err := s.svc.SaveMany(ctx, annotations); err != nil {
+		logger.Error("Error saving alert annotation batch", "error", err)
+		s.metrics.WritesFailed.WithLabelValues(org, "annotations").Inc()
+		s.metrics.TransitionsFailed.WithLabelValues(org).Add(float64(len(annotations)))
+		return fmt.Errorf("error saving alert annotation batch: %w", err)
+	}
+
+	logger.Debug("Done saving alert annotation batch")
+	return nil
+}
+
+func (s *AnnotationServiceStore) Find(ctx context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) {
+	return s.svc.Find(ctx, query)
+}

pkg/services/ngalert/state/historian/annotation_test.go

@@ -31,7 +31,7 @@ func TestAnnotationHistorian(t *testing.T) {
 	t.Run("alert annotations are queryable", func(t *testing.T) {
 		anns := createTestAnnotationBackendSut(t)
 		items := []annotations.Item{createAnnotation()}
-		require.NoError(t, anns.recordAnnotations(context.Background(), nil, items, 1, log.NewNopLogger()))
+		require.NoError(t, anns.store.Save(context.Background(), nil, items, 1, log.NewNopLogger()))
 
 		q := models.HistoryQuery{
 			RuleUID: "my-rule",
@@ -113,7 +113,8 @@ func createTestAnnotationBackendSutWithMetrics(t *testing.T, met *metrics.Histor
 	}
 	dbs := &dashboards.FakeDashboardService{}
 	dbs.On("GetDashboard", mock.Anything, mock.Anything).Return(&dashboards.Dashboard{}, nil)
-	return NewAnnotationBackend(fakeAnnoRepo, dbs, rules, met)
+	store := NewAnnotationStore(fakeAnnoRepo, dbs, met)
+	return NewAnnotationBackend(store, rules, met)
 }
 
 func createFailingAnnotationSut(t *testing.T, met *metrics.Historian) *AnnotationBackend {
@@ -124,7 +125,8 @@ func createFailingAnnotationSut(t *testing.T, met *metrics.Historian) *Annotatio
 	}
 	dbs := &dashboards.FakeDashboardService{}
 	dbs.On("GetDashboard", mock.Anything, mock.Anything).Return(&dashboards.Dashboard{}, nil)
-	return NewAnnotationBackend(fakeAnnoRepo, dbs, rules, met)
+	store := NewAnnotationStore(fakeAnnoRepo, dbs, met)
+	return NewAnnotationBackend(store, rules, met)
 }
 
 func createAnnotation() annotations.Item {

pkg/services/ngalert/state/historian/core.go

@@ -50,17 +50,17 @@ func labelFingerprint(labels data.Labels) string {
 	return fmt.Sprintf("%016x", sig)
 }
 
-// panelKey uniquely identifies a panel.
+// PanelKey uniquely identifies a panel.
-type panelKey struct {
+type PanelKey struct {
 	orgID   int64
 	dashUID string
 	panelID int64
 }
 
-// panelKey attempts to get the key of the panel attached to the given rule. Returns nil if the rule is not attached to a panel.
+// PanelKey attempts to get the key of the panel attached to the given rule. Returns nil if the rule is not attached to a panel.
-func parsePanelKey(rule history_model.RuleMeta, logger log.Logger) *panelKey {
+func parsePanelKey(rule history_model.RuleMeta, logger log.Logger) *PanelKey {
 	if rule.DashboardUID != "" {
-		return &panelKey{
+		return &PanelKey{
 			orgID:   rule.OrgID,
 			dashUID: rule.DashboardUID,
 			panelID: rule.PanelID,

pkg/services/ngalert/state/manager_bench_test.go

@@ -20,7 +20,8 @@ func BenchmarkProcessEvalResults(b *testing.B) {
 	as := annotations.FakeAnnotationsRepo{}
 	as.On("SaveMany", mock.Anything, mock.Anything).Return(nil)
 	metrics := metrics.NewHistorianMetrics(prometheus.NewRegistry())
-	hist := historian.NewAnnotationBackend(&as, nil, nil, metrics)
+	store := historian.NewAnnotationStore(&as, nil, metrics)
+	hist := historian.NewAnnotationBackend(store, nil, metrics)
 	cfg := state.ManagerCfg{
 		Historian:               hist,
 		MaxStateSaveConcurrency: 1,

pkg/services/ngalert/state/manager_test.go

@@ -226,7 +226,8 @@ func TestDashboardAnnotations(t *testing.T) {
 
 	fakeAnnoRepo := annotationstest.NewFakeAnnotationsRepo()
 	metrics := metrics.NewHistorianMetrics(prometheus.NewRegistry())
-	hist := historian.NewAnnotationBackend(fakeAnnoRepo, &dashboards.FakeDashboardService{}, nil, metrics)
+	store := historian.NewAnnotationStore(fakeAnnoRepo, &dashboards.FakeDashboardService{}, metrics)
+	hist := historian.NewAnnotationBackend(store, nil, metrics)
 	cfg := state.ManagerCfg{
 		Metrics:                 testMetrics.GetStateMetrics(),
 		ExternalURL:             nil,
@@ -2256,7 +2257,8 @@ func TestProcessEvalResults(t *testing.T) {
 	for _, tc := range testCases {
 		fakeAnnoRepo := annotationstest.NewFakeAnnotationsRepo()
 		metrics := metrics.NewHistorianMetrics(prometheus.NewRegistry())
-		hist := historian.NewAnnotationBackend(fakeAnnoRepo, &dashboards.FakeDashboardService{}, nil, metrics)
+		store := historian.NewAnnotationStore(fakeAnnoRepo, &dashboards.FakeDashboardService{}, metrics)
+		hist := historian.NewAnnotationBackend(store, nil, metrics)
 		cfg := state.ManagerCfg{
 			Metrics:                 testMetrics.GetStateMetrics(),
 			ExternalURL:             nil,