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

zapgrpc upgrade for grpclog.LoggerV2 #534

Closed
geeknoid opened this issue Dec 4, 2017 · 10 comments · Fixed by #881
Closed

zapgrpc upgrade for grpclog.LoggerV2 #534

geeknoid opened this issue Dec 4, 2017 · 10 comments · Fixed by #881

Comments

@geeknoid
Copy link

geeknoid commented Dec 4, 2017

The latest gRPC releases support an expanded logger interface (https://github.com/grpc/grpc-go/blob/master/grpclog/loggerv2.go). It'd be great if zap's gRPC support was updated accordingly.

Thanks.

@frankgreco
Copy link

I just ran into this as well.

@frankgreco
Copy link

@geeknoid if you need an immediate workaround you can use:

[[constraint]]
  name = "go.uber.org/zap"
  branch = "grpclog-v2"
  source = "github.com/frankgreco/zap"

@geeknoid
Copy link
Author

geeknoid commented Dec 20, 2017 via email

@roytman
Copy link

roytman commented Apr 7, 2018

Hi @frankgreco , do you have any estimation time when the feature will be merged ? Thanks

@frankgreco
Copy link

@roytman This PR was held up because I still needed to implement LoggerV2.V(). I'll try to finish this in the next couple of weeks so that we can get this merged.

@roytman
Copy link

roytman commented Apr 9, 2018

Many thanks for the update and of course for your work.

@AlekSi
Copy link
Contributor

AlekSi commented Aug 29, 2018

Gentle poke. grpclog.Logger is deprecated now. Would be nice to have LoggerV2-compatible implementation.

@prashantv
Copy link
Collaborator

This ended up adding significant API surface to the Sugar logger, which I don't think we want to do.

This wrapper can sit outside of zap, and in future we may provide the gRPC adapter in a sub-package rather than extending the Sugar logger's API to support the gRPC logger.

@ash2k
Copy link
Contributor

ash2k commented Jan 15, 2021

This ended up adding significant API surface to the Sugar logger, which I don't think we want to do.

But #881 does not add any methods to the Sugar logger? In fact, it does not touch any files outside of zapgrpc directory.

@prashantv
Copy link
Collaborator

You're completely right, it already is in a subpackage, so it's fine to extend it. Reviewed #881

abhinav added a commit that referenced this issue 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 issue 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 issue 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
7 participants