Add colored LevelEncoders

[bufdev]

Add a small internal package to handle TTY output coloring, and use that package to define some color-aware LevelEncoders.

Fixes #234.

[bufdev]

The code for the above output:

package main

import (
	"fmt"
	"os"

	"go.uber.org/zap"
)

func main() {
	if err := do(); err != nil {
		fmt.Fprintln(os.Stderr, err.Error())
		os.Exit(1)
	}
}

func do() error {
	logger, err := zap.NewDevelopment()
	if err != nil {
		return err
	}
	logger.Debug("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Info("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Warn("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Error("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	return nil
}

type foo struct {
	One string
	Two string
}

[akshayjshah]

In general, I'm all for colorized output by default in dev. I'd consider going even further and colorizing more of the output, too.

I left some line-level comments in the diff, but let's first discuss the overall plan here. I didn't include this functionality before because even though Fatih's package is really nice and has a stable release, it depends on packages from mattn. The actual go-colorable and go-isatty packages are still on alpha-level patch releases, which is why Fatih vendors them and pins to specific commits. We lose that with Glide, which effectively punts the problem to users. I've also had generally poor experiences with mattn's packages; goveralls routinely has breaking API changes.

Before we land these features, I'd like to see how much of these packages we're actually using. Can we vendor the required bits into an internal package that we control? Are there more stable alternatives?

[akshayjshah]

Sprintf instead, which saves an allocation. We should also cache the colorized output for the known levels, which should shave off the other allocation.

This also needs to handle map misses; are nil *color.Colors safe to use?

[akshayjshah]

Same here.

[akshayjshah]

Camel case for consistency with YAML/JSON keys.

[akshayjshah]

Don't mind putting one in testutils, though whatever code deals with levels should handle unexpected levels too.

[akshayjshah]

They're constants, so this doesn't allocate.

[bufdev]

go-isatty does about what you'd expect - it disables colors if stdout does not refer to a terminal. go-colorable handles stdout for colors if on windows, but with how we use fatih/color, it's effectively never used. It's weird that both of these operate on os.Stdout as a constant (or maybe I'm missing something), but that's how how it is.

Additionally when it comes to stability, note that fatih/color vendors go-isatty and go-colorable itself, and I think it's a bug in glide that these are separately vendored in zap. The code from fatih/color will use it's own vendored versions, so we only need to worry about updates to fatih/color. Of note, I've used this package in lion for a long time and have never had any issues.

We can try to re-implement some of the logic in fatih/color that we need, which isn't that complicated, that is one option, and having read over the code more closely, is the option I think I'd recommend. We might get some performance wins too (adding color isn't really hard to do). Adding color is actually a really simple thing, we don't need all the functionality of fatih/color to do it.

[bufdev]

I pushed a commit that does all coloring internally. Note that coloring is not disabled if there is a not a tty, but I'm not sure this is a problem for now - we can add a check if the output is a tty, but this will mean a lot of changes down the stack (find out if we're using a WriteSyncer, find out if the WriteSyncer is actually a file, do the logic in go-isatty effectively) and your setup of EncoderConfig should take this into account. All comments should be addressed through this commit.

[jcorbin]

Glad to see the internal pivot; I do think we want isatty logic still tho, at least part of zap. NewDevelopmentConfig?

Either way, I'm excited to see color support coming to zap 👍

[akshayjshah]

This looks much better to me - the logic is so small that I'd rather avoid the three extra dependencies.

Let's cache the colored strings and add test coverage before landing.

[akshayjshah]

Let's definitely do this. Instead of a map of levels to color codes, we can just have a map of levels to colored strings and a map of levels to capitalized colored strings.

[akshayjshah]

Can we instead assert that the levels are properly color-escaped?

[akshayjshah]

Can we move the coloring logic (color codes, cached strings, etc.) to a separate file? This one's getting a bit big.

If we start coloring other output, we should make this a proper internal package.

[akshayjshah]

I'd really prefer to keep the default un-colored - it's just safer.

[bufdev]

Done

[prashantv]

I'd also prefer to avoid any extra dependencies here. For the isatty logic, I think we can stat stdin to check if it's a terminal:

func useColor() bool {
  stat, err := os.Stdin.Stat()
  if err != nil {
    return false
  }
  return stat.Mode() & os. ModeCharDevice != 0
}

There'll be a ton of edge cases which I don't think we want to deal with -- if there's a terminal, we can assume it supports colour (and the user is always able to override this).

[akshayjshah]

Interesting - that's a good idea that I can get behind. Perhaps NewDevelopmentConfig can stat stdin and choose an appropriate level encoder.

[bufdev]

I'm actually going to argue against checking for color for now, and just make the default no color - I've had more situations where I end up wanting to force coloring than the reverse (with docker for example), and I think we can punt on this, especially if the default is no color.

[bufdev]

I updated this to use an internal package, to have an AllLevels variable in zapcore, and to have cached color level strings. I think that other than a comment about testing the color encoding (which seems more trivial now), everything is taken care of @akshayjshah .

[bufdev]

I also cached the uncolored strings as well.

[bufdev]

Here's the same code now updated to set the LevelEncoder to use color:

package main

import (
	"fmt"
	"os"

	"go.uber.org/zap"
	"go.uber.org/zap/zapcore"
)

func main() {
	if err := do(); err != nil {
		fmt.Fprintln(os.Stderr, err.Error())
		os.Exit(1)
	}
}

func do() error {
	config := zap.NewDevelopmentConfig()
	config.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder
	logger, err := config.Build()
	if err != nil {
		return err
	}
	logger.Debug("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Info("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Warn("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	logger.Error("test", zap.Any("foo", foo{One: "one", Two: "two"}))
	return nil
}

type foo struct {
	One string
	Two string
}

[bufdev]

Can we merge this? I was going to do #330 but this should be merged first so there's less of an issue.

I've now pushed to this branch, so Peter should review

[akshayjshah]

@peter-edge Take a look and let me know what you think. I pulled out some of the string caching and simplified the coloring package a bit.

[bufdev]

LGTM

[ascandella]

Late to the party, but FWIW I disagree with using isatty as a general approach to determining whether colorization should be turned on. Specifically I've had to patch a few third-party projects to remove the assumption that just because I'm not attached to a TTY means that the output can't be colorized. For example, in a lot of our build pipelines we tee stuff around and the escape codes are totally valid to go to stdout and also when cat-ing the file.

Looks like y'all ditched that logic anyway and went with an explicit flag which I like :)

[bufdev]

FORCE_ZAP_COLORS=1 go test aidens/code/...

…

On Tue, Mar 7, 2017 at 6:11 AM, Aiden Scandella ***@***.***> wrote: Late to the party, but FWIW I disagree with using isatty as a general approach to determining whether colorization should be turned on. Specifically I've had to patch a few third-party projects to remove the assumption that just because I'm not attached to a TTY means that the output can't be colorized. For example, in a lot of our build pipelines we tee stuff around and the escape codes are totally valid to go to stdout and also when cat-ing the file. Looks like y'all ditched that logic anyway and went with an explicit flag which I like :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvK-2nmTlyxkgZinutEv7_rqM37Qaks5rjTtugaJpZM4MEPz5> .

[kc1116]

Super late to the party, but I am trying to get colored output and I just can't seem to. Heres my code to setup the logger

func init() {
	jsonDebugging := zapcore.AddSync(ioutil.Discard)
	jsonErrors := zapcore.AddSync(ioutil.Discard)
	consoleDebugging := zapcore.Lock(os.Stdout)
	consoleErrors := zapcore.Lock(os.Stderr)

	jsonEncoder := zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig())
	consoleEncoder := zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig())

	core := zapcore.NewTee(
		zapcore.NewCore(jsonEncoder, jsonErrors, highPriority),
		zapcore.NewCore(consoleEncoder, consoleErrors, highPriority),
		zapcore.NewCore(jsonEncoder, jsonDebugging, lowPriority),
		zapcore.NewCore(consoleEncoder, consoleDebugging, lowPriority),
	)

	var opts []zap.Option
	opts = append(opts, zap.Fields(zap.Int("pid", os.Getpid())))
	opts = append(opts, zap.Fields(zap.String("exe", path.Base(os.Args[0]))))
	Logger = &LoggerWrapper{
		zapLogger: zap.New(core, opts...),
	}
	defer Logger.zapLogger.Sync()
}

[ghost]

@kc1116 this works for me:

package main
import (
   "github.com/mattn/go-colorable"
   "go.uber.org/zap"
   "go.uber.org/zap/zapcore"
)
func main() {
   aa := zap.NewDevelopmentEncoderConfig()
   aa.EncodeLevel = zapcore.CapitalColorLevelEncoder
   bb := zap.New(zapcore.NewCore(
      zapcore.NewConsoleEncoder(aa),
      zapcore.AddSync(colorable.NewColorableStdout()),
      zapcore.DebugLevel,
   ))
   bb.Warn("cc")
}

Or:

package main
import "github.com/labstack/gommon/log"
func main() {
   log.EnableColor()
   log.Info("aaaaa bbbbb")
}