From 539bc6d31b34cadedfb53aad6be08fa18ef536d9 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Thu, 21 Dec 2023 15:03:12 -0800 Subject: [PATCH] Playlist: Implement a more efficient List command to support k8s list (#79820) --- pkg/registry/apis/playlist/legacy_storage.go | 23 +------ pkg/services/playlist/model.go | 10 +-- pkg/services/playlist/playlist.go | 3 + .../playlist/playlistimpl/playlist.go | 6 ++ pkg/services/playlist/playlistimpl/store.go | 3 + .../playlist/playlistimpl/store_test.go | 62 +++++++++++++++++++ .../playlist/playlistimpl/xorm_store.go | 46 ++++++++++++++ 7 files changed, 128 insertions(+), 25 deletions(-) diff --git a/pkg/registry/apis/playlist/legacy_storage.go b/pkg/registry/apis/playlist/legacy_storage.go index 8b7f6b4b18d..699d6e5371c 100644 --- a/pkg/registry/apis/playlist/legacy_storage.go +++ b/pkg/registry/apis/playlist/legacy_storage.go @@ -62,31 +62,14 @@ func (s *legacyStorage) List(ctx context.Context, options *internalversion.ListO return nil, err } - limit := 100 - if options.Limit > 0 { - limit = int(options.Limit) - } - res, err := s.service.Search(ctx, &playlistsvc.GetPlaylistsQuery{ - OrgId: orgId, - Limit: limit, - }) + res, err := s.service.List(ctx, orgId) if err != nil { return nil, err } list := &playlist.PlaylistList{} - for _, v := range res { - p, err := s.service.Get(ctx, &playlistsvc.GetPlaylistByUidQuery{ - UID: v.UID, - OrgId: orgId, - }) - if err != nil { - return nil, err - } - list.Items = append(list.Items, *convertToK8sResource(p, s.namespacer)) - } - if len(list.Items) == limit { - list.Continue = "" // TODO? + for idx := range res { + list.Items = append(list.Items, *convertToK8sResource(&res[idx], s.namespacer)) } return list, nil } diff --git a/pkg/services/playlist/model.go b/pkg/services/playlist/model.go index ffd6938b117..b5016c426a1 100644 --- a/pkg/services/playlist/model.go +++ b/pkg/services/playlist/model.go @@ -28,7 +28,7 @@ type Playlist struct { type PlaylistDTO struct { // Unique playlist identifier. Generated on creation, either by the // creator of the playlist of by the application. - Uid string `json:"uid"` + Uid string `json:"uid" db:"uid"` // Name of the playlist. Name string `json:"name"` @@ -40,16 +40,16 @@ type PlaylistDTO struct { Items []PlaylistItemDTO `json:"items,omitempty"` // Returned for k8s - CreatedAt int64 `json:"-"` + CreatedAt int64 `json:"-" db:"created_at"` // Returned for k8s - UpdatedAt int64 `json:"-"` + UpdatedAt int64 `json:"-" db:"updated_at"` // Returned for k8s - OrgID int64 `json:"-"` + OrgID int64 `json:"-" db:"org_id"` // Returned for k8s and added as an annotation - Id int64 `json:"-"` + Id int64 `json:"-" db:"id"` } type PlaylistItemDTO struct { diff --git a/pkg/services/playlist/playlist.go b/pkg/services/playlist/playlist.go index d7fabd62719..c7c9e02ffb0 100644 --- a/pkg/services/playlist/playlist.go +++ b/pkg/services/playlist/playlist.go @@ -11,4 +11,7 @@ type Service interface { Get(context.Context, *GetPlaylistByUidQuery) (*PlaylistDTO, error) Search(context.Context, *GetPlaylistsQuery) (Playlists, error) Delete(ctx context.Context, cmd *DeletePlaylistCommand) error + + // This is optimized for the kubernetes list command that returns full bodies in the list + List(ctx context.Context, orgId int64) ([]PlaylistDTO, error) } diff --git a/pkg/services/playlist/playlistimpl/playlist.go b/pkg/services/playlist/playlistimpl/playlist.go index 90aa4b0b1f5..f81fa187848 100644 --- a/pkg/services/playlist/playlistimpl/playlist.go +++ b/pkg/services/playlist/playlistimpl/playlist.go @@ -90,3 +90,9 @@ func (s *Service) Delete(ctx context.Context, cmd *playlist.DeletePlaylistComman defer span.End() return s.store.Delete(ctx, cmd) } + +func (s *Service) List(ctx context.Context, orgId int64) ([]playlist.PlaylistDTO, error) { + ctx, span := s.tracer.Start(ctx, "playlists.List") + defer span.End() + return s.store.ListAll(ctx, orgId) +} diff --git a/pkg/services/playlist/playlistimpl/store.go b/pkg/services/playlist/playlistimpl/store.go index e3f86109efe..890b729bc11 100644 --- a/pkg/services/playlist/playlistimpl/store.go +++ b/pkg/services/playlist/playlistimpl/store.go @@ -13,4 +13,7 @@ type store interface { GetItems(context.Context, *playlist.GetPlaylistItemsByUidQuery) ([]playlist.PlaylistItem, error) List(context.Context, *playlist.GetPlaylistsQuery) (playlist.Playlists, error) Update(context.Context, *playlist.UpdatePlaylistCommand) (*playlist.PlaylistDTO, error) + + // This is optimized for the kubernetes list command that returns full bodies in the list + ListAll(ctx context.Context, orgId int64) ([]playlist.PlaylistDTO, error) } diff --git a/pkg/services/playlist/playlistimpl/store_test.go b/pkg/services/playlist/playlistimpl/store_test.go index b2d79b59c83..f0d733deb1c 100644 --- a/pkg/services/playlist/playlistimpl/store_test.go +++ b/pkg/services/playlist/playlistimpl/store_test.go @@ -2,6 +2,8 @@ package playlistimpl import ( "context" + "encoding/json" + "fmt" "testing" "time" @@ -104,6 +106,9 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { }) t.Run("Search playlist", func(t *testing.T) { + startTime := time.Now().UnixMilli() + time.Sleep(time.Millisecond * 20) + items := []playlist.PlaylistItem{ {Title: "graphite", Value: "graphite", Type: "dashboard_by_tag"}, {Title: "Backend response times", Value: "3", Type: "dashboard_by_id"}, @@ -113,10 +118,13 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { pl3 := playlist.CreatePlaylistCommand{Name: "NICE office", Interval: "10m", OrgId: 2, Items: items} _, err := playlistStore.Insert(context.Background(), &pl1) require.NoError(t, err) + time.Sleep(time.Millisecond * 20) _, err = playlistStore.Insert(context.Background(), &pl2) require.NoError(t, err) + time.Sleep(time.Millisecond * 20) _, err = playlistStore.Insert(context.Background(), &pl3) require.NoError(t, err) + time.Sleep(time.Millisecond * 20) t.Run("With Org ID", func(t *testing.T) { qr := playlist.GetPlaylistsQuery{Limit: 100, OrgId: 1} @@ -137,6 +145,60 @@ func testIntegrationPlaylistDataAccess(t *testing.T, fn getStore) { require.NoError(t, err) require.Equal(t, 2, len(res)) }) + + t.Run("With FullList support", func(t *testing.T) { + res, err := playlistStore.ListAll(context.Background(), 1) + require.NoError(t, err) + + // Make sure the timestamps came through OK (the risk with SQLX) + offsetTime := startTime + for id, v := range res { + res[id].Uid = fmt.Sprintf("ROW:%d", id) // normalize for JSON test + + elapsed := v.CreatedAt - offsetTime + require.Greater(t, v.CreatedAt, startTime) + require.Greater(t, elapsed, int64(10)) // sleeps 20ms + offsetTime = v.CreatedAt + } + + jj, err := json.MarshalIndent(res, "", " ") + require.NoError(t, err) + //fmt.Printf("OUT:%s\n", string(jj)) + + // Each row has a full payload + require.JSONEq(t, `[ + { + "uid": "ROW:0", + "name": "NYC office", + "interval": "10m", + "items": [ + { + "type": "dashboard_by_tag", + "value": "graphite" + }, + { + "type": "dashboard_by_id", + "value": "3" + } + ] + }, + { + "uid": "ROW:1", + "name": "NICE office", + "interval": "10m", + "items": [ + { + "type": "dashboard_by_tag", + "value": "graphite" + }, + { + "type": "dashboard_by_id", + "value": "3" + } + ] + } + ]`, string(jj)) + }) }) t.Run("Delete playlist that doesn't exist, should not return error", func(t *testing.T) { diff --git a/pkg/services/playlist/playlistimpl/xorm_store.go b/pkg/services/playlist/playlistimpl/xorm_store.go index 871252d3a65..3b3a1998eb6 100644 --- a/pkg/services/playlist/playlistimpl/xorm_store.go +++ b/pkg/services/playlist/playlistimpl/xorm_store.go @@ -2,6 +2,7 @@ package playlistimpl import ( "context" + "fmt" "time" "github.com/grafana/grafana/pkg/infra/db" @@ -180,6 +181,51 @@ func (s *sqlStore) List(ctx context.Context, query *playlist.GetPlaylistsQuery) return playlists, err } +func (s *sqlStore) ListAll(ctx context.Context, orgId int64) ([]playlist.PlaylistDTO, error) { + db := s.db.GetSqlxSession() // OK because dates are numbers! + + playlists := []playlist.PlaylistDTO{} + err := db.Select(ctx, &playlists, "SELECT * FROM playlist WHERE org_id=? ORDER BY created_at asc", orgId) + if err != nil { + return nil, err + } + + // Create a map that links playlist id to the playlist array index + lookup := map[int64]int{} + for i, v := range playlists { + lookup[v.Id] = i + } + + var playlistId int64 + var itemType string + var itemValue string + + rows, err := db.Query(ctx, `SELECT playlist.id,playlist_item.type,playlist_item.value + FROM playlist_item + JOIN playlist ON playlist_item.playlist_id = playlist.id + WHERE playlist.org_id = ? + ORDER BY playlist_id asc, `+s.db.Quote("order")+` asc`, orgId) + if err != nil { + return nil, err + } + for rows.Next() { + err = rows.Scan(&playlistId, &itemType, &itemValue) + if err != nil { + return nil, err + } + idx, ok := lookup[playlistId] + if !ok { + return nil, fmt.Errorf("could not find playlist by id") + } + items := append(playlists[idx].Items, playlist.PlaylistItemDTO{ + Type: itemType, + Value: itemValue, + }) + playlists[idx].Items = items + } + return playlists, err +} + func (s *sqlStore) GetItems(ctx context.Context, query *playlist.GetPlaylistItemsByUidQuery) ([]playlist.PlaylistItem, error) { var playlistItems = make([]playlist.PlaylistItem, 0) if query.PlaylistUID == "" || query.OrgId == 0 {