JakeHillion/storj
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

don't use global loggers (#2671)

Browse Source 
* pkg/server: don't use global logger
* satellite/overlay: use correct logger
* pkg/kademlia: use correct logger
* linksharing: use conventional way to pass in logger
* use zaptest in tests
This commit is contained in:
Egon Elbre 2019-07-31 15:09:45 +03:00 committed by GitHub
parent 9ba8b53ed5
commit ec3d5c0bdd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 43 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
bootstrap/peer.go
View File

@ -111,7 +111,7 @@ func New(log *zap.Logger, full *identity.FullIdentity, db DB, config Config, ver
peer.Transport = transport.NewClient(options)
peer.Server, err = server.New(options, sc.Address, sc.PrivateAddress, nil)
peer.Server, err = server.New(log.Named("server"), options, sc.Address, sc.PrivateAddress, nil)
if err != nil {
return nil, errs.Combine(err, peer.Close())
}

2
cmd/certificates/main.go
View File

@ -59,7 +59,7 @@ func cmdRun(cmd *cobra.Command, args []string) error {
zap.S().Fatal(err)
}
return config.Server.Run(ctx, identity, nil, config.Signer)
return config.Server.Run(ctx, zap.L(), identity, nil, config.Signer)
}
func main() {

3
cmd/linksharing/main.go
View File

@ -76,8 +76,7 @@ func cmdRun(cmd *cobra.Command, args []string) (err error) {
return err
}
handler, err := linksharing.NewHandler(linksharing.HandlerConfig{
Log: log,
handler, err := linksharing.NewHandler(log, linksharing.HandlerConfig{
Uplink: uplink,
URLBase: runCfg.PublicURL,
})

2
internal/testplanet/uplink_test.go
View File

@ -216,7 +216,7 @@ func TestDownloadFromUnresponsiveNode(t *testing.T) {
})
require.NoError(t, err)
server, err := server.New(options, storageNode.Addr(), storageNode.PrivateAddr(), nil)
server, err := server.New(storageNode.Log.Named("mock-server"), options, storageNode.Addr(), storageNode.PrivateAddr(), nil)
require.NoError(t, err)
pb.RegisterPiecestoreServer(server.GRPC(), &piecestoreMock{})
go func() {

11
linksharing/handler.go
View File

@ -27,9 +27,6 @@ var (
// HandlerConfig specifies the handler configuration
type HandlerConfig struct {
// Log is a logger used for logging
Log *zap.Logger
// Uplink is the uplink used to talk to the storage network
Uplink *uplink.Uplink
@ -46,11 +43,7 @@ type Handler struct {
}
// NewHandler creates a new link sharing HTTP handler
func NewHandler(config HandlerConfig) (*Handler, error) {
if config.Log == nil {
config.Log = zap.L()
}
func NewHandler(log *zap.Logger, config HandlerConfig) (*Handler, error) {
if config.Uplink == nil {
return nil, errs.New("uplink is required")
}
@ -61,7 +54,7 @@ func NewHandler(config HandlerConfig) (*Handler, error) {
}
return &Handler{
log: config.Log,
log: log,
uplink: config.Uplink,
urlBase: urlBase,
}, nil

5
linksharing/handler_test.go
View File

@ -99,7 +99,7 @@ func TestNewHandler(t *testing.T) {
testCase := testCase
t.Run(testCase.name, func(t *testing.T) {
handler, err := NewHandler(testCase.config)
handler, err := NewHandler(zaptest.NewLogger(t), testCase.config)
if testCase.err != "" {
require.EqualError(t, err, testCase.err)
return
@ -253,8 +253,7 @@ func testHandlerRequests(t *testing.T, ctx *testcontext.Context, planet *testpla
uplink := newUplink(ctx, t)
defer ctx.Check(uplink.Close)
handler, err := NewHandler(HandlerConfig{
Log: zaptest.NewLogger(t),
handler, err := NewHandler(zaptest.NewLogger(t), HandlerConfig{
Uplink: uplink,
URLBase: "http://localhost",
})

6
pkg/certificates/certificates_test.go
View File

@ -17,7 +17,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/zeebo/errs"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/peer"
@ -609,7 +609,7 @@ func TestCertificateSigner_Sign_E2E(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, serverOpts)
service, err := server.New(serverOpts, sc.Address, sc.PrivateAddress, nil, config)
service, err := server.New(zaptest.NewLogger(t), serverOpts, sc.Address, sc.PrivateAddress, nil, config)
require.NoError(t, err)
require.NotNil(t, service)
@ -770,7 +770,7 @@ func TestCertificateSigner_Sign(t *testing.T) {
}
peerCtx := peer.NewContext(ctx, grpcPeer)
certSigner := NewServer(zap.L(), signer, authDB, 0)
certSigner := NewServer(zaptest.NewLogger(t), signer, authDB, 0)
req := pb.SigningRequest{
Timestamp: time.Now().Unix(),
AuthToken: auths[0].Token.String(),

4
pkg/kademlia/routing.go
View File

@ -314,7 +314,7 @@ func (rt *RoutingTable) iterateNodes(ctx context.Context, start storj.NodeID, f
func (rt *RoutingTable) ConnFailure(ctx context.Context, node *pb.Node, err error) {
err2 := rt.ConnectionFailed(ctx, node)
if err2 != nil {
zap.L().Debug(fmt.Sprintf("error with ConnFailure hook %+v : %+v", err, err2))
rt.log.Debug(fmt.Sprintf("error with ConnFailure hook %+v : %+v", err, err2))
}
}
@ -322,6 +322,6 @@ func (rt *RoutingTable) ConnFailure(ctx context.Context, node *pb.Node, err erro
func (rt *RoutingTable) ConnSuccess(ctx context.Context, node *pb.Node) {
err := rt.ConnectionSuccess(ctx, node)
if err != nil {
zap.L().Debug("connection success error:", zap.Error(err))
rt.log.Debug("connection success error:", zap.Error(err))
}
}

4
pkg/server/config.go
View File

@ -23,7 +23,7 @@ type Config struct {
}
// Run will run the given responsibilities with the configured identity.
func (sc Config) Run(ctx context.Context, identity *identity.FullIdentity, interceptor grpc.UnaryServerInterceptor, services ...Service) (err error) {
func (sc Config) Run(ctx context.Context, log *zap.Logger, identity *identity.FullIdentity, interceptor grpc.UnaryServerInterceptor, services ...Service) (err error) {
defer mon.Task()(&ctx)(&err)
opts, err := tlsopts.NewOptions(identity, sc.Config)
@ -32,7 +32,7 @@ func (sc Config) Run(ctx context.Context, identity *identity.FullIdentity, inter
}
defer func() { err = errs.Combine(err, opts.RevDB.Close()) }()
server, err := New(opts, sc.Address, sc.PrivateAddress, interceptor, services...)
server, err := New(log.Named("server"), opts, sc.Address, sc.PrivateAddress, interceptor, services...)
if err != nil {
return err
}

9
pkg/server/interceptors.go
View File

@ -19,7 +19,7 @@ import (
"storj.io/storj/storage"
)
func logOnErrorStreamInterceptor(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) (err error) {
func (server *Server) logOnErrorStreamInterceptor(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) (err error) {
err = handler(srv, ss)
if err != nil {
// no zap errors for canceled or wrong file downloads
@ -29,20 +29,19 @@ func logOnErrorStreamInterceptor(srv interface{}, ss grpc.ServerStream, info *gr
err == io.EOF {
return err
}
zap.S().Errorf("%+v", err)
server.log.Sugar().Errorf("%+v", err)
}
return err
}
func logOnErrorUnaryInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{},
err error) {
func (server *Server) logOnErrorUnaryInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
resp, err = handler(ctx, req)
if err != nil {
// no zap errors for wrong file downloads
if status.Code(err) == codes.NotFound {
return resp, err
}
zap.S().Errorf("%+v", err)
server.log.Sugar().Errorf("%+v", err)
}
return resp, err
}

25
pkg/server/server.go
View File

@ -8,6 +8,7 @@ import (
"net"
"github.com/zeebo/errs"
"go.uber.org/zap"
"golang.org/x/sync/errgroup"
"google.golang.org/grpc"
@ -35,6 +36,7 @@ type private struct {
// Server represents a bundle of services defined by a specific ID.
// Examples of servers are the satellite, the storagenode, and the uplink.
type Server struct {
log *zap.Logger
public public
private private
next []Service
@ -43,8 +45,14 @@ type Server struct {
// New creates a Server out of an Identity, a net.Listener,
// a UnaryServerInterceptor, and a set of services.
func New(opts *tlsopts.Options, publicAddr, privateAddr string, interceptor grpc.UnaryServerInterceptor, services ...Service) (*Server, error) {
unaryInterceptor := logOnErrorUnaryInterceptor
func New(log *zap.Logger, opts *tlsopts.Options, publicAddr, privateAddr string, interceptor grpc.UnaryServerInterceptor, services ...Service) (*Server, error) {
server := &Server{
log: log,
next: services,
identity: opts.Ident,
}
unaryInterceptor := server.logOnErrorUnaryInterceptor
if interceptor != nil {
unaryInterceptor = CombineInterceptors(unaryInterceptor, interceptor)
}
@ -53,10 +61,10 @@ func New(opts *tlsopts.Options, publicAddr, privateAddr string, interceptor grpc
if err != nil {
return nil, err
}
public := public{
server.public = public{
listener: publicListener,
grpc: grpc.NewServer(
grpc.StreamInterceptor(logOnErrorStreamInterceptor),
grpc.StreamInterceptor(server.logOnErrorStreamInterceptor),
grpc.UnaryInterceptor(unaryInterceptor),
opts.ServerOption(),
),
@ -66,17 +74,12 @@ func New(opts *tlsopts.Options, publicAddr, privateAddr string, interceptor grpc
if err != nil {
return nil, errs.Combine(err, publicListener.Close())
}
private := private{
server.private = private{
listener: privateListener,
grpc: grpc.NewServer(),
}
return &Server{
public: public,
private: private,
next: services,
identity: opts.Ident,
}, nil
return server, nil
}
// Identity returns the server's identity

4
pkg/transport/transport_test.go
View File

@ -11,7 +11,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
"storj.io/storj/internal/testcontext"
"storj.io/storj/internal/testidentity"
@ -188,7 +188,7 @@ func TestDialNode_BadServerCertificate(t *testing.T) {
defer ctx.Cleanup()
planet, err := testplanet.NewCustom(
zap.L(),
zaptest.NewLogger(t),
testplanet.Config{
SatelliteCount: 0,
StorageNodeCount: 2,

6
satellite/accounting/live/live_test.go
View File

@ -11,7 +11,7 @@ import (
"github.com/skyrings/skyring-common/tools/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest"
"golang.org/x/sync/errgroup"
"storj.io/storj/internal/testrand"
@ -26,7 +26,7 @@ func TestPlainMemoryLiveAccounting(t *testing.T) {
config := Config{
StorageBackend: "plainmemory:",
}
service, err := New(zap.L().Named("live-accounting"), config)
service, err := New(zaptest.NewLogger(t).Named("live-accounting"), config)
require.NoError(t, err)
// ensure we are using the expected underlying type
@ -83,7 +83,7 @@ func TestResetTotals(t *testing.T) {
config := Config{
StorageBackend: "plainmemory:",
}
service, err := New(zap.L().Named("live-accounting"), config)
service, err := New(zaptest.NewLogger(t).Named("live-accounting"), config)
require.NoError(t, err)
// ensure we are using the expected underlying type

6
satellite/overlay/cache.go
View File

@ -372,7 +372,7 @@ func (cache *Cache) ConnFailure(ctx context.Context, node *pb.Node, failureError
// it differently.
_, err = cache.db.UpdateUptime(ctx, node.Id, false, lambda, weight, uptimeDQ)
if err != nil {
zap.L().Debug("error updating uptime for node", zap.Error(err))
cache.log.Debug("error updating uptime for node", zap.Error(err))
}
}
@ -383,7 +383,7 @@ func (cache *Cache) ConnSuccess(ctx context.Context, node *pb.Node) {
err = cache.Put(ctx, node.Id, *node)
if err != nil {
zap.L().Debug("error updating uptime for node", zap.Error(err))
cache.log.Debug("error updating uptime for node", zap.Error(err))
}
lambda := cache.preferences.UptimeReputationLambda
@ -392,7 +392,7 @@ func (cache *Cache) ConnSuccess(ctx context.Context, node *pb.Node) {
_, err = cache.db.UpdateUptime(ctx, node.Id, true, lambda, weight, uptimeDQ)
if err != nil {
zap.L().Debug("error updating node connection info", zap.Error(err))
cache.log.Debug("error updating node connection info", zap.Error(err))
}
}

2
satellite/peer.go
View File

@ -262,7 +262,7 @@ func New(log *zap.Logger, full *identity.FullIdentity, db DB, config *Config, ve
if sc.DebugLogTraffic {
unaryInterceptor = server.CombineInterceptors(unaryInterceptor, server.UnaryMessageLoggingInterceptor(log))
}
peer.Server, err = server.New(options, sc.Address, sc.PrivateAddress, unaryInterceptor)
peer.Server, err = server.New(log.Named("server"), options, sc.Address, sc.PrivateAddress, unaryInterceptor)
if err != nil {
return nil, errs.Combine(err, peer.Close())
}

2
storagenode/peer.go
View File

@ -165,7 +165,7 @@ func New(log *zap.Logger, full *identity.FullIdentity, db DB, config Config, ver
peer.Transport = transport.NewClient(options)
peer.Server, err = server.New(options, sc.Address, sc.PrivateAddress, nil)
peer.Server, err = server.New(log.Named("server"), options, sc.Address, sc.PrivateAddress, nil)
if err != nil {
return nil, errs.Combine(err, peer.Close())
}