From 9b929ab7cdc70de3de1baf4d71b445d2463f3746 Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:57:11 +0200 Subject: [PATCH] [v10.4.x] Annotations: Optimize search by tags (#93608) Annotations: Optimize search by tags (#93547) * Annotations: Optimize search on large number of dashboards * refactor * fix batch size * Return early if no annotations found * revert go.mod * return nil in case of error * Move default limit to the API package * fix empty access control filter * Set default limit to 100 * optimize query when number of annotations is less than limit * Update pkg/services/annotations/annotationsimpl/annotations.go Co-authored-by: Ieva * remove limit from store since it's set in API * set default limit in Find method (do not break tests) * Only add limit to the query if it's set * use limit trick for all searches without dashboard filter * set default page if not provided --------- Co-authored-by: Ieva (cherry picked from commit 5e713673e139af4df2217b9a89eb472428a4b928) Co-authored-by: Alexander Zobnin --- pkg/api/annotations.go | 5 ++ .../accesscontrol/accesscontrol.go | 51 ++++++++----------- .../accesscontrol/accesscontrol_test.go | 4 +- .../annotations/accesscontrol/models.go | 2 + .../annotationsimpl/annotations.go | 46 +++++++++++++++-- .../annotations/annotationsimpl/xorm_store.go | 16 ++++-- pkg/services/annotations/models.go | 1 + 7 files changed, 85 insertions(+), 40 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index aae4d96ddb5..7a85da8f7fa 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -22,6 +22,8 @@ import ( "github.com/grafana/grafana/pkg/web" ) +const defaultAnnotationsLimit = 100 + // swagger:route GET /annotations annotations getAnnotations // // Find Annotations. @@ -48,6 +50,9 @@ func (hs *HTTPServer) GetAnnotations(c *contextmodel.ReqContext) response.Respon MatchAny: c.QueryBool("matchAny"), SignedInUser: c.SignedInUser, } + if query.Limit == 0 { + query.Limit = defaultAnnotationsLimit + } // When dashboard UID present in the request, we ignore dashboard ID if query.DashboardUID != "" { diff --git a/pkg/services/annotations/accesscontrol/accesscontrol.go b/pkg/services/annotations/accesscontrol/accesscontrol.go index d0d22703414..5f364433b56 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol.go @@ -39,7 +39,7 @@ func NewAuthService(db db.DB, features featuremgmt.FeatureToggles) *AuthService } // Authorize checks if the user has permission to read annotations, then returns a struct containing dashboards and scope types that the user has access to. -func (authz *AuthService) Authorize(ctx context.Context, orgID int64, query *annotations.ItemQuery) (*AccessResources, error) { +func (authz *AuthService) Authorize(ctx context.Context, query *annotations.ItemQuery) (*AccessResources, error) { user := query.SignedInUser if user == nil || user.IsNil() { return nil, ErrReadForbidden.Errorf("missing user") @@ -60,14 +60,14 @@ func (authz *AuthService) Authorize(ctx context.Context, orgID int64, query *ann var err error if canAccessDashAnnotations { if query.AnnotationID != 0 { - annotationDashboardID, err := authz.getAnnotationDashboard(ctx, query, orgID) + annotationDashboardID, err := authz.getAnnotationDashboard(ctx, query) if err != nil { return nil, ErrAccessControlInternal.Errorf("failed to fetch annotations: %w", err) } query.DashboardID = annotationDashboardID } - visibleDashboards, err = authz.dashboardsWithVisibleAnnotations(ctx, query, orgID) + visibleDashboards, err = authz.dashboardsWithVisibleAnnotations(ctx, query) if err != nil { return nil, ErrAccessControlInternal.Errorf("failed to fetch dashboards: %w", err) } @@ -80,7 +80,7 @@ func (authz *AuthService) Authorize(ctx context.Context, orgID int64, query *ann }, nil } -func (authz *AuthService) getAnnotationDashboard(ctx context.Context, query *annotations.ItemQuery, orgID int64) (int64, error) { +func (authz *AuthService) getAnnotationDashboard(ctx context.Context, query *annotations.ItemQuery) (int64, error) { var items []annotations.Item params := make([]any, 0) err := authz.db.WithDbSession(ctx, func(sess *db.Session) error { @@ -92,7 +92,7 @@ func (authz *AuthService) getAnnotationDashboard(ctx context.Context, query *ann FROM annotation as a WHERE a.org_id = ? AND a.id = ? ` - params = append(params, orgID, query.AnnotationID) + params = append(params, query.OrgID, query.AnnotationID) return sess.SQL(sql, params...).Find(&items) }) @@ -106,7 +106,7 @@ func (authz *AuthService) getAnnotationDashboard(ctx context.Context, query *ann return items[0].DashboardID, nil } -func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, query *annotations.ItemQuery, orgID int64) (map[string]int64, error) { +func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, query *annotations.ItemQuery) (map[string]int64, error) { recursiveQueriesSupported, err := authz.db.RecursiveQueriesAreSupported() if err != nil { return nil, err @@ -119,7 +119,7 @@ func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, filters := []any{ permissions.NewAccessControlDashboardPermissionFilter(query.SignedInUser, dashboardaccess.PERMISSION_VIEW, filterType, authz.features, recursiveQueriesSupported), - searchstore.OrgFilter{OrgId: orgID}, + searchstore.OrgFilter{OrgId: query.OrgID}, } if query.DashboardUID != "" { @@ -134,32 +134,25 @@ func (authz *AuthService) dashboardsWithVisibleAnnotations(ctx context.Context, } sb := &searchstore.Builder{Dialect: authz.db.GetDialect(), Filters: filters, Features: authz.features} - - visibleDashboards := make(map[string]int64) - - var page int64 = 1 + // This is a limit for a batch size, not for the end query result. var limit int64 = 1000 - for { - var res []dashboardProjection - sql, params := sb.ToSQL(limit, page) - - err = authz.db.WithDbSession(ctx, func(sess *db.Session) error { - return sess.SQL(sql, params...).Find(&res) - }) - if err != nil { - return nil, err - } + if query.Page == 0 { + query.Page = 1 + } + sql, params := sb.ToSQL(limit, query.Page) - for _, p := range res { - visibleDashboards[p.UID] = p.ID - } + visibleDashboards := make(map[string]int64) + var res []dashboardProjection - // if the result is less than the limit, we have reached the end - if len(res) < int(limit) { - break - } + err = authz.db.WithDbSession(ctx, func(sess *db.Session) error { + return sess.SQL(sql, params...).Find(&res) + }) + if err != nil { + return nil, err + } - page++ + for _, p := range res { + visibleDashboards[p.UID] = p.ID } return visibleDashboards, nil diff --git a/pkg/services/annotations/accesscontrol/accesscontrol_test.go b/pkg/services/annotations/accesscontrol/accesscontrol_test.go index 02674e42b24..aef9fa5c434 100644 --- a/pkg/services/annotations/accesscontrol/accesscontrol_test.go +++ b/pkg/services/annotations/accesscontrol/accesscontrol_test.go @@ -175,8 +175,8 @@ func TestIntegrationAuthorize(t *testing.T) { authz := NewAuthService(sql, featuremgmt.WithFeatures(tc.featureToggle)) - query := &annotations.ItemQuery{SignedInUser: u} - resources, err := authz.Authorize(context.Background(), 1, query) + query := &annotations.ItemQuery{SignedInUser: u, OrgID: 1} + resources, err := authz.Authorize(context.Background(), query) require.NoError(t, err) if tc.expectedResources.Dashboards != nil { diff --git a/pkg/services/annotations/accesscontrol/models.go b/pkg/services/annotations/accesscontrol/models.go index 7b74de929d8..8a2c7ed01e5 100644 --- a/pkg/services/annotations/accesscontrol/models.go +++ b/pkg/services/annotations/accesscontrol/models.go @@ -8,6 +8,8 @@ type AccessResources struct { CanAccessDashAnnotations bool // CanAccessOrgAnnotations true if the user is allowed to access organization annotations CanAccessOrgAnnotations bool + // Skip filtering + SkipAccessControlFilter bool } type dashboardProjection struct { diff --git a/pkg/services/annotations/annotationsimpl/annotations.go b/pkg/services/annotations/annotationsimpl/annotations.go index 3cf88288d27..475a94f1ef2 100644 --- a/pkg/services/annotations/annotationsimpl/annotations.go +++ b/pkg/services/annotations/annotationsimpl/annotations.go @@ -68,12 +68,50 @@ func (r *RepositoryImpl) Update(ctx context.Context, item *annotations.Item) err } func (r *RepositoryImpl) Find(ctx context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { - resources, err := r.authZ.Authorize(ctx, query.OrgID, query) - if err != nil { - return make([]*annotations.ItemDTO, 0), err + if query.Limit == 0 { + query.Limit = 100 } - return r.reader.Get(ctx, query, resources) + // Search without dashboard UID filter is expensive, so check without access control first + if query.DashboardID == 0 && query.DashboardUID == "" { + // Return early if no annotations found, it's not necessary to perform expensive access control filtering + res, err := r.reader.Get(ctx, query, &accesscontrol.AccessResources{ + SkipAccessControlFilter: true, + }) + if err != nil || len(res) == 0 { + return []*annotations.ItemDTO{}, err + } + // If number of resources is less than limit, it makes sense to set query limit to this + // value, otherwise query will be iterating over all user's dashboards since original + // query limit is never reached. + query.Limit = int64(len(res)) + } + + results := make([]*annotations.ItemDTO, 0, query.Limit) + query.Page = 1 + + // Iterate over available annotations until query limit is reached + // or all available dashboards are checked + for len(results) < int(query.Limit) { + resources, err := r.authZ.Authorize(ctx, query) + if err != nil { + return nil, err + } + + res, err := r.reader.Get(ctx, query, resources) + if err != nil { + return nil, err + } + + results = append(results, res...) + query.Page++ + // All user's dashboards are fetched + if len(resources.Dashboards) < int(query.Limit) { + break + } + } + + return results, nil } func (r *RepositoryImpl) Delete(ctx context.Context, params *annotations.DeleteParams) error { diff --git a/pkg/services/annotations/annotationsimpl/xorm_store.go b/pkg/services/annotations/annotationsimpl/xorm_store.go index 6ae9ddc3ce1..77adda9db27 100644 --- a/pkg/services/annotations/annotationsimpl/xorm_store.go +++ b/pkg/services/annotations/annotationsimpl/xorm_store.go @@ -351,14 +351,16 @@ func (r *xormRepositoryImpl) Get(ctx context.Context, query *annotations.ItemQue if err != nil { return err } - sql.WriteString(fmt.Sprintf(" AND (%s)", acFilter)) - - if query.Limit == 0 { - query.Limit = 100 + if acFilter != "" { + sql.WriteString(fmt.Sprintf(" AND (%s)", acFilter)) } // order of ORDER BY arguments match the order of a sql index for performance - sql.WriteString(" ORDER BY a.org_id, a.epoch_end DESC, a.epoch DESC" + r.db.GetDialect().Limit(query.Limit) + " ) dt on dt.id = annotation.id") + orderBy := " ORDER BY a.org_id, a.epoch_end DESC, a.epoch DESC" + if query.Limit > 0 { + orderBy += r.db.GetDialect().Limit(query.Limit) + } + sql.WriteString(orderBy + " ) dt on dt.id = annotation.id") if err := sess.SQL(sql.String(), params...).Find(&items); err != nil { items = nil @@ -372,6 +374,10 @@ func (r *xormRepositoryImpl) Get(ctx context.Context, query *annotations.ItemQue } func (r *xormRepositoryImpl) getAccessControlFilter(user identity.Requester, accessResources *accesscontrol.AccessResources) (string, error) { + if accessResources.SkipAccessControlFilter { + return "", nil + } + var filters []string if accessResources.CanAccessOrgAnnotations { diff --git a/pkg/services/annotations/models.go b/pkg/services/annotations/models.go index 2b1325b4f3c..71c307123e0 100644 --- a/pkg/services/annotations/models.go +++ b/pkg/services/annotations/models.go @@ -21,6 +21,7 @@ type ItemQuery struct { SignedInUser identity.Requester Limit int64 `json:"limit"` + Page int64 } // TagsQuery is the query for a tags search.