Alerting: Cache result of dashboard ID lookups (#56587)

* Create caching dashboard resolver

* A couple tests for dashboard resolving

* Log warning on not found

* Additional polish + review nits

* Move to singleflight instead of a plain mutex

* Store errors instead of -1 in cache and use reflection when reading

* Address linter error

* One more linter error

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

@@ -9,7 +9,6 @@ import (
 
 	"github.com/grafana/grafana-plugin-sdk-go/data"
 	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/annotations"
 	"github.com/grafana/grafana/pkg/services/dashboards"
 	ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
@@ -19,14 +18,14 @@ import (
 // AnnotationStateHistorian is an implementation of state.Historian that uses Grafana Annotations as the backing datastore.
 type AnnotationStateHistorian struct {
 	annotations annotations.Repository
-	dashboards  dashboards.DashboardService
+	dashboards  *dashboardResolver
 	log         log.Logger
 }
 
 func NewAnnotationHistorian(annotations annotations.Repository, dashboards dashboards.DashboardService, log log.Logger) *AnnotationStateHistorian {
 	return &AnnotationStateHistorian{
 		annotations: annotations,
-		dashboards:  dashboards,
+		dashboards:  newDashboardResolver(dashboards, log, defaultDashboardCacheExpiry),
 		log:         log,
 	}
 }
@@ -56,19 +55,14 @@ func (h *AnnotationStateHistorian) RecordState(ctx context.Context, rule *ngmode
 			return
 		}
 
-		query := &models.GetDashboardQuery{
-			Uid:   dashUid,
-			OrgId: rule.OrgID,
-		}
-
-		err = h.dashboards.GetDashboard(ctx, query)
+		dashID, err := h.dashboards.getID(ctx, rule.OrgID, dashUid)
 		if err != nil {
 			h.log.Error("error getting dashboard for alert annotation", "dashboardUID", dashUid, "alertRuleUID", rule.UID, "err", err.Error())
 			return
 		}
 
 		item.PanelId = panelId
-		item.DashboardId = query.Result.Id
+		item.DashboardId = dashID
 	}
 
 	if err := h.annotations.Save(ctx, item); err != nil {

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

@@ -0,0 +1,103 @@
+package historian
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"strconv"
+	"time"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/dashboards"
+	"github.com/patrickmn/go-cache"
+	"golang.org/x/sync/singleflight"
+)
+
+const (
+	defaultDashboardCacheExpiry = 1 * time.Minute
+	minCleanupInterval          = 1 * time.Second
+)
+
+// dashboardResolver resolves dashboard UIDs to IDs with caching.
+type dashboardResolver struct {
+	dashboards   dashboards.DashboardService
+	cache        *cache.Cache
+	singleflight singleflight.Group
+	log          log.Logger
+}
+
+func newDashboardResolver(dbs dashboards.DashboardService, log log.Logger, expiry time.Duration) *dashboardResolver {
+	return &dashboardResolver{
+		dashboards:   dbs,
+		cache:        cache.New(expiry, maxDuration(2*expiry, minCleanupInterval)),
+		singleflight: singleflight.Group{},
+		log:          log,
+	}
+}
+
+// getId gets the ID of the dashboard with the given uid/orgID combination, or returns dashboardNotFound if the dashboard does not exist.
+func (r *dashboardResolver) getID(ctx context.Context, orgID int64, uid string) (int64, error) {
+	// Optimistically query without acquiring lock. This is okay because cache.Cache is thread-safe.
+	// We don't need to lock anything ourselves on cache miss, because singleflight will lock for us within a given key.
+	// Different keys which correspond to different queries will never block each other.
+	key := packCacheKey(orgID, uid)
+
+	if id, found := r.cache.Get(key); found {
+		return toQueryResult(id, nil)
+	}
+
+	id, err, _ := r.singleflight.Do(key, func() (interface{}, error) {
+		r.log.Debug("dashboard cache miss, querying dashboards", "dashboardUID", uid)
+
+		var result interface{}
+		query := &models.GetDashboardQuery{
+			Uid:   uid,
+			OrgId: orgID,
+		}
+		err := r.dashboards.GetDashboard(ctx, query)
+		// We also cache lookups where we don't find anything.
+		if err != nil && errors.Is(err, dashboards.ErrDashboardNotFound) {
+			result = err
+		} else if err != nil {
+			return 0, err
+		} else if query.Result == nil {
+			result = dashboards.ErrDashboardNotFound
+		} else {
+			result = query.Result.Id
+		}
+
+		// By setting the cache inside the singleflighted routine, we avoid any accidental re-queries that could get initiated after the query completes.
+		r.cache.Set(key, result, cache.DefaultExpiration)
+		return result, nil
+	})
+
+	return toQueryResult(id, err)
+}
+
+func packCacheKey(orgID int64, uid string) string {
+	const base = 10
+	return strconv.FormatInt(orgID, base) + "-" + uid
+}
+
+func maxDuration(a, b time.Duration) time.Duration {
+	if a >= b {
+		return a
+	}
+	return b
+}
+
+func toQueryResult(cacheVal interface{}, err error) (int64, error) {
+	if err != nil {
+		return 0, err
+	}
+
+	switch cacheVal := cacheVal.(type) {
+	case error:
+		return 0, cacheVal
+	case int64:
+		return cacheVal, err
+	default:
+		panic(fmt.Sprintf("unexpected value stored in cache: %#v", cacheVal))
+	}
+}

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

@@ -0,0 +1,46 @@
+package historian
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/dashboards"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDashboardResolver(t *testing.T) {
+	t.Run("fetches dashboards from dashboard service", func(t *testing.T) {
+		dbs := &dashboards.FakeDashboardService{}
+		exp := int64(14)
+		dbs.On("GetDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+			args.Get(1).(*models.GetDashboardQuery).Result = &models.Dashboard{Id: exp}
+		}).Return(nil)
+		sut := createDashboardResolverSut(dbs)
+
+		id, err := sut.getID(context.Background(), 1, "dashboard-uid")
+
+		require.NoError(t, err)
+		require.Equal(t, exp, id)
+	})
+
+	t.Run("fetches dashboardNotFound if underlying dashboard does not exist", func(t *testing.T) {
+		dbs := &dashboards.FakeDashboardService{}
+		dbs.On("GetDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+			args.Get(1).(*models.GetDashboardQuery).Result = nil
+		}).Return(dashboards.ErrDashboardNotFound)
+		sut := createDashboardResolverSut(dbs)
+
+		_, err := sut.getID(context.Background(), 1, "not-exist")
+
+		require.Error(t, err)
+		require.ErrorIs(t, err, dashboards.ErrDashboardNotFound)
+	})
+}
+
+func createDashboardResolverSut(dbs *dashboards.FakeDashboardService) *dashboardResolver {
+	return newDashboardResolver(dbs, log.NewNopLogger(), 1*time.Nanosecond)
+}