[RSDK 9149] Use RDK Logger Across Interceptors #405
Conversation
…if it's un upgrade
|
For security reasons, this PR must be labeled with |
viambot
added
safe to test
rpc/server_options.go
Outdated
There was a problem hiding this comment.
@dgottlieb @cheukt This, in addition to this, are the changes to stop the leaked goroutine in RDK testing from happening. The RDK PR cannot merge until this goutils PR does and the goutils version is bumped in RDK. Once this merges and goutils in RDK is bumped, the RDK tests will still fail (leak takes place) until the other PR merges.
rpc/server.go
Outdated
| @@ -404,6 +397,8 @@ func NewServer(logger utils.ZapCompatibleLogger, opts ...ServerOption) (Server, | |||
| serverOpts = append(serverOpts, grpc.StatsHandler(sOpts.statsHandler)) | |||
| } | |||
|
|
|||
| serverOpts = append(serverOpts, grpc.WaitForHandlers(sOpts.waitForHandlers)) | |||
There was a problem hiding this comment.
@dgottlieb another option is to just have grpcServer call GracefulStop() instead of Stop() in L864. It is a riskier change but also not sure why that Stop() wasn't graceful, when all of the other stops in the function are.
There was a problem hiding this comment.
Yeah, GracefulStop works too. I have no strong opinions on this. Just confirming: I expect all servers to be constructed from this NewServer call. Given that, I don't see any behavior difference between using an option vs GracefulStop.
viambot
added
safe to test
viambot
added
safe to test
| @@ -861,7 +856,7 @@ func (ss *simpleServer) Stop() error { | |||
| err = multierr.Combine(err, ss.signalingCallQueue.Close()) | |||
| } | |||
| ss.logger.Debug("stopping gRPC server") | |||
| defer ss.grpcServer.Stop() | |||
| defer ss.grpcServer.GracefulStop() | |||
There was a problem hiding this comment.
Was causing a goroutine leak in RDK testing.
viambot
added
safe to test
Stop converting the logger into a zap logger and instead use the native zap compatible logger. See pics below for visible change (the error log in between the highlighted lines is the same as what's referenced in the ticket).
Note
re: all the 'from here' comments..
To no longer use an ordinary zap logger, I replaced calls to grpc interceptor methods with calls to copies of those methods that I modify to use an RDK logger instead. An effect of this change was the additional copying over of all private types employed by these functions in the grpc library we use,, is there a better way about this? It seems like this PR used a similar tactic to get client interceptors to use the preferred logger.
Before:
After: notice here that the logger (from goutils/logger.go)is outputting logs that are not ERROR level or higher. This is not desired behavior. This has since been fixed; the original behavior has been restored.After:
Testing
Modify the the module.go RDK file in the simple module example custom resource so that the DoCommand() implementation always returns an error. Then, follow the instructions provided in the custom resources README.md file to see the logs output by the robot server.