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

Format console encoder fields as key=value #1188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tamj0rd2
Copy link

This is related to #1076

@CLAassistant
Copy link

CLAassistant commented Oct 19, 2022

CLA assistant check
All committers have signed the CLA.

@ringerc
Copy link

ringerc commented Oct 20, 2022

Delighted to find this PR just as I was looking for a way to format key/value data in a more friendly way in the console encoder.

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (9b86a50) 98.25% compared to head (05964c5) 97.79%.
Report is 76 commits behind head on master.

Files Patch % Lines
zapcore/console_encoder.go 85.71% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
- Coverage   98.25%   97.79%   -0.47%     
==========================================
  Files          49       49              
  Lines        2180     2221      +41     
==========================================
+ Hits         2142     2172      +30     
- Misses         29       37       +8     
- Partials        9       12       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tamj0rd2
Copy link
Author

Any ideas behind the linting failure? I got it locally too after pulling in the latest changes from main, but it's unrelated to any of the files I changed:

https://github.com/uber-go/zap/actions/runs/3280798463/jobs/5425117446

cd tools && go install golang.org/x/lint/golint
cd tools && go install honnef.co/go/tools/cmd/staticcheck
Checking formatting...
diff sugar_test.go.orig sugar_test.go
--- sugar_test.go.orig
+++ sugar_test.go
@@ -220,[7](https://github.com/uber-go/zap/actions/runs/3280798463/jobs/5425117446#step:6:8) +220,7 @@
 		extra          = []interface{}{err, "baz", false}
 		expectedFields = []Field{String("foo", "bar"), Error(err), Bool("baz", false)}
 	)
-	
+
 	for _, tt := range tests {
 		withSugar(t, DebugLevel, nil, func(logger *SugaredLogger, logs *observer.ObservedLogs) {
 			logger.With(context...).Debugw(tt.msg, extra...)
Checking vet...
Checking lint...
Checking staticcheck...
Checking for unresolved FIXMEs...
Checking for license headers...
make: *** [Makefile:35: lint] Error 1
Error: Process completed with exit code 2.

@mway
Copy link
Contributor

mway commented Oct 20, 2022

Thanks for the PR! This is something that's been on our minds as well. We'll review over the next couple of days.

Any ideas behind the linting failure? I got it locally too after pulling in the latest changes from main, but it's unrelated to any of the files I changed

I haven't had a chance to look, but it looks like either (a) an unformatted whitespace change existed before or (b) something subtle has changed upstream since. Not a big deal.

@bjartek
Copy link

bjartek commented Jan 11, 2023

Any update here? this would be very usefull.

@tamj0rd2
Copy link
Author

Anything I need to do to get this moving along?

@abhinav
Copy link
Collaborator

abhinav commented Feb 11, 2023

Hey, I don't represent the Zap maintainers anymore, but I recall that one of the problems with the proposed changes was that the result wouldn't be fully one format. It would be a mix of logfmt-style and JSON output and that wasn't quite right.

However, inspired by this PR, I made an attempt at something that would cover all cases--printing everything in a logfmt-style format (although not to-spec). I hadn't fully ironed out the details before I moved on from the project, but the work is available in this branch. It's super rough and it has many issues: there's duplication, no consideration for performance, and tests are nonexistent. I may make time to pick it up again, but meanwhile if someone else would like to have a go at it, I think the shape of the solution in that branch might be adaptable into a general purpose readable console encoder for Zap -- either in the official package or as an external library.

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.

6 participants