add NewTestingLogger for usage in tests

[oliverpool]

Having access to the log can be quite useful in tests. However it is even nicer if the log is displayed only for failing tests, as well as the location where the call was made.

This PR add a NewTestingLogger function which supports such usecase, by calling t.Helper and t.Log with the logfmt-formatted string.

It uses logfmt.MarshalKeyvals directly, since I don't think that performance is so critical in tests.

[peterbourgon]

There are many ways to compose the Go kit logger interface with other standard packages and components. In fact, package log was designed specifically so that these kinds of value-add capabilities could be "layered on" to loggers without requiring additions or modifications to the package itself.

I'd say this is a good example of something that you can and should implement yourself in your consumer application/library. In fact, I frequently write something quite similar to your test logger: an implementation of the log.Logger interface that converts logger.Log events to tb.Logf calls:

type logWriter struct {
    tb testing.TB
}

func (w *logWriter) Write(p []byte) (int, error) {
    w.tb.Log(strings.TrimSpace(string(p)))
    return len(p), nil
}

func newTestLogger(tb testing.TB) log.Logger {
    return log.NewLogfmtLogger(&logWriter{tb})
}

[oliverpool]

Thanks for the quick reply. I agree with your comment and I think such a function would be a nice addition to this library:

I frequently write something quite similar to your test logger

• so do I, that's why I finally suggested adding it here :smile:
• however before having this version, I did many version, which where wrong or suboptimal:
  • one of the first version simply used t.Log(keyvals ...interface{}) (which misses the = and the "Dynamic Contextual Values")
  • I then had the same version as you propose, however the calling-site logged by t.Log was the w.tb.Log line and adding t.Helper here moved it to the (*logfmt).w.Write line, which isn't very interesting (and the TrimSpace does not feel nice)
  • I finally landed on the version I propose here, which checks all the boxes (the pointed lines are wrong when using log.With, but this is something that I didn't find an elegant solution to)
• and finally adding this function does not add any dependency

So to sum up, adding such a function does not add much complexity to this library, encourages developers to use it in there tests and prevents having them re-implementing it (which can go wrong).

[ChrisHines]

I have also written variations on this theme with different bells and whistles depending on the situation. My initial thoughts are that I agree the idea is useful, but I'm not sure the code needs to be in this module because there may be too many small variations that people want. The variations may swamp the simplicity over time and that would be a shame.

My vote would be to post the implementation to a discussion thread in this repo. Maybe others will share their tweaks and variations. If, over time, a common core becomes obvious we could add an implementation into the module itself. If we do add it in the future, my instinct is to add it as a separate go-kit/log/testlog package.

[oliverpool]

Thanks for the feedback! I agree that we can't provide a solution for every single problem. However I think that proposing a canonical approach is greatly beneficial: my code is 20 lines long, if they really want a variation they can copy/paste and adapt them to the situation.

there may be too many small variations that people want

What would you want? for me the version posted has enough (as mentioned, it is limited regarding t.Log calling site when using log.With, but I accept this drawback for the ease of use of simplicity of the code). If I really need something fancy, I will copy/paste and adapt. This already covers 99% of my usecases: how about you?

My vote would be to post the implementation to a discussion thread in this repo.

I think that this PR can be considered a discussion thread, but feel free to open a thread a ping me :)

separate go-kit/log/testlog package

For better discoverability I think the main package would make more sense (however if the goal is to provide many variation, a dedicated package would make sense - I would rather have one accessible convenient function than many variations)

[peterbourgon]

I appreciate the effort! Your suggestion is certainly useful. But the bar for expanding the surface area of the log module is very very high. And, as @ChrisHines notes, there are many solutions to this problem, none of which IMO are sufficiently general to be the "canonical" implementation. So I'll close this PR, but, again, thanks for the thought and discussion.