Skip to content

Commit

Permalink
Use a centralised logger interface
Browse files Browse the repository at this point in the history 
This standardises all of the logging in juju to one and only one
interface. This will ensure that when we implement status history
that we can guarantee that the logging is correctly nested and
can pass labels through.

Currently loggo is the backing for logger.Logger, but it should
be really easy to just change this to slog if so desired.
  • Loading branch information
@SimonRichardson
SimonRichardson committed May 9, 2024
1 parent 6cfce0f commit 3b4a68d
Show file tree
Hide file tree
Showing 822 changed files with 4,461 additions and 5,260 deletions.
8 changes: 2 additions & 6 deletions agent/addons/addons.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/juju/worker/v4/dependency"
"github.com/prometheus/client_golang/prometheus"

"github.com/juju/juju/core/logger"
"github.com/juju/juju/core/machinelock"
"github.com/juju/juju/core/presence"
"github.com/juju/juju/internal/worker/introspection"
Expand All @@ -25,11 +26,6 @@ type MetricSink interface {
Unregister() bool
}

// Logger is the interface that the introspection worker uses to log details.
type Logger interface {
Debugf(string, ...any)
}

// DefaultIntrospectionSocketName returns the socket name to use for the
// abstract domain socket that the introspection worker serves requests
// over.
Expand All @@ -50,7 +46,7 @@ type IntrospectionConfig struct {
Clock clock.Clock
LocalHub introspection.SimpleHub
CentralHub introspection.StructuredHub
Logger Logger
Logger logger.Logger

NewSocketName func(names.Tag) string
WorkerFunc func(config introspection.Config) (worker.Worker, error)
Expand Down
7 changes: 4 additions & 3 deletions agent/addons/addons_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
gc "gopkg.in/check.v1"

"github.com/juju/juju/agent/addons"
loggertesting "github.com/juju/juju/internal/logger/testing"
"github.com/juju/juju/internal/worker/introspection"
coretesting "github.com/juju/juju/testing"
)
Expand All @@ -41,7 +42,7 @@ func (s *introspectionSuite) TestStartNonLinux(c *gc.C) {
started = true
return nil, errors.New("shouldn't call start")
},
Logger: coretesting.CheckLogger{Log: c},
Logger: loggertesting.WrapCheckLog(c),
}

err := addons.StartIntrospection(cfg)
Expand All @@ -60,7 +61,7 @@ func (s *introspectionSuite) TestStartError(c *gc.C) {
WorkerFunc: func(_ introspection.Config) (worker.Worker, error) {
return nil, errors.New("boom")
},
Logger: coretesting.CheckLogger{Log: c},
Logger: loggertesting.WrapCheckLog(c),
}

err := addons.StartIntrospection(cfg)
Expand Down Expand Up @@ -93,7 +94,7 @@ func (s *introspectionSuite) TestStartSuccess(c *gc.C) {
fake.config = cfg
return fake, nil
},
Logger: coretesting.CheckLogger{Log: c},
Logger: loggertesting.WrapCheckLog(c),
}

err = addons.StartIntrospection(cfg)
Expand Down
19 changes: 4 additions & 15 deletions agent/agentbootstrap/bootstrap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

"github.com/juju/clock"
"github.com/juju/errors"
"github.com/juju/loggo/v2"
"github.com/juju/mgo/v3"
"github.com/juju/names/v5"

Expand All @@ -22,6 +21,7 @@ import (
"github.com/juju/juju/core/credential"
coredatabase "github.com/juju/juju/core/database"
"github.com/juju/juju/core/instance"
"github.com/juju/juju/core/logger"
"github.com/juju/juju/core/model"
corenetwork "github.com/juju/juju/core/network"
coreos "github.com/juju/juju/core/os"
Expand Down Expand Up @@ -57,21 +57,10 @@ type DqliteInitializerFunc func(
ctx stdcontext.Context,
mgr database.BootstrapNodeManager,
modelUUID model.UUID,
logger database.Logger,
logger logger.Logger,
options ...database.BootstrapOpt,
) error

// Logger describes methods for emitting log output.
type Logger interface {
Errorf(string, ...any)
Warningf(string, ...any)
Debugf(string, ...any)
Infof(string, ...any)

// Logf is used to proxy Dqlite logs via this b.logger.
Logf(level loggo.Level, msg string, args ...any)
}

// ProviderFunc is a function that returns an EnvironProvider.
type ProviderFunc func(string) (environs.EnvironProvider, error)

Expand Down Expand Up @@ -109,7 +98,7 @@ type AgentBootstrap struct {
// StorageProviderRegistry is used to determine and store the
// details of the default storage pools.
storageProviderRegistry storage.ProviderRegistry
logger Logger
logger logger.Logger
}

// AgentBootstrapArgs are the arguments to NewAgentBootstrap that are required
Expand All @@ -126,7 +115,7 @@ type AgentBootstrapArgs struct {
StorageProviderRegistry storage.ProviderRegistry
BootstrapDqlite DqliteInitializerFunc
Provider ProviderFunc
Logger Logger
Logger logger.Logger

// Deprecated: use InstancePrechecker
StateNewPolicy state.NewPolicyFunc
Expand Down
12 changes: 7 additions & 5 deletions agent/agentbootstrap/bootstrap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
corebase "github.com/juju/juju/core/base"
"github.com/juju/juju/core/constraints"
"github.com/juju/juju/core/instance"
"github.com/juju/juju/core/logger"
"github.com/juju/juju/core/model"
corenetwork "github.com/juju/juju/core/network"
"github.com/juju/juju/environs"
Expand All @@ -29,6 +30,7 @@ import (
"github.com/juju/juju/internal/charmhub"
"github.com/juju/juju/internal/cloudconfig/instancecfg"
"github.com/juju/juju/internal/database"
loggertesting "github.com/juju/juju/internal/logger/testing"
"github.com/juju/juju/internal/mongo"
"github.com/juju/juju/internal/mongo/mongotest"
"github.com/juju/juju/internal/network"
Expand Down Expand Up @@ -182,7 +184,7 @@ func (s *bootstrapSuite) TestInitializeState(c *gc.C) {
c.Assert(t, gc.Equals, "dummy")
return &envProvider, nil
},
Logger: testing.NewCheckLogger(c),
Logger: loggertesting.WrapCheckLog(c),
InstancePrecheckerGetter: func(s *state.State) (environs.InstancePrechecker, error) {
return state.NoopInstancePrechecker{}, nil
},
Expand Down Expand Up @@ -356,7 +358,7 @@ func (s *bootstrapSuite) TestInitializeStateWithStateServingInfoNotAvailable(c *
SharedSecret: "abc123",
StorageProviderRegistry: provider.CommonStorageProviders(),
BootstrapDqlite: bootstrapDqliteWithDummyCloudType,
Logger: testing.NewCheckLogger(c),
Logger: loggertesting.WrapCheckLog(c),
InstancePrecheckerGetter: func(s *state.State) (environs.InstancePrechecker, error) {
return state.NoopInstancePrechecker{}, nil
},
Expand Down Expand Up @@ -435,7 +437,7 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) {
Provider: func(t string) (environs.EnvironProvider, error) {
return &fakeProvider{}, nil
},
Logger: testing.NewCheckLogger(c),
Logger: loggertesting.WrapCheckLog(c),
InstancePrecheckerGetter: func(s *state.State) (environs.InstancePrechecker, error) {
return state.NoopInstancePrechecker{}, nil
},
Expand All @@ -458,7 +460,7 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) {
SharedSecret: "baz",
StorageProviderRegistry: provider.CommonStorageProviders(),
BootstrapDqlite: database.BootstrapDqlite,
Logger: testing.NewCheckLogger(c),
Logger: loggertesting.WrapCheckLog(c),
InstancePrecheckerGetter: func(s *state.State) (environs.InstancePrechecker, error) {
return state.NoopInstancePrechecker{}, nil
},
Expand Down Expand Up @@ -563,7 +565,7 @@ func bootstrapDqliteWithDummyCloudType(
ctx context.Context,
mgr database.BootstrapNodeManager,
modelUUID model.UUID,
logger database.Logger,
logger logger.Logger,
opts ...database.BootstrapOpt,
) error {
// The dummy cloud type needs to be inserted before the other operations.
Expand Down
18 changes: 6 additions & 12 deletions agent/errors/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,10 @@ import (
"github.com/juju/version/v2"

"github.com/juju/juju/agent/tools"
"github.com/juju/juju/core/logger"
jworker "github.com/juju/juju/internal/worker"
)

// Logger represents the logging methods used by this package.
type Logger interface {
Debugf(string, ...interface{})
Infof(string, ...interface{})
Errorf(string, ...interface{})
}

// UpgradeReadyError is returned by an Upgrader to report that
// an upgrade is ready to be performed and a restart is due.
type UpgradeReadyError struct {
Expand All @@ -42,7 +36,7 @@ func (e *UpgradeReadyError) Error() string {
// ChangeAgentTools does the actual agent upgrade.
// It should be called just before an agent exits, so that
// it will restart running the new tools.
func (e *UpgradeReadyError) ChangeAgentTools(logger Logger) error {
func (e *UpgradeReadyError) ChangeAgentTools(logger logger.Logger) error {
agentTools, err := tools.ChangeAgentTools(e.DataDir, e.AgentName, e.NewTools)
if err != nil {
return err
Expand Down Expand Up @@ -116,7 +110,7 @@ type Breakable interface {
// isFatal argument to worker.NewRunner, that diagnoses an error as
// fatal if the connection has failed or if the error is otherwise
// fatal.
func ConnectionIsFatal(ctx context.Context, logger Logger, conns ...Breakable) func(err error) bool {
func ConnectionIsFatal(ctx context.Context, logger logger.Logger, conns ...Breakable) func(err error) bool {
return func(err error) bool {
if IsFatal(err) {
return true
Expand All @@ -131,7 +125,7 @@ func ConnectionIsFatal(ctx context.Context, logger Logger, conns ...Breakable) f
}

// ConnectionIsDead returns true if the given Breakable is broken.
var ConnectionIsDead = func(ctx context.Context, logger Logger, conn Breakable) bool {
var ConnectionIsDead = func(ctx context.Context, logger logger.Logger, conn Breakable) bool {
return conn.IsBroken(ctx)
}

Expand All @@ -150,7 +144,7 @@ type Pinger interface {
// actually quite a nice idea).
// 2. The dependency engine conversion is completed for the machine
// agent.
func PingerIsFatal(logger Logger, conns ...Pinger) func(err error) bool {
func PingerIsFatal(logger logger.Logger, conns ...Pinger) func(err error) bool {
return func(err error) bool {
if IsFatal(err) {
return true
Expand All @@ -165,7 +159,7 @@ func PingerIsFatal(logger Logger, conns ...Pinger) func(err error) bool {
}

// PingerIsDead returns true if the given pinger fails to ping.
var PingerIsDead = func(logger Logger, conn Pinger) bool {
var PingerIsDead = func(logger logger.Logger, conn Pinger) bool {
if err := conn.Ping(); err != nil {
logger.Infof("error pinging %T: %v", conn, err)
return true
Expand Down
6 changes: 4 additions & 2 deletions agent/errors/errors_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
gc "gopkg.in/check.v1"

agenterrors "github.com/juju/juju/agent/errors"
"github.com/juju/juju/core/logger"
loggertesting "github.com/juju/juju/internal/logger/testing"
"github.com/juju/juju/internal/worker"
"github.com/juju/juju/rpc/params"
"github.com/juju/juju/testing"
Expand All @@ -24,12 +26,12 @@ var (

type toolSuite struct {
testing.BaseSuite
logger agenterrors.Logger
logger logger.Logger
}

func (s *toolSuite) SetUpTest(c *gc.C) {
s.BaseSuite.SetUpTest(c)
s.logger = testing.CheckLogger{Log: c}
s.logger = loggertesting.WrapCheckLog(c)
}

func (*toolSuite) TestErrorImportance(c *gc.C) {
Expand Down
5 changes: 3 additions & 2 deletions api/apiclient_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
apiservererrors "github.com/juju/juju/apiserver/errors"
apiservertesting "github.com/juju/juju/apiserver/testing"
"github.com/juju/juju/core/network"
loggertesting "github.com/juju/juju/internal/logger/testing"
proxy "github.com/juju/juju/internal/proxy/config"
"github.com/juju/juju/rpc"
"github.com/juju/juju/rpc/jsoncodec"
Expand Down Expand Up @@ -1406,7 +1407,7 @@ func (s *apiclientSuite) TestWatchDebugLogParamsEncoded(c *gc.C) {
conn, err := api.Open(info, api.DialOpts{})
c.Assert(err, jc.ErrorIsNil)
defer conn.Close()
client := apiclient.NewClient(conn, jtesting.NoopLogger{})
client := apiclient.NewClient(conn, loggertesting.WrapCheckLog(c))
_, err = client.WatchDebugLog(params)
c.Assert(err, jc.ErrorIsNil)

Expand All @@ -1422,7 +1423,7 @@ func (s *apiclientSuite) TestWatchDebugLogConnected(c *gc.C) {
conn, err := api.Open(info, api.DialOpts{})
c.Assert(err, jc.ErrorIsNil)
defer conn.Close()
cl := apiclient.NewClient(conn, jtesting.NoopLogger{})
cl := apiclient.NewClient(conn, loggertesting.WrapCheckLog(c))
// Use the no tail option so we don't try to start a tailing cursor
// on the oplog when there is no oplog configured in mongo as the tests
// don't set up mongo in replicaset mode.
Expand Down
10 changes: 3 additions & 7 deletions api/client/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/juju/juju/api/base"
"github.com/juju/juju/api/client/storage"
"github.com/juju/juju/api/common"
"github.com/juju/juju/core/logger"
"github.com/juju/juju/core/model"
"github.com/juju/juju/core/status"
"github.com/juju/juju/internal/tools"
Expand All @@ -31,22 +32,17 @@ type Option = base.Option
// supplied tracer.
var WithTracer = base.WithTracer

// Logger is the interface used by the client to log errors.
type Logger interface {
Errorf(string, ...interface{})
}

// Client represents the client-accessible part of the state.
type Client struct {
base.ClientFacade
facade base.FacadeCaller
conn api.Connection
logger Logger
logger logger.Logger
}

// NewClient returns an object that can be used to access client-specific
// functionality.
func NewClient(c api.Connection, logger Logger, options ...Option) *Client {
func NewClient(c api.Connection, logger logger.Logger, options ...Option) *Client {
frontend, backend := base.NewClientFacade(c, "Client", options...)
return &Client{
ClientFacade: frontend,
Expand Down
9 changes: 5 additions & 4 deletions apiserver/admin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/juju/juju/core/permission"
"github.com/juju/juju/domain/access/service"
"github.com/juju/juju/internal/auth"
loggertesting "github.com/juju/juju/internal/logger/testing"
"github.com/juju/juju/internal/password"
"github.com/juju/juju/internal/uuid"
jujutesting "github.com/juju/juju/juju/testing"
Expand Down Expand Up @@ -198,7 +199,7 @@ func (s *loginSuite) TestLoginAsDeactivatedUser(c *gc.C) {
Code: "unauthorized access",
})

_, err = apiclient.NewClient(st, coretesting.NoopLogger{}).Status(nil)
_, err = apiclient.NewClient(st, loggertesting.WrapCheckLog(c)).Status(nil)
c.Assert(err, gc.NotNil)
c.Check(errors.Is(err, errors.NotImplemented), jc.IsTrue)
c.Check(strings.Contains(err.Error(), `unknown facade type "Client"`), jc.IsTrue)
Expand Down Expand Up @@ -230,7 +231,7 @@ func (s *loginSuite) TestLoginAsDeletedUser(c *gc.C) {
Code: "unauthorized access",
})

_, err = apiclient.NewClient(st, coretesting.NoopLogger{}).Status(nil)
_, err = apiclient.NewClient(st, loggertesting.WrapCheckLog(c)).Status(nil)
c.Assert(err, gc.NotNil)
c.Check(errors.Is(err, errors.NotImplemented), jc.IsTrue)
c.Check(strings.Contains(err.Error(), `unknown facade type "Client"`), jc.IsTrue)
Expand Down Expand Up @@ -1017,7 +1018,7 @@ func (s *migrationSuite) TestImportingModel(c *gc.C) {
// Users should be able to log in but RPC requests should fail.
userConn := s.OpenControllerModelAPI(c)
defer userConn.Close()
_, err = apiclient.NewClient(userConn, coretesting.NoopLogger{}).Status(nil)
_, err = apiclient.NewClient(userConn, loggertesting.WrapCheckLog(c)).Status(nil)
c.Check(err, gc.ErrorMatches, "migration in progress, model is importing")

// Machines should be able to use the API.
Expand All @@ -1037,7 +1038,7 @@ func (s *migrationSuite) TestExportingModel(c *gc.C) {
defer userConn.Close()

// Status is fine.
_, err = apiclient.NewClient(userConn, coretesting.NoopLogger{}).Status(nil)
_, err = apiclient.NewClient(userConn, loggertesting.WrapCheckLog(c)).Status(nil)
c.Check(err, jc.ErrorIsNil)

// Modifying commands like destroy machines are not.
Expand Down
Loading

0 comments on commit 3b4a68d

Please sign in to comment.