-
Notifications
You must be signed in to change notification settings - Fork 167
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
tk: Format logs with the console log formatter if outputting to a tty #776
Conversation
Since the switch to structured logging in cc21323 we now output evaluation errors all on one line, with escaped newlines: ``` {"level":"fatal","time":"2022-10-07T12:50:42+01:00","message":"Error: evaluating jsonnet: RUNTIME ERROR: function expected 3 positional argument(s), but got 4\n\t/home/laney/temp/tk/environments/default/main.jsonnet:3:14-56\tobject <anonymous>\n\tField \"bar\"\t\n\tDuring manifestation\t\n"} ``` It's pretty unreadable. Instead, what we can do is turn on zerolog's console writer when we're running interactively (precisely, when stderr is a tty). Then the above message becomes: ``` 12:52PM FTL Error: evaluating jsonnet: RUNTIME ERROR: function expected 3 positional argument(s), but got 4 /home/laney/temp/tk/environments/default/main.jsonnet:3:14-56 object <anonymous> Field "bar" During manifestation ``` which is legible again.
cmd/tk/main.go
Outdated
@@ -12,7 +12,7 @@ import ( | |||
"github.com/grafana/tanka/pkg/tanka" | |||
) | |||
|
|||
var interactive = term.IsTerminal(int(os.Stdout.Fd())) | |||
var interactive = term.IsTerminal(int(os.Stderr.Fd())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zerolog outputs to stderr, this is the relevant fd to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used elsewhere though (to tell whether or not the pager should be activated), does checking stdout and stderr do the same thing, or should it be two different vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point, I guess the error one should check stderr and the other one can keep on looking at stdout
It's not really a TTY problem. We have the same problem in CI. The logs are ugly since #774, the fix should've been to add |
I don't really mind if you want to close this, but this has the effect of making all logs look nice. It's a decent practice if you ask me. But I'm not that wedded to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Other than the one comment!
cmd/tk/main.go
Outdated
@@ -12,7 +12,7 @@ import ( | |||
"github.com/grafana/tanka/pkg/tanka" | |||
) | |||
|
|||
var interactive = term.IsTerminal(int(os.Stdout.Fd())) | |||
var interactive = term.IsTerminal(int(os.Stderr.Fd())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used elsewhere though (to tell whether or not the pager should be activated), does checking stdout and stderr do the same thing, or should it be two different vars?
inteactive -> stdoutIsTTY add stderrIsTTY Use the latter for checking if logs should be pretty printed, the former for other checks, e.g. whether to use a pager.
@julienduchesne can you help me with the CI failure please? 🙈 |
Since the switch to structured logging in
cc21323 we now output evaluation errors
all on one line, with escaped newlines:
It's pretty unreadable. Instead, what we can do is turn on zerolog's
console writer when we're running interactively (precisely, when stderr
is a tty). Then the above message becomes:
which is legible again.