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

log.WithSuffix, appends kvs to those passed to Log #992

Merged
merged 1 commit into from Jul 4, 2020

Conversation

vinayvinay
Copy link
Contributor

@vinayvinay vinayvinay commented Jun 18, 2020

enhancement suggested in: #991

why?

if a human is reading logfmt output on the shell or in the web browser, it helps to have certain fields prefixed, e.g. ts, but certain fields are better suffixed, e.g. caller, environment, so that more important information appears first.

benchmarks suggest an additional cost only if WithSuffix is used.

goos: darwin
goarch: amd64
pkg: github.com/go-kit/kit/log
BenchmarkDiscard-4               	32230156	        38 ns/op	      32 B/op	       1 allocs/op
BenchmarkOneWith-4               	 9647907	       122 ns/op	      96 B/op	       2 allocs/op
BenchmarkTwoWith-4               	 8935790	       134 ns/op	     160 B/op	       2 allocs/op
BenchmarkTenWith-4               	 5016836	       236 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithPrefix-4         	 9907198	       123 ns/op	      96 B/op	       2 allocs/op
BenchmarkTenWithPrefix-4         	 5076309	       239 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithSuffix-4         	 6432942	       189 ns/op	     128 B/op	       3 allocs/op
BenchmarkTenWithSuffix-4         	 4899711	       246 ns/op	     416 B/op	       3 allocs/op
BenchmarkOneWithPrefixSuffix-4   	 6111750	       197 ns/op	     160 B/op	       3 allocs/op
BenchmarkTenWithPrefixSuffix-4   	 2172066	       555 ns/op	    1952 B/op	       3 allocs/op
PASS
ok  	github.com/go-kit/kit/log	14.224s

@vinayvinay vinayvinay marked this pull request as ready for review Jun 18, 2020
log/log.go Outdated Show resolved Hide resolved
log/log_test.go Show resolved Hide resolved
Copy link
Member

@ChrisHines ChrisHines left a comment

Thanks for the well done PR! Check my comments for some things I think we should change.

log/log_test.go Outdated Show resolved Hide resolved
log/log_test.go Show resolved Hide resolved
log/log.go Outdated Show resolved Hide resolved
@vinayvinay
Copy link
Contributor Author

vinayvinay commented Jun 24, 2020

@ChrisHines Thanks for your guidance. I agree to the feedback and have made changes accordingly. Please take another look. I've kept commits separate to make it easy for you to review, will squash later.

@vinayvinay
Copy link
Contributor Author

vinayvinay commented Jun 24, 2020

There isn't a noticeable change to benchmarks after recent changes.

goos: darwin
goarch: amd64
pkg: github.com/go-kit/kit/log
BenchmarkDiscard-4               	28010008	        41.7 ns/op	      32 B/op	       1 allocs/op
BenchmarkOneWith-4               	 8629581	       141 ns/op	      96 B/op	       2 allocs/op
BenchmarkTwoWith-4               	 7579276	       164 ns/op	     160 B/op	       2 allocs/op
BenchmarkTenWith-4               	 4112787	       295 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithPrefix-4         	 8480372	       145 ns/op	      96 B/op	       2 allocs/op
BenchmarkTenWithPrefix-4         	 4161474	       291 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithSuffix-4         	 5459065	       230 ns/op	     128 B/op	       3 allocs/op
BenchmarkTenWithSuffix-4         	 3396878	       318 ns/op	     416 B/op	       3 allocs/op
BenchmarkOneWithPrefixSuffix-4   	 5223512	       227 ns/op	     160 B/op	       3 allocs/op
BenchmarkTenWithPrefixSuffix-4   	 1719489	       695 ns/op	    1952 B/op	       3 allocs/op
PASS
ok  	github.com/go-kit/kit/log	16.108s

Copy link
Member

@ChrisHines ChrisHines left a comment

The updates look great. Just one minor change to make and then this will be ready to merge.

Sorry it took so long to get back to reviewing this. I promise to be quicker to respond this time.

log/log.go Show resolved Hide resolved
log/log_test.go Show resolved Hide resolved
Copy link
Contributor Author

@vinayvinay vinayvinay left a comment

No worries about the timing. Thank you for the feedback. You'll easily spot the reordering of context fields (only change since last review), hence I've squashed all commits into one.

One minor thing I'd like your opinion on: comments mention With and WithPrefix (example), now that there's a WithSuffix, would you say it is OK keeping these comments as-is, or update them to mention this 3rd func, or something like With* functions instead?

Do let me know if there's anything else you'd like to change.

@vinayvinay vinayvinay requested a review from ChrisHines Jul 2, 2020
Copy link
Member

@ChrisHines ChrisHines left a comment

LGTM. Thanks for the contribution!

@ChrisHines
Copy link
Member

ChrisHines commented Jul 3, 2020

One minor thing I'd like your opinion on: comments mention With and WithPrefix (example), now that there's a WithSuffix, would you say it is OK keeping these comments as-is, or update them to mention this 3rd func, or something like With* functions instead?

Just saw this question. That's a good catch. We prefer our code comments to be of the highest quality in Go kit, so yes, please update that to add WithSuffix to the list. I would rather not use With* functions because comments should preferably be in clear human language.

Copy link
Member

@ChrisHines ChrisHines left a comment

Please add WithSuffix where appropriate to any comments that currently mention With and WithPrefix.

enhancement suggested in:
go-kit#991

why?

if a human is reading logfmt output on the
shell or in the web browser, it helps to
have certain fields prefixed, e.g. ts,
but certain fields are better suffixed,
e.g. caller, environment, so that more
important information appears first.

benchmarks suggest an additional cost
only if WithSuffix is used.

goos: darwin
goarch: amd64
pkg: github.com/go-kit/kit/log
BenchmarkDiscard-4               	32230156	        38 ns/op	      32 B/op	       1 allocs/op
BenchmarkOneWith-4               	 9647907	       122 ns/op	      96 B/op	       2 allocs/op
BenchmarkTwoWith-4               	 8935790	       134 ns/op	     160 B/op	       2 allocs/op
BenchmarkTenWith-4               	 5016836	       236 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithPrefix-4         	 9907198	       123 ns/op	      96 B/op	       2 allocs/op
BenchmarkTenWithPrefix-4         	 5076309	       239 ns/op	     672 B/op	       2 allocs/op
BenchmarkOneWithSuffix-4         	 6432942	       189 ns/op	     128 B/op	       3 allocs/op
BenchmarkTenWithSuffix-4         	 4899711	       246 ns/op	     416 B/op	       3 allocs/op
BenchmarkOneWithPrefixSuffix-4   	 6111750	       197 ns/op	     160 B/op	       3 allocs/op
BenchmarkTenWithPrefixSuffix-4   	 2172066	       555 ns/op	    1952 B/op	       3 allocs/op
PASS
ok  	github.com/go-kit/kit/log	14.224s
Copy link
Contributor Author

@vinayvinay vinayvinay left a comment

@ChrisHines I've updated comments to reflect all 3 functions that build context.

@vinayvinay vinayvinay requested a review from ChrisHines Jul 3, 2020
Copy link
Member

@ChrisHines ChrisHines left a comment

Fantastic work and thanks for your patience through the review process.

@ChrisHines ChrisHines merged commit ebfffd0 into go-kit:master Jul 4, 2020
2 checks passed
@vinayvinay
Copy link
Contributor Author

vinayvinay commented Jul 6, 2020

Thank you for your guidance Chris. It has been a pleasure!

@vinayvinay vinayvinay deleted the log-with-suffix branch Jul 20, 2020
@sagikazarmark sagikazarmark added this to the v0.11.0 milestone Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants