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

Feature Request: Replace glog with Zap logger #11613

Open
EmadMokhtar opened this issue Nov 1, 2022 · 10 comments
Open

Feature Request: Replace glog with Zap logger #11613

EmadMokhtar opened this issue Nov 1, 2022 · 10 comments
Labels
Component: General Changes throughout the code base Type: Feature

Comments

@EmadMokhtar
Copy link
Contributor

Feature Description

Overview

Vitess is using glog as its logger. glog package isn't under development "as stated in the README" plus its original version is maintained by Google, and it's a closed-source.

The repository contains an open source version of the log package used inside Google. The master copy of the source lives inside Google, not here. The code in this repo is for export only and is not itself under development. Feature requests will be ignored.

Motivation

Unmaintained package

Replacing an unmaintained package with one is maintained and well-known in the golang community.

Package Stars Last commit OSS
glog 3.2K 20 Aug. 2021
zap 17.3K 28 Oct. 2022

Faster logger

Plus point (which is a marketing point) zap logger is blazingly fast 😉

Verbosity flag

One of my colleague is working on a feature that needs to import vitess libraries. He's building a CLI where -v flag is needed. He stumbled across a problem because the glog package already defines the -v flag. So he can't import Vitess libs and define -v flag.

Use Case(s)

  • Use suggered logger for development and unsugered logger for production. Suggered logger i s better for debugging, and it has better readability.
  • Avoid the -v flag collision.
@EmadMokhtar EmadMokhtar added Needs Triage This issue needs to be correctly labelled and triaged Type: Feature labels Nov 1, 2022
@EmadMokhtar
Copy link
Contributor Author

In case the proposal approved, I can start working on this feature.

@rohit-nayak-ps rohit-nayak-ps added Component: General Changes throughout the code base and removed Needs Triage This issue needs to be correctly labelled and triaged labels Nov 1, 2022
@rohit-nayak-ps
Copy link
Contributor

@EmadMokhtar ,thanks for reporting this and offering to work on it. We are going to discuss it internally and hope to get back to you in a week or two.

@deepthi
Copy link
Member

deepthi commented Nov 3, 2022

Related: #11614
Should keep that in mind.

@EmadMokhtar
Copy link
Contributor Author

Thanks for the comment. I'll do my research on what is the memory friendly logger in Go.

@ejortegau
Copy link
Contributor

Related: https://github.com/planetscale/log

@EmadMokhtar
Copy link
Contributor Author

Related: #11614 Should keep that in mind.
I ran a benchmark tests for multiple logger and AFAIK zap “Production” logger is good choice.

https://github.com/EmadMokhtar/golang-benchmark

@ejortegau
Copy link
Contributor

Planetscale's log is a wrapper for zap, @EmadMokhtar. Coupled with noglog, it can make for an easy migration path, I think.

@rohit-nayak-ps
Copy link
Contributor

@EmadMokhtar @ejortegau, just repeating here what Deepthi said in Vitess Slack: we all think it is a good idea to get this done now. Thanks for this initiative.

It will be good if you can implement this in parts. Since this change will impact a lot of code, doing this in multiple PRs will help get any review feedback earlier in the cycle. We can also start merging and testing sooner in main.

You can take a call on what works best: using PlanetScale's log or some other approach and share it on this issue with your thoughts/decisions.

@EmadMokhtar
Copy link
Contributor Author

After the merge of #11960 the next step is to make the structure logging as the default logging in Vitess. This will help us in deprecating the glog package and using PlanetScale logger as standard logging in Vitess.

Risks

  • Not all components are migrated to PlanetScale logger. So I'm not sure about the dependency of these components and glog package.

@dbussink
Copy link
Contributor

I know it's a bit late, but I think we should at this point also seriously consider https://pkg.go.dev/golang.org/x/exp/slog. Afaik it's aimed to be part of the standard library in the future as well with the goal of having something more standardized across different Go projects.

We might want to change the PlanetScale logger then to wrap exp/slog instead of zap then though to make the migration easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Type: Feature
Projects
None yet
Development

No branches or pull requests

5 participants