From 129a28919b807df99d99b0d30ec4bd25a2952ee0 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Fri, 14 Oct 2022 15:48:02 -0500 Subject: [PATCH] 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 --- .../ngalert/state/historian/annotation.go | 14 +-- .../ngalert/state/historian/dashboard.go | 103 ++++++++++++++++++ .../ngalert/state/historian/dashboard_test.go | 46 ++++++++ 3 files changed, 153 insertions(+), 10 deletions(-) create mode 100644 pkg/services/ngalert/state/historian/dashboard.go create mode 100644 pkg/services/ngalert/state/historian/dashboard_test.go diff --git a/pkg/services/ngalert/state/historian/annotation.go b/pkg/services/ngalert/state/historian/annotation.go index 739fa901ce3..c4ff14fae1b 100644 --- a/pkg/services/ngalert/state/historian/annotation.go +++ b/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 { diff --git a/pkg/services/ngalert/state/historian/dashboard.go b/pkg/services/ngalert/state/historian/dashboard.go new file mode 100644 index 00000000000..46ab1c5e6ca --- /dev/null +++ b/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)) + } +} diff --git a/pkg/services/ngalert/state/historian/dashboard_test.go b/pkg/services/ngalert/state/historian/dashboard_test.go new file mode 100644 index 00000000000..0f93980dd7a --- /dev/null +++ b/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) +}