Embedded server should not mess global loggers (by default) #12861
Conversation
In particular bring up zapgrpc V2 code: uber-go/zap@89e3820 https://pkg.go.dev/google.golang.org/grpc/grpclog#LoggerV2
So far each instance of embed server was overriding the grpc loggers and zap.global loggers. It's counter intutitive that last created Embedded server was 'wining' and more-over it was breaking grpc expectation to change it "only" before the grpc stack is being used. This PR introduces explicit call: `embed.Config::SetupGlobalLoggers()`, that changes the loggers where requested. The call is used by etcd main binary. The immediate benefit from this change is reduction of test flakiness, as there were flakes due to not a proper logger being used across tests.
ptabor
force-pushed
the
20210413-test-logging-fixes
branch
from
683ff4f
to
fad6391
Compare
server/embed/config_logging.go
Outdated
| "go.etcd.io/etcd/client/pkg/v3/logutil" | ||
| "go.uber.org/zap/zapgrpc" | ||
| "google.golang.org/grpc/grpclog" | ||
|
|
There was a problem hiding this comment.
Nit: Any reason for a new line here?
| // The method is not executed by embed server by default (since 3.5) to | ||
| // enable setups where grpc/zap.Global logging is configured independently | ||
| // or spans separate lifecycle (like in tests). | ||
| func (cfg *Config) SetupGlobalLoggers() { |
There was a problem hiding this comment.
A small future refactor suggestion: Since this is no longer called by embed by default should we move it to be closer to the etcdmain package instead? Or is there maybe a reason we want to keep it here?
There was a problem hiding this comment.
All direct users of Embed servers might want to call that API in similar way to how etcd does.
I decided to change existing behavior, but I think we should make the costs of getting previous behavior as low as possible.
To some degree Embed server is (the only) part of public API of go.etcd.io/etcd/server module.
server/etcdmain/etcd.go
Outdated
| @@ -57,6 +57,8 @@ func startEtcdOrProxyV2(args []string) { | |||
|
|
|||
| err := cfg.parse(args[1:]) | |||
| lg := cfg.ec.GetLogger() | |||
| cfg.ec.SetupGlobalLoggers() | |||
There was a problem hiding this comment.
Nit: What do you think, should this maybe be moved before getting the logger, so to line 59:
cfg.ec.SetupGlobalLoggers()
lg := cfg.ec.GetLogger()
There was a problem hiding this comment.
I moved it to the later place of the file, so make it just after error handling (see the other comment)
| @@ -57,6 +57,8 @@ func startEtcdOrProxyV2(args []string) { | |||
|
|
|||
| err := cfg.parse(args[1:]) | |||
There was a problem hiding this comment.
I wonder why this error is only checked on line 70.
There was a problem hiding this comment.
If think the whole logic says:
If we failed to parse the whole configuration, print the error using preferable the resolved logger from the config, but if does not exists, create a new temporary logger.
There was a problem hiding this comment.
Thank you for the explanation!
| ) | ||
| m.Close() |
|
Thank you for review ! |
Give control to Embedded servers whether they override global loggers
So far each instance of embed server was overriding the grpc loggers and zap.global loggers.
It's counter intutitive that last created Embedded server was 'wining' and more-over it was breaking grpc expectation to change it "only" before the grpc stack is being used.
This PR introduces explicit call:
embed.Config::SetupGlobalLoggers(), that changes the loggers where requested. The call is used by etcd main binary.The immediate benefit from this change is lower test flakiness, as there were flakes due to not-proper logger usage across tests.