log.WithSuffix, appends kvs to those passed to Log

[vinayvinay]

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 

[ChrisHines]

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

[vinayvinay]

@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]

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 

[ChrisHines]

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.

[vinayvinay]

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.

[ChrisHines]

LGTM. Thanks for the contribution!

[ChrisHines]

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.

[ChrisHines]

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

[vinayvinay]

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

[ChrisHines]

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

[vinayvinay]

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