Create logger hierarchy for config and incoming / outgoing loggers #4240
Conversation
viambot
added
safe to test
viambot
added
safe to test
grpc/shared_conn.go
Outdated
| @@ -156,7 +156,7 @@ func (sc *SharedConn) ResetConn(conn rpc.ClientConn, moduleLogger logging.Logger | |||
| // The first call to `ResetConn` happens before anything can access `sc.logger`. So long as | |||
| // we never write to the member variable, everything can continue to access this without | |||
| // locks. | |||
| sc.logger = moduleLogger.Sublogger("conn") | |||
| sc.logger = moduleLogger.Sublogger("network_outgoing.conn") | |||
There was a problem hiding this comment.
From what I understand, this is where sc.logger gets initialized. I think it is valuable to keep using the moduleLogger for the module logger name prefix, but since this logger deals with outgoing network connections, we want to be able to configure this using the network_outgoing string as well. This deviates from the logging.GetOrNewLogger("rdk.network_outgoing") pattern slightly in order to keep the module name and still be configurable using e.g. *.network_outgoing.*. I am open to suggestions or reverting this change.
There was a problem hiding this comment.
Thanks for calling this out. So, logs from the shared conn to a module from the module manager will have the logger name: rdk.modmanager.[modulename].network_outgoing.conn? I think that's fine. I can't really think of a better option: rdk.network_outgoing.modmanager.module.conn would also be slightly inconvenient, as if a user tried to set rdk.modmanager.*, they'd miss this logger. Also tagging @dgottlieb for visibility on our changes here.
There was a problem hiding this comment.
correct, I also agree this is probably the best we can do for now
logging/net_appender.go
Outdated
| @@ -439,7 +439,7 @@ func CreateNewGRPCClient(ctx context.Context, cloudCfg *CloudConfig) (rpc.Client | |||
| dialOpts = append(dialOpts, rpc.WithInsecure()) | |||
| } | |||
|
|
|||
| return rpc.DialDirectGRPC(ctx, grpcURL.Host, NewLogger("netlogger"), dialOpts...) | |||
| return rpc.DialDirectGRPC(ctx, grpcURL.Host, GetOrNewLogger("rdk.network_outgoing.netlogger"), dialOpts...) | |||
There was a problem hiding this comment.
This logger is guaranteed to exist from the diff above, but this was the most straightforward way to retrieve it.
logging/logger_registry.go
Outdated
| func GetOrNewLogger(name string) (logger Logger) { | ||
| logger, ok := globalLoggerRegistry.loggerNamed(name) | ||
| if !ok { | ||
| return NewLogger(name) | ||
| } | ||
| return logger | ||
| } |
There was a problem hiding this comment.
I don't think there is a clear natural place to initialize a e.g. rdk.network_outgoing logger so this helper lets us safely retrieve and use the same logger object globally without fear of overwriting a map entry to a new logger with the same name.
viambot
added
safe to test
viambot
added
safe to test
danielbotros
changed the title
module/module.go
Outdated
| @@ -236,7 +236,7 @@ func NewModule(ctx context.Context, address string, logger logging.Logger) (*Mod | |||
| } | |||
|
|
|||
| // attempt to configure PeerConnection | |||
| pcReady, pcClosed, err := rpc.ConfigureForRenegotiation(pc, rpc.PeerRoleServer, logger) | |||
| pcReady, pcClosed, err := rpc.ConfigureForRenegotiation(pc, rpc.PeerRoleServer, logging.GetOrNewLogger("rdk.network_outgoing")) | |||
There was a problem hiding this comment.
some of these are a bit dubious as to whether or not they are incoming or outgoing network events in my opinion. we may want to consider renaming / reorganizing things but this PR tries to follow the topology as close as possible to the scope doc
There was a problem hiding this comment.
Why would we put this logger under rdk.network_outgoing? From what I can tell it's a logger used as part of connection establishment from a Go module back to the RDK. I think we should continue to use logger; that's the module's author's logger.
There was a problem hiding this comment.
discussed off PR, we are changing all network_outgoing / network_incoming to networking
viambot
added
safe to test
danielbotros
force-pushed
the
RSDK-8084-create-sublogger-for-config-logs
branch
from
7def9f1
to
92f30ea
Compare
viambot
added
safe to test
viambot
added
safe to test
Description
RSDK-8084: Add config sublogger for all config related log lines
RSDK-8091: Create and organize outgoing network logger and subloggers
RSDK-8092: Create and organize incoming network logger and subloggers