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

Implementing grpclog.LoggerV2 compatible logger #538

Closed
wants to merge 3 commits into from

Conversation

frankgreco
Copy link

@frankgreco frankgreco commented Dec 19, 2017

Currently, go.uber.org/zap/zapgrpc.Logger only provides an implementation for grpclog.Logger. With the release of grpclog.LoggerV2, an updated implementation is needed so that go.uber.org/zap/zapgrpc.Logger implements both grpclog.Logger and grpclog.LoggerV2.

Fixes #534

@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@f85c78b). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #538   +/-   ##
=========================================
  Coverage          ?   97.41%           
=========================================
  Files             ?       40           
  Lines             ?     2128           
  Branches          ?        0           
=========================================
  Hits              ?     2073           
  Misses            ?       47           
  Partials          ?        8
Impacted Files Coverage Δ
zapgrpc/zapgrpc.go 100% <100%> (ø)

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 f85c78b...d78f25e. Read the comment docs.

@billf billf requested a review from bufdev January 9, 2018 01:44
print: (*zap.SugaredLogger).Info,
printf: (*zap.SugaredLogger).Infof,
log: l.Sugar(),
info: (*zap.SugaredLogger).Info,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason why we have to keep function pointers for each level, instead of just doing l.sugar.Info(..) etc?

Copy link
Author

Choose a reason for hiding this comment

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

@prashantv no reason. I was just trying to maintain the programming style that was previously present.

Copy link
Contributor

Choose a reason for hiding this comment

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

@prashantv this is for the WithDebug function, which changes the zap function used for the Print functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. In that case, aren't the only functions we need to do this for print and printf? The rest should just be normal function calls on the Logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would work too, either/or, I was just being consistent, especially “back in the day” before this PR when there were only a few functions :-)

Copy link
Contributor

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

I'm sorry I missed this. The V implementation is incorrect. V does not correspond to log level, it can be things like 100. See https://godoc.org/github.com/golang/glog#Verbose and https://github.com/golang/glog/blob/master/glog.go#L999 for more details. This is why I didn't implement this originally as it was extra logic.

@bufdev
Copy link
Contributor

bufdev commented Jan 11, 2018

Also see https://github.com/grpc/grpc-go/blob/master/grpclog/loggerv2.go.

It's not hard to implement, but it should be done in a thread-safe manner if you make V modifiable. glog does this.

@frankgreco
Copy link
Author

@peter-edge good catch. I'll fix the implementation.

@bufdev
Copy link
Contributor

bufdev commented Jun 22, 2018

@frankgreco any progress on this?

@kazegusuri
Copy link

I fixed the V implementation in my repo. Can I overtake this PR?

@frankgreco
Copy link
Author

Yea! Sorry I haven’t had time to complete this. I’d be awesome if you could provide the V implementation!

@kazegusuri
Copy link

Thanks. I sent PR based on your commit, so please sign CLA.

@AlekSi
Copy link
Contributor

AlekSi commented Aug 29, 2018

@frankgreco please sign CLA. That should take only a few minutes.

@frankgreco
Copy link
Author

@AlekSi

It seems that I have already signed the CLA.
screen shot 2018-08-29 at 8 43 39 am

You can verify from the username in that image and the username from my commits in this PR that they are the same.

Maybe we should get a repo admin involved?

@AlekSi
Copy link
Contributor

AlekSi commented Sep 2, 2018

Let's continue at #613.

@frankgreco
Copy link
Author

@AlekSi I think I fixed it :) I did an interactive rebase an modified the author/email for this commit to an email that was on my account (I don't have access to verify the original email as I no longer work for that company). As you will see, the CLA has been signed in this PR. Now, the reason it isn't signed in #613 is because that is off of @kazegusuri branch which does not have my modified commit.

@kazegusuri can you update your branch so that you are using my modified commit? Then, we should all be good.

Sorry that this was such a mess :(

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ GRECO, FRANK
❌ frankgreco


GRECO, FRANK seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ash2k
Copy link
Contributor

ash2k commented Nov 16, 2020

Would be great to get this merged! @frankgreco Perhaps you could squash all commits and use the right email to commit it?

@abhinav abhinav closed this in #881 Feb 12, 2021
abhinav added a commit that referenced this pull request Feb 12, 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>
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
9 participants