From 2554f6e7361d6cc310ad2ab54985639cbf851637 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Fri, 26 Apr 2024 16:06:57 +0100 Subject: [PATCH 1/7] moving folders to slog --- pkg/infra/log/log.go | 8 ++-- pkg/infra/log/slogadapter/adapter.go | 3 ++ pkg/server/wire.go | 2 + pkg/services/folder/folderimpl/folder.go | 42 +++++++------------ pkg/services/folder/folderimpl/folder_test.go | 22 +++++----- pkg/services/folder/folderimpl/sqlstore.go | 6 +-- 6 files changed, 38 insertions(+), 45 deletions(-) diff --git a/pkg/infra/log/log.go b/pkg/infra/log/log.go index f9c741e1346a..767d6e349c16 100644 --- a/pkg/infra/log/log.go +++ b/pkg/infra/log/log.go @@ -200,19 +200,21 @@ func (cl *ConcreteLogger) log(msg string, logLevel level.Value, args ...any) err return cl.Log(append([]any{level.Key(), logLevel, "msg", msg}, args...)...) } -func (cl *ConcreteLogger) FromContext(ctx context.Context) Logger { +func FromContext(ctx context.Context) []any { args := []any{} - for _, p := range ctxLogProviders { if pArgs, exists := p(ctx); exists { args = append(args, pArgs...) } } + return args +} +func (cl *ConcreteLogger) FromContext(ctx context.Context) Logger { + args := FromContext(ctx) if len(args) > 0 { return cl.New(args...) } - return cl } diff --git a/pkg/infra/log/slogadapter/adapter.go b/pkg/infra/log/slogadapter/adapter.go index 759888b3286b..48ff5c2c561d 100644 --- a/pkg/infra/log/slogadapter/adapter.go +++ b/pkg/infra/log/slogadapter/adapter.go @@ -13,6 +13,8 @@ type slogHandler struct { log.Logger } +func Provide() slog.Handler { return New(log.New()) } + // NewSLogHandler returns a new slog.Handler that logs to the given log.Logger. func New(logger log.Logger) *slogHandler { return &slogHandler{logger} @@ -31,6 +33,7 @@ func (h *slogHandler) Handle(ctx context.Context, r slog.Record) error { return true } r.Attrs(fn) + attrs = append(attrs, log.FromContext(ctx)...) switch level := r.Level; { case level < slog.LevelInfo: diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 56fa778bf6ac..f0520f9ae732 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -21,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/infra/httpclient/httpclientprovider" "github.com/grafana/grafana/pkg/infra/kvstore" "github.com/grafana/grafana/pkg/infra/localcache" + "github.com/grafana/grafana/pkg/infra/log/slogadapter" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/infra/serverlock" @@ -368,6 +369,7 @@ var wireBasicSet = wire.NewSet( anonstore.ProvideAnonDBStore, wire.Bind(new(anonstore.AnonStore), new(*anonstore.AnonDBStore)), loggermw.Provide, + slogadapter.Provide, signingkeysimpl.ProvideEmbeddedSigningKeysService, wire.Bind(new(signingkeys.Service), new(*signingkeysimpl.Service)), ssoSettingsImpl.ProvideService, diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index c7a74ff7d2b9..148e703a211e 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "runtime" "strings" "sync" @@ -17,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/events" "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/metrics" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/auth/identity" @@ -31,15 +31,13 @@ import ( "github.com/grafana/grafana/pkg/services/store/entity" "github.com/grafana/grafana/pkg/services/supportbundles" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) type Service struct { store store db db.DB - log log.Logger - cfg *setting.Cfg + log *slog.Logger dashboardStore dashboards.Store dashboardFolderStore folder.FolderStore features featuremgmt.FeatureToggles @@ -55,18 +53,17 @@ type Service struct { func ProvideService( ac accesscontrol.AccessControl, bus bus.Bus, - cfg *setting.Cfg, dashboardStore dashboards.Store, folderStore folder.FolderStore, db db.DB, // DB for the (new) nested folder store features featuremgmt.FeatureToggles, supportBundles supportbundles.Service, r prometheus.Registerer, + slogHandler slog.Handler, ) folder.Service { - store := ProvideStore(db, cfg) + store := ProvideStore(db) srv := &Service{ - cfg: cfg, - log: log.New("folder-service"), + log: slog.New(slogHandler).With("logger", "folder-service"), dashboardStore: dashboardStore, dashboardFolderStore: folderStore, store: store, @@ -546,8 +543,6 @@ func (s *Service) getFolderByTitle(ctx context.Context, orgID int64, title strin } func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) { - logger := s.log.FromContext(ctx) - if cmd.SignedInUser == nil || cmd.SignedInUser.IsNil() { return nil, folder.ErrBadRequest.Errorf("missing signed in user") } @@ -630,7 +625,7 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) ( } if nestedFolder, err = s.nestedFolderCreate(ctx, cmd); err != nil { - logger.Error("error saving folder to nested folder store", "error", err) + s.log.ErrorContext(ctx, "error saving folder to nested folder store", "error", err) return err } @@ -648,8 +643,6 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) ( } func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) { - logger := s.log.FromContext(ctx) - if cmd.SignedInUser == nil { return nil, folder.ErrBadRequest.Errorf("missing signed in user") } @@ -683,7 +676,7 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) ( UID: dashFolder.UID, OrgID: cmd.OrgID, }); err != nil { - logger.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", id, "namespace", namespace, "error", err) + s.log.ErrorContext(ctx, "failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", id, "namespace", namespace, "error", err) return err } } @@ -692,7 +685,7 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) ( }) if err != nil { - logger.Error("folder update failed", "folderUID", cmd.UID, "error", err) + s.log.ErrorContext(ctx, "folder update failed", "folderUID", cmd.UID, "error", err) return nil, err } @@ -710,8 +703,6 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) ( } func (s *Service) legacyUpdate(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) { - logger := s.log.FromContext(ctx) - query := dashboards.GetDashboardQuery{OrgID: cmd.OrgID, UID: cmd.UID} queryResult, err := s.dashboardStore.GetDashboard(ctx, &query) if err != nil { @@ -736,7 +727,7 @@ func (s *Service) legacyUpdate(ctx context.Context, cmd *folder.UpdateFolderComm if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { userID, err = identity.IntIdentifier(namespace, id) if err != nil { - logger.Error("failed to parse user ID", "namespace", namespace, "userID", id, "error", err) + s.log.ErrorContext(ctx, "failed to parse user ID", "namespace", namespace, "userID", id, "error", err) } } @@ -791,7 +782,6 @@ func prepareForUpdate(dashFolder *dashboards.Dashboard, orgId int64, userId int6 } func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) error { - logger := s.log.FromContext(ctx) if cmd.SignedInUser == nil { return folder.ErrBadRequest.Errorf("missing signed in user") } @@ -819,7 +809,7 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e descendants, err := s.nestedFolderDelete(ctx, cmd) if err != nil { - logger.Error("the delete folder on folder table failed with err: ", "error", err) + s.log.ErrorContext(ctx, "the delete folder on folder table failed with err: ", "error", err) return err } folders = append(folders, descendants...) @@ -962,7 +952,6 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol // its descendant folders (folders which are nested within it either directly or indirectly) from // the folder store and returns the UIDs for all its descendants. func (s *Service) nestedFolderDelete(ctx context.Context, cmd *folder.DeleteFolderCommand) ([]string, error) { - logger := s.log.FromContext(ctx) descendantUIDs := []string{} if cmd.SignedInUser == nil { return descendantUIDs, folder.ErrBadRequest.Errorf("missing signed in user") @@ -979,25 +968,24 @@ func (s *Service) nestedFolderDelete(ctx context.Context, cmd *folder.DeleteFold descendants, err := s.store.GetDescendants(ctx, cmd.OrgID, cmd.UID) if err != nil { - logger.Error("failed to get descendant folders", "error", err) + s.log.ErrorContext(ctx, "failed to get descendant folders", "error", err) return descendantUIDs, err } for _, f := range descendants { descendantUIDs = append(descendantUIDs, f.UID) } - logger.Info("deleting folder and its descendants", "org_id", cmd.OrgID, "uid", cmd.UID) + s.log.InfoContext(ctx, "deleting folder and its descendants", "org_id", cmd.OrgID, "uid", cmd.UID) toDelete := append(descendantUIDs, cmd.UID) err = s.store.Delete(ctx, toDelete, cmd.OrgID) if err != nil { - logger.Info("failed deleting folder", "org_id", cmd.OrgID, "uid", cmd.UID, "err", err) + s.log.InfoContext(ctx, "failed deleting folder", "org_id", cmd.OrgID, "uid", cmd.UID, "err", err) return descendantUIDs, err } return descendantUIDs, nil } func (s *Service) GetDescendantCounts(ctx context.Context, q *folder.GetDescendantCountsQuery) (folder.DescendantCounts, error) { - logger := s.log.FromContext(ctx) if q.SignedInUser == nil { return nil, folder.ErrBadRequest.Errorf("missing signed-in user") } @@ -1013,7 +1001,7 @@ func (s *Service) GetDescendantCounts(ctx context.Context, q *folder.GetDescenda if s.features.IsEnabled(ctx, featuremgmt.FlagNestedFolders) { descendantFolders, err := s.store.GetDescendants(ctx, q.OrgID, *q.UID) if err != nil { - logger.Error("failed to get descendant folders", "error", err) + s.log.ErrorContext(ctx, "failed to get descendant folders", "error", err) return nil, err } for _, f := range descendantFolders { @@ -1025,7 +1013,7 @@ func (s *Service) GetDescendantCounts(ctx context.Context, q *folder.GetDescenda for _, v := range s.registry { c, err := v.CountInFolders(ctx, q.OrgID, folders, q.SignedInUser) if err != nil { - logger.Error("failed to count folder descendants", "error", err) + s.log.ErrorContext(ctx, "failed to count folder descendants", "error", err) return nil, err } countsMap[v.Kind()] = c diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 5abe492742cd..d5f45d8a02d6 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -90,7 +90,7 @@ func TestIntegrationFolderService(t *testing.T) { service := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -367,7 +367,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { serviceWithFlagOn := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -482,7 +482,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { serviceWithFlagOff := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -562,7 +562,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() serviceWithFlagOff := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardFolderStore: folderStore, features: featuresFlagOff, bus: b, @@ -742,7 +742,7 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) { dashboardStore: &dashStore, dashboardFolderStore: dashboardFolderStore, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), accessControl: acimpl.ProvideAccessControl(cfg), metrics: newFoldersMetrics(nil), } @@ -779,7 +779,7 @@ func TestFolderServiceDualWrite(t *testing.T) { dashboardStore: dashStore, dashboardFolderStore: dashboardFolderStore, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), accessControl: acimpl.ProvideAccessControl(cfg), metrics: newFoldersMetrics(nil), bus: bus.ProvideBus(tracing.InitializeTracerForTest()), @@ -1295,7 +1295,7 @@ func TestIntegrationNestedFolderSharedWithMe(t *testing.T) { serviceWithFlagOn := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1664,7 +1664,7 @@ func TestFolderServiceGetFolder(t *testing.T) { return Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1747,7 +1747,7 @@ func TestFolderServiceGetFolders(t *testing.T) { serviceWithFlagOff := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1834,7 +1834,7 @@ func TestGetChildrenFilterByPermission(t *testing.T) { folderSvcOn := &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -2138,7 +2138,7 @@ func setup(t *testing.T, dashStore dashboards.Store, dashboardFolderStore folder cfg := setting.NewCfg() return &Service{ cfg: cfg, - log: log.New("test-folder-service"), + glog: log.New("test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: dashboardFolderStore, store: nestedFolderStore, diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index d77ef2bcd44e..a554c6470f21 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -15,7 +15,6 @@ import ( "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" - "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -24,14 +23,13 @@ const DEFAULT_BATCH_SIZE = 999 type sqlStore struct { db db.DB log log.Logger - cfg *setting.Cfg } // sqlStore implements the store interface. var _ store = (*sqlStore)(nil) -func ProvideStore(db db.DB, cfg *setting.Cfg) *sqlStore { - return &sqlStore{db: db, log: log.New("folder-store"), cfg: cfg} +func ProvideStore(db db.DB) *sqlStore { + return &sqlStore{db: db, log: log.New("folder-store")} } func (ss *sqlStore) Create(ctx context.Context, cmd folder.CreateFolderCommand) (*folder.Folder, error) { From 2b2c57d39f90e5bd76320861d5a405750e134b90 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Sat, 27 Apr 2024 11:06:53 +0100 Subject: [PATCH 2/7] trying to fix the tests --- pkg/infra/log/logtest/slog.go | 41 +++++++++++++ pkg/services/folder/folderimpl/folder_test.go | 57 ++++++++----------- .../folder/folderimpl/sqlstore_test.go | 16 +++--- 3 files changed, 72 insertions(+), 42 deletions(-) create mode 100644 pkg/infra/log/logtest/slog.go diff --git a/pkg/infra/log/logtest/slog.go b/pkg/infra/log/logtest/slog.go new file mode 100644 index 000000000000..3ebe7fbbbca0 --- /dev/null +++ b/pkg/infra/log/logtest/slog.go @@ -0,0 +1,41 @@ +package logtest + +import ( + "context" + "log/slog" + "strings" + "testing" +) + +type NopHandler struct{} + +func NewNopHandler(t *testing.T) *NopHandler { + return &NopHandler{} +} +func (h *NopHandler) Enabled(_ context.Context, _ slog.Level) bool { return false } +func (h *NopHandler) WithAttrs(_ []slog.Attr) slog.Handler { return h } +func (h *NopHandler) WithGroup(_ string) slog.Handler { return h } +func (h *NopHandler) Handle(_ context.Context, r slog.Record) error { return nil } + +type TestHandler struct { + t *testing.T +} + +func NewTestHandler(t *testing.T) *TestHandler { + return &TestHandler{t: t} +} + +func (th *TestHandler) Enabled(_ context.Context, _ slog.Level) bool { return true } +func (th *TestHandler) WithAttrs(_ []slog.Attr) slog.Handler { return th } +func (th *TestHandler) WithGroup(_ string) slog.Handler { return th } +func (th *TestHandler) Handle(ctx context.Context, r slog.Record) error { + th.t.Helper() + buf := &strings.Builder{} + h := slog.NewTextHandler(buf, nil) + err := h.Handle(ctx, r) + if err != nil { + return err + } + th.t.Log(buf.String()) + return nil +} diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index d5f45d8a02d6..3b5675400289 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "math/rand" "strings" "testing" @@ -20,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/db/dbtest" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/log/logtest" "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" @@ -56,10 +58,9 @@ func TestIntegrationProvideFolderService(t *testing.T) { t.Skip("skipping integration test") } t.Run("should register scope resolvers", func(t *testing.T) { - cfg := setting.NewCfg() ac := acmock.New() db := db.InitTestDB(t) - ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), cfg, nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) + ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil, logtest.NewTestHandler(t)) require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 3) }) @@ -72,7 +73,7 @@ func TestIntegrationFolderService(t *testing.T) { t.Run("Folder service tests", func(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} db, cfg := sqlstore.InitTestDB(t) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) folderStore := foldertest.NewFakeFolderStore(t) @@ -89,8 +90,7 @@ func TestIntegrationFolderService(t *testing.T) { } service := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -360,14 +360,13 @@ func TestIntegrationNestedFolderService(t *testing.T) { featuresFlagOn := featuremgmt.WithFeatures("nestedFolders") dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOn, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) b := bus.ProvideBus(tracing.InitializeTracerForTest()) ac := acimpl.ProvideAccessControl(cfg) serviceWithFlagOn := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -478,11 +477,10 @@ func TestIntegrationNestedFolderService(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOff, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) serviceWithFlagOff := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -561,8 +559,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { t.Run("Should delete folders", func(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() serviceWithFlagOff := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardFolderStore: folderStore, features: featuresFlagOff, bus: b, @@ -643,7 +640,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { dashStore, err := database.ProvideDashboardStore(db, cfg, tc.featuresFlag, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) tc.service.dashboardStore = dashStore tc.service.store = nestedFolderStore @@ -736,13 +733,12 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) { db, cfg := sqlstore.InitTestDB(t) folderService := &Service{ - cfg: cfg, store: nestedFolderStore, + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), db: db, dashboardStore: &dashStore, dashboardFolderStore: dashboardFolderStore, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), - glog: log.New("test-folder-service"), accessControl: acimpl.ProvideAccessControl(cfg), metrics: newFoldersMetrics(nil), } @@ -765,7 +761,7 @@ func TestFolderServiceDualWrite(t *testing.T) { db, _ := sqlstore.InitTestDB(t) cfg := setting.NewCfg() features := featuremgmt.WithFeatures() - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) dashStore, err := database.ProvideDashboardStore(db, cfg, features, tagimpl.ProvideService(db), "atest.FakeQuotaService{}) require.NoError(t, err) @@ -773,13 +769,12 @@ func TestFolderServiceDualWrite(t *testing.T) { dashboardFolderStore := ProvideDashboardFolderStore(db) folderService := &Service{ - cfg: setting.NewCfg(), store: nestedFolderStore, + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), db: db, dashboardStore: dashStore, dashboardFolderStore: dashboardFolderStore, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), - glog: log.New("test-folder-service"), accessControl: acimpl.ProvideAccessControl(cfg), metrics: newFoldersMetrics(nil), bus: bus.ProvideBus(tracing.InitializeTracerForTest()), @@ -1288,14 +1283,13 @@ func TestIntegrationNestedFolderSharedWithMe(t *testing.T) { featuresFlagOn := featuremgmt.WithFeatures("nestedFolders") dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOn, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) b := bus.ProvideBus(tracing.InitializeTracerForTest()) ac := acimpl.ProvideAccessControl(cfg) serviceWithFlagOn := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1657,14 +1651,13 @@ func TestFolderServiceGetFolder(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOff, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) b := bus.ProvideBus(tracing.InitializeTracerForTest()) ac := acimpl.ProvideAccessControl(cfg) return Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1740,14 +1733,13 @@ func TestFolderServiceGetFolders(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOff, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) b := bus.ProvideBus(tracing.InitializeTracerForTest()) ac := acimpl.ProvideAccessControl(cfg) serviceWithFlagOff := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -1825,7 +1817,7 @@ func TestGetChildrenFilterByPermission(t *testing.T) { featuresFlagOff := featuremgmt.WithFeatures() dashStore, err := database.ProvideDashboardStore(db, cfg, featuresFlagOff, tagimpl.ProvideService(db), quotaService) require.NoError(t, err) - nestedFolderStore := ProvideStore(db, cfg) + nestedFolderStore := ProvideStore(db) b := bus.ProvideBus(tracing.InitializeTracerForTest()) ac := acimpl.ProvideAccessControl(cfg) @@ -1833,8 +1825,7 @@ func TestGetChildrenFilterByPermission(t *testing.T) { features := featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders) folderSvcOn := &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: folderStore, store: nestedFolderStore, @@ -2135,10 +2126,8 @@ func setup(t *testing.T, dashStore dashboards.Store, dashboardFolderStore folder t.Helper() // nothing enabled yet - cfg := setting.NewCfg() return &Service{ - cfg: cfg, - glog: log.New("test-folder-service"), + log: slog.New(logtest.NewTestHandler(t)).With("logger", "test-folder-service"), dashboardStore: dashStore, dashboardFolderStore: dashboardFolderStore, store: nestedFolderStore, diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index 5c33c3d935b4..e346c91a9605 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -32,7 +32,7 @@ func TestIntegrationCreate(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -152,7 +152,7 @@ func TestIntegrationDelete(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -199,7 +199,7 @@ func TestIntegrationUpdate(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -374,7 +374,7 @@ func TestIntegrationGet(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -491,7 +491,7 @@ func TestIntegrationGetParents(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -559,7 +559,7 @@ func TestIntegrationGetChildren(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -739,7 +739,7 @@ func TestIntegrationGetHeight(t *testing.T) { } db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) @@ -772,7 +772,7 @@ func TestIntegrationGetFolders(t *testing.T) { foldersNum := 10 db, cfg := sqlstore.InitTestDB(t) - folderStore := ProvideStore(db, cfg) + folderStore := ProvideStore(db) orgID := CreateOrg(t, db, cfg) From 83360386fa2b885fbfe361d21d8a44b61ea692df Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 30 Apr 2024 12:13:49 +0200 Subject: [PATCH 3/7] fix per-logger filters in slog --- pkg/infra/log/log.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/pkg/infra/log/log.go b/pkg/infra/log/log.go index 767d6e349c16..1f478b1f48b2 100644 --- a/pkg/infra/log/log.go +++ b/pkg/infra/log/log.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "io" + "log/slog" "os" "path/filepath" "sort" @@ -111,15 +112,23 @@ func (lm *logManager) initialize(loggers []logWithFilters) { } func (lm *logManager) New(ctx ...any) *ConcreteLogger { - if len(ctx) == 0 { + // First key-value could be "logger" and a logger name, that would be handled differently + // to allow per-logger filtering. Otherwise a simple concrete logger is returned. + if len(ctx) < 2 || ctx[0] != "logger" { return lm.ConcreteLogger } lm.mutex.Lock() defer lm.mutex.Unlock() - loggerName, ok := ctx[1].(string) - if !ok { + // Logger name could be a string variable or an slog.Value() + loggerName := "" + switch v := ctx[1].(type) { + case string: + loggerName = v + case slog.Value: + loggerName = v.String() + default: return lm.ConcreteLogger } @@ -220,10 +229,10 @@ func (cl *ConcreteLogger) FromContext(ctx context.Context) Logger { func (cl *ConcreteLogger) New(ctx ...any) *ConcreteLogger { if len(ctx) == 0 { - root.New() + return root.New() } - return newConcreteLogger(gokitlog.With(&cl.SwapLogger), ctx...) + return root.New(append(cl.ctx, ctx...)...) } // New creates a new logger. From 1272d9400ee7f8e07dfc645b2ef555e17661e557 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 30 Apr 2024 13:43:30 +0200 Subject: [PATCH 4/7] allow using slog.Default() --- pkg/infra/log/slogadapter/adapter.go | 6 ++++++ pkg/services/folder/folderimpl/folder.go | 3 +-- pkg/services/folder/folderimpl/folder_test.go | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/infra/log/slogadapter/adapter.go b/pkg/infra/log/slogadapter/adapter.go index 48ff5c2c561d..5263a58ee9f1 100644 --- a/pkg/infra/log/slogadapter/adapter.go +++ b/pkg/infra/log/slogadapter/adapter.go @@ -13,6 +13,12 @@ type slogHandler struct { log.Logger } +func init() { + // Lots of New's here: Default = slog.Logger <- slog.Handler <- infra/log.Logger + slog.SetDefault(slog.New(New(log.New()))) +} + +// Provide is a helper method to be used with Wire, however most services should use slog.Default() func Provide() slog.Handler { return New(log.New()) } // NewSLogHandler returns a new slog.Handler that logs to the given log.Logger. diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 148e703a211e..23095d137687 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -59,11 +59,10 @@ func ProvideService( features featuremgmt.FeatureToggles, supportBundles supportbundles.Service, r prometheus.Registerer, - slogHandler slog.Handler, ) folder.Service { store := ProvideStore(db) srv := &Service{ - log: slog.New(slogHandler).With("logger", "folder-service"), + log: slog.Default().With("logger", "folder-service"), dashboardStore: dashboardStore, dashboardFolderStore: folderStore, store: store, diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 3b5675400289..4857156be7b0 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -60,7 +60,7 @@ func TestIntegrationProvideFolderService(t *testing.T) { t.Run("should register scope resolvers", func(t *testing.T) { ac := acmock.New() db := db.InitTestDB(t) - ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil, logtest.NewTestHandler(t)) + ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 3) }) From d93afc9b984f0e159ed8d225bec3e91482bbdf96 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 30 Apr 2024 14:15:12 +0200 Subject: [PATCH 5/7] bring cfg back to keep the pr small --- pkg/services/folder/folderimpl/folder.go | 2 ++ pkg/services/folder/folderimpl/folder_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 23095d137687..4f4e2f543d22 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -31,6 +31,7 @@ import ( "github.com/grafana/grafana/pkg/services/store/entity" "github.com/grafana/grafana/pkg/services/supportbundles" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -53,6 +54,7 @@ type Service struct { func ProvideService( ac accesscontrol.AccessControl, bus bus.Bus, + _ *setting.Cfg, dashboardStore dashboards.Store, folderStore folder.FolderStore, db db.DB, // DB for the (new) nested folder store diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 4857156be7b0..7398adf23c05 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -60,7 +60,7 @@ func TestIntegrationProvideFolderService(t *testing.T) { t.Run("should register scope resolvers", func(t *testing.T) { ac := acmock.New() db := db.InitTestDB(t) - ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) + ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 3) }) From a3a8062fb77e34e350c728001d5cd27addcef033 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 30 Apr 2024 16:50:34 +0200 Subject: [PATCH 6/7] fix tests --- pkg/infra/log/log.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/infra/log/log.go b/pkg/infra/log/log.go index 1f478b1f48b2..c77ccdd30526 100644 --- a/pkg/infra/log/log.go +++ b/pkg/infra/log/log.go @@ -114,9 +114,12 @@ func (lm *logManager) initialize(loggers []logWithFilters) { func (lm *logManager) New(ctx ...any) *ConcreteLogger { // First key-value could be "logger" and a logger name, that would be handled differently // to allow per-logger filtering. Otherwise a simple concrete logger is returned. - if len(ctx) < 2 || ctx[0] != "logger" { + if len(ctx) < 2 { return lm.ConcreteLogger } + if ctx[0] != "logger" { + return newConcreteLogger(lm.ConcreteLogger, ctx...) + } lm.mutex.Lock() defer lm.mutex.Unlock() @@ -133,7 +136,10 @@ func (lm *logManager) New(ctx ...any) *ConcreteLogger { } if logger, exists := lm.loggersByName[loggerName]; exists { - return logger + if len(ctx) == 0 { + return logger + } + return newConcreteLogger(logger, ctx...) } if len(lm.logFilters) == 0 { @@ -228,11 +234,11 @@ func (cl *ConcreteLogger) FromContext(ctx context.Context) Logger { } func (cl *ConcreteLogger) New(ctx ...any) *ConcreteLogger { - if len(ctx) == 0 { - return root.New() + if len(cl.ctx) == 0 { + return root.New(ctx...) } - return root.New(append(cl.ctx, ctx...)...) + return newConcreteLogger(gokitlog.With(&cl.SwapLogger), ctx...) } // New creates a new logger. From 6584f93bfab3a5e36c321fd13c9c27b068637d76 Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Tue, 30 Apr 2024 18:15:40 +0200 Subject: [PATCH 7/7] back to the roots --- pkg/infra/log/log.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/infra/log/log.go b/pkg/infra/log/log.go index c77ccdd30526..b3dca258753e 100644 --- a/pkg/infra/log/log.go +++ b/pkg/infra/log/log.go @@ -136,10 +136,7 @@ func (lm *logManager) New(ctx ...any) *ConcreteLogger { } if logger, exists := lm.loggersByName[loggerName]; exists { - if len(ctx) == 0 { - return logger - } - return newConcreteLogger(logger, ctx...) + return logger } if len(lm.logFilters) == 0 {