Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for grpclog.LoggerV2 #881

Merged
merged 13 commits into from Feb 12, 2021
Merged

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Nov 22, 2020

Fixes #534

Closes #538

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 22, 2020

Codecov Report

Merging #881 (4b0ccf6) into master (9d2e5de) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
zapgrpc/zapgrpc.go 100.00% <100.00%> (ø)
sugar.go 100.00% <0.00%> (ø)
options.go 100.00% <0.00%> (ø)
http_handler.go 100.00% <0.00%> (ø)
clock.go 100.00% <0.00%> (ø)
logger.go 96.19% <0.00%> (+0.03%) ⬆️
zapcore/error.go 96.77% <0.00%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d2e5de...4b0ccf6. Read the comment docs.

@ash2k
Copy link
Contributor Author

ash2k commented Nov 22, 2020

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 Fatal.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this significantly larger API surface added to the zap logger.

I think it's best for a wrapper to live outside of zap to support the gRPC v2 logger interface.

@ash2k
Copy link
Contributor Author

ash2k commented Nov 26, 2020

Hm, fine. I assumed you wanted it because #534 was open for 3 years and nobody closed it, and the PR too.

@prashantv
Copy link
Collaborator

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 prashantv closed this Dec 8, 2020
@ash2k
Copy link
Contributor Author

ash2k commented Dec 8, 2020

@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.

@prashantv
Copy link
Collaborator

prashantv commented Dec 8, 2020

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.

@ash2k
Copy link
Contributor Author

ash2k commented Dec 8, 2020

By API change, I mean the additional methods that are added to the Sugar logger.

But this PR does not add any methods to the Sugar logger?

@prashantv
Copy link
Collaborator

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

@prashantv prashantv reopened this Jan 25, 2021
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ash2k
Copy link
Contributor Author

ash2k commented Jan 27, 2021

@prashantv

clarify in the documentation for WithDebug

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?

@prashantv
Copy link
Collaborator

@prashantv

clarify in the documentation for WithDebug

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 WithDebug will only affect the Print* methods, which are only used in the gRPC v1 Logger APIs. Thanks

@ash2k
Copy link
Contributor Author

ash2k commented Jan 29, 2021

@prashantv done

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing, I wanted to make sure I gave this a thorough look, and noticed a couple of small issues.

zapgrpc/zapgrpc.go Outdated Show resolved Hide resolved
zapgrpc/zapgrpc.go Show resolved Hide resolved
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor comments, but overall looks good

zapgrpc/zapgrpc.go Outdated Show resolved Hide resolved
zapgrpc/zapgrpc.go Outdated Show resolved Hide resolved
zapgrpc/zapgrpc.go Outdated Show resolved Hide resolved
ash2k and others added 2 commits February 4, 2021 14:18
Co-authored-by: Prashant Varanasi <github@prashantv.com>
zapgrpc/zapgrpc.go Outdated Show resolved Hide resolved
zapgrpc/zapgrpc_test.go Show resolved Hide resolved
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with a couple of minor test suggestions.

zapgrpc/zapgrpc.go Show resolved Hide resolved
zapgrpc/zapgrpc_test.go Outdated Show resolved Hide resolved
ash2k and others added 3 commits February 10, 2021 10:02
Co-authored-by: Prashant Varanasi <github@prashantv.com>
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think it would be valuable to add a test that tests directly against grpc-go
so that we know we're interface compliant.

But we would also like to not add a dependency on grpc-go in Zap. This
probably requires a submodule. I can add that.

@abhinav
Copy link
Collaborator

abhinav commented Feb 12, 2021

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.
@abhinav abhinav merged commit 89e3820 into uber-go:master Feb 12, 2021
abhinav added a commit that referenced this pull request May 25, 2021
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 <github@prashantv.com>
Co-authored-by: Abhinav Gupta <abg@uber.com>
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
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 <github@prashantv.com>
Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

zapgrpc upgrade for grpclog.LoggerV2
4 participants