Skip to content

Commit

Permalink
Replace use of global loggers in the module subsystem (#143)
Browse files Browse the repository at this point in the history 
In prep for moving the "modules" package to dskit, replace uses
of the global Mimir logger with an injected one.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
  • Loading branch information
@56quarters
56quarters committed Aug 16, 2021
1 parent 56dc083 commit 184bffd
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 24 deletions.
2 changes: 1 addition & 1 deletion pkg/mimir/modules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ func (t *Mimir) initQueryScheduler() (services.Service, error) {
}

func (t *Mimir) setupModuleManager() error {
mm := modules.NewManager()
mm := modules.NewManager(util_log.Logger)

// Register all modules here.
// RegisterModule(name string, initFn func()(services.Service, error))
Expand Down
19 changes: 10 additions & 9 deletions pkg/util/module_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ import (
"context"
"fmt"

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
"github.com/grafana/dskit/services"
"github.com/pkg/errors"

util_log "github.com/grafana/mimir/pkg/util/log"
)

// This service wraps module service, and adds waiting for dependencies to start before starting,
Expand All @@ -23,6 +22,7 @@ type moduleService struct {

service services.Service
name string
logger log.Logger

// startDeps, stopDeps return map of service names to services
startDeps, stopDeps func(string) map[string]services.Service
Expand All @@ -31,9 +31,10 @@ type moduleService struct {
// NewModuleService wraps a module service, and makes sure that dependencies are started/stopped before module service starts or stops.
// If any dependency fails to start, this service fails as well.
// On stop, errors from failed dependencies are ignored.
func NewModuleService(name string, service services.Service, startDeps, stopDeps func(string) map[string]services.Service) services.Service {
func NewModuleService(name string, logger log.Logger, service services.Service, startDeps, stopDeps func(string) map[string]services.Service) services.Service {
w := &moduleService{
name: name,
logger: logger,
service: service,
startDeps: startDeps,
stopDeps: stopDeps,
Expand All @@ -51,7 +52,7 @@ func (w *moduleService) start(serviceContext context.Context) error {
continue
}

level.Debug(util_log.Logger).Log("msg", "module waiting for initialization", "module", w.name, "waiting_for", m)
level.Debug(w.logger).Log("msg", "module waiting for initialization", "module", w.name, "waiting_for", m)

err := s.AwaitRunning(serviceContext)
if err != nil {
Expand All @@ -61,7 +62,7 @@ func (w *moduleService) start(serviceContext context.Context) error {

// we don't want to let this service to stop until all dependant services are stopped,
// so we use independent context here
level.Info(util_log.Logger).Log("msg", "initialising", "module", w.name)
level.Info(w.logger).Log("msg", "initialising", "module", w.name)
err := w.service.StartAsync(context.Background())
if err != nil {
return errors.Wrapf(err, "error starting module: %s", w.name)
Expand All @@ -83,17 +84,17 @@ func (w *moduleService) stop(_ error) error {
// Only wait for other modules, if underlying service is still running.
w.waitForModulesToStop()

level.Debug(util_log.Logger).Log("msg", "stopping", "module", w.name)
level.Debug(w.logger).Log("msg", "stopping", "module", w.name)

err = services.StopAndAwaitTerminated(context.Background(), w.service)
} else {
err = w.service.FailureCase()
}

if err != nil && err != ErrStopProcess {
level.Warn(util_log.Logger).Log("msg", "module failed with error", "module", w.name, "err", err)
level.Warn(w.logger).Log("msg", "module failed with error", "module", w.name, "err", err)
} else {
level.Info(util_log.Logger).Log("msg", "module stopped", "module", w.name)
level.Info(w.logger).Log("msg", "module stopped", "module", w.name)
}
return err
}
Expand All @@ -106,7 +107,7 @@ func (w *moduleService) waitForModulesToStop() {
continue
}

level.Debug(util_log.Logger).Log("msg", "module waiting for", "module", w.name, "waiting_for", n)
level.Debug(w.logger).Log("msg", "module waiting for", "module", w.name, "waiting_for", n)
// Passed context isn't canceled, so we can only get error here, if service
// fails. But we don't care *how* service stops, as long as it is done.
_ = s.AwaitTerminated(context.Background())
Expand Down
5 changes: 3 additions & 2 deletions pkg/util/modules/module_service_wrapper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
package modules

import (
"github.com/go-kit/kit/log"
"github.com/grafana/dskit/services"

"github.com/grafana/mimir/pkg/util"
)

// This function wraps module service, and adds waiting for dependencies to start before starting,
// and dependant modules to stop before stopping this module service.
func newModuleServiceWrapper(serviceMap map[string]services.Service, mod string, modServ services.Service, startDeps []string, stopDeps []string) services.Service {
func newModuleServiceWrapper(serviceMap map[string]services.Service, mod string, logger log.Logger, modServ services.Service, startDeps []string, stopDeps []string) services.Service {
getDeps := func(deps []string) map[string]services.Service {
r := map[string]services.Service{}
for _, m := range deps {
Expand All @@ -25,7 +26,7 @@ func newModuleServiceWrapper(serviceMap map[string]services.Service, mod string,
return r
}

return util.NewModuleService(mod, modServ,
return util.NewModuleService(mod, logger, modServ,
func(_ string) map[string]services.Service {
return getDeps(startDeps)
},
Expand Down
7 changes: 5 additions & 2 deletions pkg/util/modules/modules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"sort"

"github.com/go-kit/kit/log"
"github.com/grafana/dskit/services"
"github.com/pkg/errors"
)
Expand All @@ -29,6 +30,7 @@ type module struct {
// in the right order of dependencies.
type Manager struct {
modules map[string]*module
logger log.Logger
}

// UserInvisibleModule is an option for `RegisterModule` that marks module not visible to user. Modules are user visible by default.
Expand All @@ -37,9 +39,10 @@ func UserInvisibleModule(m *module) {
}

// NewManager creates a new Manager
func NewManager() *Manager {
func NewManager(logger log.Logger) *Manager {
return &Manager{
modules: make(map[string]*module),
logger: logger,
}
}

Expand Down Expand Up @@ -112,7 +115,7 @@ func (m *Manager) initModule(name string, initMap map[string]bool, servicesMap m
if s != nil {
// We pass servicesMap, which isn't yet complete. By the time service starts,
// it will be fully built, so there is no need for extra synchronization.
serv = newModuleServiceWrapper(servicesMap, n, s, m.DependenciesForModule(n), m.findInverseDependencies(n, deps[ix+1:]))
serv = newModuleServiceWrapper(servicesMap, n, m.logger, s, m.DependenciesForModule(n), m.findInverseDependencies(n, deps[ix+1:]))
}
}

Expand Down
21 changes: 11 additions & 10 deletions pkg/util/modules/modules_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"testing"
"time"

"github.com/go-kit/kit/log"
"github.com/grafana/dskit/services"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -40,7 +41,7 @@ func TestDependencies(t *testing.T) {
},
}

mm := NewManager()
mm := NewManager(log.NewNopLogger())
for name, mod := range testModules {
mm.RegisterModule(name, mod.initFn)
}
Expand Down Expand Up @@ -79,7 +80,7 @@ func TestDependencies(t *testing.T) {
}

func TestRegisterModuleDefaultsToUserVisible(t *testing.T) {
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule("module1", mockInitFunc)

m := sut.modules["module1"]
Expand All @@ -92,7 +93,7 @@ func TestFunctionalOptAtTheEndWins(t *testing.T) {
userVisibleMod := func(option *module) {
option.userVisible = true
}
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule("mod1", mockInitFunc, UserInvisibleModule, userVisibleMod, UserInvisibleModule)

m := sut.modules["mod1"]
Expand All @@ -102,7 +103,7 @@ func TestFunctionalOptAtTheEndWins(t *testing.T) {
}

func TestGetAllUserVisibleModulesNames(t *testing.T) {
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule("userVisible3", mockInitFunc)
sut.RegisterModule("userVisible2", mockInitFunc)
sut.RegisterModule("userVisible1", mockInitFunc)
Expand All @@ -115,7 +116,7 @@ func TestGetAllUserVisibleModulesNames(t *testing.T) {
}

func TestGetAllUserVisibleModulesNamesHasNoDupWithDependency(t *testing.T) {
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule("userVisible1", mockInitFunc)
sut.RegisterModule("userVisible2", mockInitFunc)
sut.RegisterModule("userVisible3", mockInitFunc)
Expand All @@ -129,7 +130,7 @@ func TestGetAllUserVisibleModulesNamesHasNoDupWithDependency(t *testing.T) {
}

func TestGetEmptyListWhenThereIsNoUserVisibleModule(t *testing.T) {
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule("internal1", mockInitFunc, UserInvisibleModule)
sut.RegisterModule("internal2", mockInitFunc, UserInvisibleModule)
sut.RegisterModule("internal3", mockInitFunc, UserInvisibleModule)
Expand All @@ -143,7 +144,7 @@ func TestGetEmptyListWhenThereIsNoUserVisibleModule(t *testing.T) {
func TestIsUserVisibleModule(t *testing.T) {
userVisibleModName := "userVisible"
internalModName := "internal"
sut := NewManager()
sut := NewManager(log.NewNopLogger())
sut.RegisterModule(userVisibleModName, mockInitFunc)
sut.RegisterModule(internalModName, mockInitFunc, UserInvisibleModule)

Expand All @@ -161,7 +162,7 @@ func TestIsModuleRegistered(t *testing.T) {
successModule := "successModule"
failureModule := "failureModule"

m := NewManager()
m := NewManager(log.NewNopLogger())
m.RegisterModule(successModule, mockInitFunc)

var result = m.IsModuleRegistered(successModule)
Expand All @@ -172,7 +173,7 @@ func TestIsModuleRegistered(t *testing.T) {
}

func TestDependenciesForModule(t *testing.T) {
m := NewManager()
m := NewManager(log.NewNopLogger())
m.RegisterModule("test", nil)
m.RegisterModule("dep1", nil)
m.RegisterModule("dep2", nil)
Expand Down Expand Up @@ -210,7 +211,7 @@ func TestModuleWaitsForAllDependencies(t *testing.T) {
}, nil), nil
}

m := NewManager()
m := NewManager(log.NewNopLogger())
m.RegisterModule("A", initA)
m.RegisterModule("B", nil)
m.RegisterModule("C", initC)
Expand Down

0 comments on commit 184bffd

Please sign in to comment.