Settable grpc logger #402
Settable grpc logger #402
Conversation
ptabor
force-pushed
the
20210315-settable-logger
branch
3 times, most recently
from
22818d0
to
7fc9235
Compare
There was a problem hiding this comment.
Minor comment on the docs license header, but generally this LGTM. Could you add a section to the README as well? We're also in the process of moving to v2, so if you want you may consider cherry picking this against v2 as well (some of the interfaces have changed though).
The go-grpc assumes that logger can be only configured once as the `SetLoggerV2` method is: ```Not mutex-protected, should be called before any gRPC functions.`` This is insufficient in case of e.g. "testing" scenarios where loggers might need to be supplied on per-test-case basis.
The settable_test.go is a good example of integration of testing harness with grpclogging using zaptest, that is canonical usecase for this infrastructure.
ptabor
force-pushed
the
20210315-settable-logger
branch
from
7fc9235
to
91bfafe
Compare
Codecov Report
@@ Coverage Diff @@
## master #402 +/- ##
==========================================
- Coverage 73.64% 73.31% -0.33%
==========================================
Files 41 42 +1
Lines 1324 1353 +29
==========================================
+ Hits 975 992 +17
- Misses 295 307 +12
Partials 54 54
Continue to review full report at Codecov.
|
|
Thanks for your contribution! Could you please cherry pick this to the v2 branch? I'm not sure how many changes will be required though. |
|
PTAL. From quick glance, v2 is completely missing functionality of setting loggers as default grpc implementation |
|
Yeah we might've removed it, it's been a while since I looked at it TBH. |
grpc_logsettable contains a thread-safe wrapper around grpc-logging infrastructure.
Go-grpc library assumes that logger can be only configured once as the
SetLoggerV2method is:Not mutex-protected, should be called before any gRPC functions.This provided package allows to supply parent logger once ("before any grpc"), but after change underlying implementation in thread-safe way when needed.
It's in particular useful for testing, where each testcase might need its own logger.
settable_test.go shows e2e usage of the enhanced infrastructure.
Case study:
In etcd we found it challenging to properly configure grpc logging in scope of the test framework.
When test fails we want to get all relevant logs from the test-case alone.
ZAP allows to wrap testing.T with zapttest.Logger. This needs to happen in each test-case.
When we try to use that zap-logger as grpc logger, we observe following RACEs:
The fact that server is property shutdown (GracefulStop + Stop), does not mitigate the reality that there is no synchronization between the connection goroutines and the next test-case start in terms of 'logger' memory.
The supplied implementation allows to configure it 'properly', on the cost of additional layer of indirection in the tests only.