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

Add colored LevelEncoders #307

Merged
merged 2 commits into from
Mar 6, 2017
Merged

Add colored LevelEncoders #307

merged 2 commits into from
Mar 6, 2017

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented Feb 17, 2017

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

Fixes #234.

image

@bufdev
Copy link
Contributor Author

bufdev commented Feb 17, 2017

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
}

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

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?

// ColoredLowercaseLevelEncoder serializes a Level to a lowercase string and adds coloring.
// For example, InfoLevel is serialized to "info".
func ColoredLowercaseLevelEncoder(l Level, enc PrimitiveArrayEncoder) {
enc.AppendString(levelToColor[l].Sprint(l.String()))
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

// ColoredCapitalLevelEncoder serializes a Level to an all-caps string and adds color.
// For example, InfoLevel is serialized to "INFO".
func ColoredCapitalLevelEncoder(l Level, enc PrimitiveArrayEncoder) {
enc.AppendString(levelToColor[l].Sprint(strings.ToUpper(l.String())))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

func (e *LevelEncoder) UnmarshalText(text []byte) error {
switch string(text) {
case "capital":
*e = CapitalLevelEncoder
case "colored_capital":
Copy link
Contributor

Choose a reason for hiding this comment

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

Camel case for consistency with YAML/JSON keys.

var (
// TODO(pedge): add a test to verify all levels covered? It would be nice
// to maintain a slice of all levels in level.go for testing, even if there
// is already _minLevel and _maxLevel
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
)

// TODO(pedge): we can probably just cache the level strings
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@bufdev
Copy link
Contributor Author

bufdev commented Feb 20, 2017

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
Copy link
Contributor Author

bufdev commented Feb 20, 2017

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
Copy link
Contributor

jcorbin commented Feb 24, 2017

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 👍

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

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.

}
)

// TODO(pedge): we can probably just cache the level strings
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

config_test.go Outdated
@@ -61,7 +63,8 @@ func TestConfig(t *testing.T) {
defer os.Remove(temp.Name())

tt.cfg.OutputPaths = []string{temp.Name()}
tt.cfg.EncoderConfig.TimeKey = "" // no timestamps in tests
tt.cfg.EncoderConfig.TimeKey = "" // no timestamps in tests
tt.cfg.EncoderConfig.EncodeLevel = zapcore.LowercaseLevelEncoder // no tty in tests
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return
}
enc.AppendString(fmt.Sprintf("\x1b[%dm%s\x1b[0m", colorCode, s))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

config.go Outdated
@@ -129,7 +129,7 @@ func NewDevelopmentConfig() Config {
CallerKey: "C",
MessageKey: "M",
StacktraceKey: "S",
EncodeLevel: zapcore.CapitalLevelEncoder,
EncodeLevel: zapcore.ColoredCapitalLevelEncoder,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@prashantv
Copy link
Collaborator

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
Copy link
Contributor

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

@bufdev
Copy link
Contributor Author

bufdev commented Feb 28, 2017

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
Copy link
Contributor Author

bufdev commented Feb 28, 2017

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
Copy link
Contributor Author

bufdev commented Feb 28, 2017

I also cached the uncolored strings as well.

@bufdev
Copy link
Contributor Author

bufdev commented Feb 28, 2017

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
Copy link
Contributor Author

bufdev commented Mar 5, 2017

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

Akshay Shah added 2 commits March 6, 2017 10:24
Avoid vendoring more third-party dependencies by creating a small package for
TTY coloring.
Use the internal package to add small, colored LevelEncoders. For safety (since
we don't check that the output is a TTY), keep the default uncolored.
@akshayjshah akshayjshah dismissed their stale review March 6, 2017 18:26

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

@akshayjshah
Copy link
Contributor

@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
Copy link
Contributor Author

bufdev commented Mar 6, 2017

LGTM

@akshayjshah akshayjshah merged commit 3e4a6c3 into master Mar 6, 2017
@bufdev bufdev deleted the color branch March 6, 2017 18:49
@ascandella
Copy link

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
Copy link
Contributor Author

bufdev commented Mar 7, 2017 via email

@akshayjshah akshayjshah mentioned this pull request Mar 7, 2017
@kc1116
Copy link

kc1116 commented Mar 5, 2018

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
Copy link

ghost commented Jun 23, 2019

@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")
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Colored log output
6 participants