-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enhancement: log.WithSuffix #991
Comments
Hello @peterbourgon @ChrisHines, do you have any thoughts on this one? |
I don't have strong opinions, but I'm not immediately convinced that this is useful enough to justify the complexity. I'm also a little concerned there might be a performance impact. Could you add a few test cases so I can see the behavior? Mixing and matching |
Another reason this is of indeterminate value is that not all output formats respect order (e.g. JSON). |
My apologies for the late response. My first reaction when I saw this feature request was skepticism. After a little thinking about what the implementation might look like I was less concerned because in my mind it looked pretty much like what you wrote in your prototype and it doesn't feel too costly. I would like to see some benchmarks but it seems like it shouldn't add much overhead for people that never use it. My biggest concern at this point is whether it pays for the cost of an additional exported function in the API. It may, since I cannot think of a good way to implement it externally and get semantics similar to to My vote is to proceed with a PR and benchmarking. |
Thank you for these inputs, fair points. I will come back with a PR with benchmarks. |
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
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
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
enhancement suggested in: go-kit/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
enhancement suggested in: go-kit/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
Contrary to
log.WithPrefix
, which prepends keyvals to those passed toLog
, we have a requirement to append keyvals.Especially 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 when suffixed, e.g.caller
,environment
, so that more important information appears first, e.g.:could be better
WithSuffix
:Here's what it might look like in code, which is just a draft, will polish it up and PR if there's interest. Do you think
WithSuffix
will come handy?The text was updated successfully, but these errors were encountered: