[Developer] Don't use glog

[broady]

Most of the samples use glog. Just use the standard log package.

https://github.com/elafros/elafros/blob/master/sample/helloworld/helloworld.go

[mattmoor]

I'm curious why?

Is it because things should work with just the standard log package? I'd buy that.

[broady]

Yep, glog is overly complicated, pollutes global flags, and is really only suitable for use internally at Google (and even then, I think the version on github is out of date compared to the internal version).

It's also not maintained (as it states in the README).

Could you please assign to me? (or give me permissions to re-assign things?)

[grantr]

FWIW Istio uses zap for logging along with a wrapper that directs glog output to zap. In addition to the points @broady mentions, zap might be a better general choice than glog because it emits structured logs that are easier for log indexers to consume (@mdemirhan might have opinions on that).

[broady]

I'd probably just go with standard library log. If people want to swap it out for their own, they can go for it. I strongly believe minimizing dependencies for example code is a good thing (particularly because it minimizes cognitive load), unless there's a very good reason to add a dependency. As you say, if elafros consumes zap log messages better than standard log, then that might be a good enough reason to add it. If not, I don't think it's worth it.

edit: another mention - it's important to separate the decisions made for elafros (zap, glog, etc) from the ones we impose/suggest to users. The tradeoffs chosen for elafros aren't obviously applicable to users.

[tcnghia]

I still see some code using glog. @mattmoor should we make this consistent across all code?

@jonjohnsonjr also recommends logrus

[dprotaso]

Is this for samples only?

[dprotaso]

nm - learned to read

[mdemirhan]

Samples are converted. But most of the controller code still use glog. We don't have to use the standard log for elafros and ideally we should use a structured logging library in Elafros to make things easier to index and categorize. logrus seems to be the most widely used logging library. zap also seems promising and more focused on performance than features. Decisions decisions :)

[jonjohnsonjr]

I mostly brought this up because some logs had the file name and line number, but others didn't. That was frustrating when debugging and it looks like I can fix that by just calling log.SetFlags.

I bias towards fewer deps... so IMO, let's just do that and ditch glog for the standard log everywhere. If later we want leveled logging and/or find logging perf to be an issue (doubtful), we can bikeshed over zap vs logrus. WDYT?

[jonjohnsonjr]

Started ripping out glog:
jonjohnsonjr/elafros@0f510b4

I stumbled across some uses here:
https://github.com/elafros/elafros/blob/master/docs/telemetry.md#generating-logs

@mdemirhan Should I change those? Is it just for samples?

[jonjohnsonjr]

/assign jonjohnsonjr

[mdemirhan]

We should change those. My mistake - I should have changed when I replaced the samples to use log.

[vaikas]

Just want to address "If later we want leveled logging" because I believe we'll certainly need two kinds of logs from the get-go because there are two kinds of logs ( I think at least):

1. messages for developers of Elafros (think debug)
2. messages for users of Elafros

First of these is the one that will be potentially noisy and I'd argue that by default we should only be printing user facing messages but have the facility to expose the debug logs without exposing them to users that shouldn't care about seeing those. So, as long as there's a way to do that without later converting log.X to something else again or if there's already a way to do this. Anyways, just my $.02.

[jonjohnsonjr]

That's a good point. #884 doesn't distinguish between these log kinds at all (in fact, it drops any distinction that currently exists), so we probably don't want to merge that PR in its current state. I'm also not sure what the bar should be for what is debug vs user-facing.

I don't really have any strong opinions on this, so I'm happy to pursue whatever people thing is best.

[mdemirhan]

It might be worth holding on this. Please don't close and just assign to me. I will evaluate the use of logrus and zap and see if we can satisfy 1 and 2 @vaikas-google mentioned above.

I also think that we should use structured logging for at least (2) above. (1) above should be turned off by default.

[grantr]

we can bikeshed over zap vs logrus.

I'd be surprised if logrus won that argument given its dramatic performance deficit compared to zap. @mdemirhan what features does logrus have that zap doesn't?

As measured by zap's benchmarking suite:

Log a message and 10 fields:

Package	Time	Objects Allocated
⚡ zap	3131 ns/op	5 allocs/op
logrus	23662 ns/op	142 allocs/op

Log a message with a logger that already has 10 fields of context:

Package	Time	Objects Allocated
⚡ zap	380 ns/op	0 allocs/op
logrus	22312 ns/op	130 allocs/op

Log a static string, without any context or printf-style templating:

Package	Time	Objects Allocated
⚡ zap	361 ns/op	0 allocs/op
logrus	2291 ns/op	27 allocs/op

[mdemirhan]

@grantr One example where zap doesn't have parity with is the hooks (or sinks, writers and encoders in zap terms). Specifically, we have been evaluating syslog instead of file based logging with @yanweiguo and @evankanderson and zap doesn't have support for it yet but logrus does.

[jonjohnsonjr]

/assign mdemirhan

[grantr]

Zap not supporting syslog because Uber doesn't use it is indeed unfortunate. https://github.com/tchap/zapext includes a zap syslog package though. Here's an example of using it. Seems reasonably straightforward.

[mdemirhan]

Looking into the code, it seems so silly that zap doesn't support something that looks so simple. Thanks for the pointers - given the package you linked above, it can't think of a feature we need that is supported in logrus but not in zap. I will evaluate using zap for Elafros logging. Thanks!

[dprotaso]

Another criteria I'd like us to consider in addition to structured and level logging, both of which I think we should have, is the practice of contextual logging. This can be accomplished in zap using the Logger.Named and Logger.With

The purposes of this is for traceability within a process which I don't really see us talking about. Our controllers will become more complex and if we bump threadiness we'll be dealing with simultaneous concurrent operations. Debugging and determining causality will become a pain if we don't start utilizing contextual logging.

For this to work you'll need to pass the Logger through function invocations as an argument. This is similar to the usage of context.Context. In fact some projects attach the logger as part of a context. See containerd here and here for a usage example.

[mdemirhan]

Great points @dprotaso - agree with all. Coincidentally, I started working on doing exactly that in one of the controllers in Elafros, using zap.Logger.With() passed between different contexts. One difference is though, in our controllers, we don't really have a context to work with and instead of passing the context, for each work item, I create a child logger using .With(...) and pass the logger around.

With this, for each operation in controller, we should have relevant information attached to all logs. For example, if we are working on updating route rules for a revision, the logs should have configuration, revision and route rule name attached and when debugging, we should be able to slice and dice with any of these fields.

I plan on sending a PR later on and hopefully duplicate the work in rest of Elafros.

PS: Passing a logger in each call is getting quite ugly though and if you have some creative way to hide that ugliness, let me know (in the context of our controllers, which are background workers reading from queues every 30 seconds and executing the work - the With() fields are filled with contextual information about the specific work item).

[dprotaso]

I don't think there's an equivalent to Java's ThreadLocal in golang

This article (https://blog.golang.org/context) has a guideline

At Google, we require that Go programmers pass a Context parameter as the first argument to every function on the call path between incoming and outgoing requests. This allows Go code developed by many different teams to interoperate well. It provides simple control over timeouts and cancelation and ensures that critical values like security credentials transit Go programs properly.

If we dump the logger into a context we'll easily get timeouts and cancellation down the road. Curious what others think in terms of this