Add support for grpclog.LoggerV2 #881
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #881 +/- ##
==========================================
+ Coverage 98.21% 98.26% +0.05%
==========================================
Files 43 44 +1
Lines 1904 1965 +61
==========================================
+ Hits 1870 1931 +61
Misses 27 27
Partials 7 7
Continue to review full report at Codecov.
|
|
I don't see how coverage can be improved further. The only two lines that are not executed are the ones that call the real |
|
Hm, fine. I assumed you wanted it because #534 was open for 3 years and nobody closed it, and the PR too. |
Apologies for leaving the task open, I didn't realize how much of an API change it would result in. |
|
@prashantv I'm sorry, what API change are you talking about? This PR add a few methods to this package only to satisfy the interface gRPC wants. |
|
By API change, I mean the additional methods that are added to the Sugar logger. The number of methods for each level increases from the 3 per-level (which all behave differently) to 4 per-level (where the Ln suffixed methods behave the same as one of the other methods). Once an API is added, it must be maintained for backwards compatibility, and I don't think think this added API surface makes sense to add directly to the zap Sugar logger. |
But this PR does not add any methods to the Sugar logger? |
|
For some reason, I thought we were modifying the Sugar logger, which was a misread on my part. Given this doesn't add methods to the sugar logger, but to the gRPC-specific logger, I think this is reasonable, reopening for review. Sorry for the confusion @ash2k |
There was a problem hiding this comment.
The WithDebug option still makes sense for the V1 logger Print* methods, but not for the V2 logger. We can either:
- clarify in the documentation for
WithDebug - Add a separate constructor + type for the V2 logger (which takes a separate option type)
The latter increases the API surface, but may be clearer (especially if there's other options in future that will only be applicable to V2).
I don't have strong opinions on either approach though, thoughts @ash2k?
I'd go for this option as it's less work and does not add API surface - this can always be done later if needed but cannot be undone, so I think it's better to keep API as is. Could you suggest what should be documented there? That the option is only applicable to v1 API? |
Yep, specifically that |
|
@prashantv done |
ash2k
force-pushed
the
ash2k/grpc-logger
branch
from
6ab10a5 to
cb9e579
Compare
Co-authored-by: Prashant Varanasi <[email protected]>
ash2k
force-pushed
the
ash2k/grpc-logger
branch
from
9ab1258 to
bb6d5a3
Compare
Co-authored-by: Prashant Varanasi <[email protected]>
|
RE: coverage: The failing cases are the ones for Fatal. I think there's a way to organize this that addresses that issue. I'll add a commit, just a moment. |
Instead of an if/else on each Print and Fatal log statement, handle the indirection once with function references.
Add a zapgrpc/internal/test submodule that tests the zapgrpc logger against grpc-go directly. Because this is a submodule, it will not add grpc as a dependency of zap.
gRPC has an updated [logger interface][1] that the v1 API has been deprecated in favor of. [1]: https://pkg.go.dev/google.golang.org/grpc/grpclog#LoggerV2 This adds support for it to the existing gRPC adapter in Zap. Fixes #534 Closes #538 Co-authored-by: Prashant Varanasi <[email protected]> Co-authored-by: Abhinav Gupta <[email protected]>
gRPC has an updated [logger interface][1] that the v1 API has been deprecated in favor of. [1]: https://pkg.go.dev/google.golang.org/grpc/grpclog#LoggerV2 This adds support for it to the existing gRPC adapter in Zap. Fixes uber-go#534 Closes uber-go#538 Co-authored-by: Prashant Varanasi <[email protected]> Co-authored-by: Abhinav Gupta <[email protected]>
Fixes #534
Closes #538