Replace kit/log with log

[ChrisHines]

The kit/log packages that were copied to their own module become aliases for the new packages to avoid breaking existing code.

This change may also help projects that end up with dependencies using both import paths. They can use the types from the two paths interchangably and will only have one copy of the actual implementations compiled in to them.

Also change all the other Go kit packages to use the new log packages directly.

[sagikazarmark]

Seems good to me, given deprecation is what we want to go with. I think Peter had different ideas.

Wild idea: would it make sense to create a Logger interface in each package where it is imported to decouple them from the log package.

[peterbourgon]

Maybe worth a little note here indicating our intent re: compatibility?

[ChrisHines]

Sure, happy to do that, but I'm not sure I know what the consensus is on that topic. Would you mind sharing your thoughts in more detail?

[peterbourgon]

Hmm, yeah, there are lots of unknowns re: the impact of removing this package from the kit module altogether. Let's defer for now.

[peterbourgon]

I had it in my head that they should be separate packages. I can't recall a precise technical reason why this was a good idea. And I've encountered more than one situation, now, where it's been rather annoying that go-kit/kit/log wasn't an alias for, or equivalent to, or etc. go-kit/log. So I'm basically +1. What things do we need to be careful about?

[ChrisHines]

What things do we need to be careful about?

I intentionally left behind the example tests in the gutted packages for two reasons. First, to maintain their role as documentation, and second to serve as a small test that the APIs still work as before.

We should be triply sure that I didn't accidentally cross any wires on any of the forward references. Make sure I didn't alias the wrong type, variable, function, or constant anywhere.

One good sign: With a local replace directive to use this PR code in the go-kit/examples/go.mod file all the examples build successfully for me even though they all still import kit/log. I don't see any of the examples importing any kit/log/... packages, though, so we may want to find some code that uses those and make sure they still build properly.

I have some projects that use kit/log and kit/log/level that I can validate. I can't think of any projects I have that use the other modified packages off the top of my head.

[ChrisHines]

Wild idea: would it make sense to create a Logger interface in each package where it is imported to decouple them from the log package.

That would certainly decouple them more than they are today. I'm not sure it's a good idea, but I am not completely opposed.

If we did that I would expect the docs for the new declarations to be a copy/paste of the existing Logger interface to help remind people of its full contract. I would also expect an additional comment referring people to the log package so they can find those ready made implementations easily.

[peterbourgon]

Wild idea: would it make sense to create a Logger interface in each package where it is imported to decouple them from the log package.

My initial reaction was -1, but upon reflection I'm less negative. In any case we can defer this til later.

[peterbourgon]

@ChrisHines I'll check all the module/package import references carefully, but assuming that's kosher, you're OK with merging, is that right?

[ChrisHines]

@ChrisHines I'll check all the module/package import references carefully, but assuming that's kosher, you're OK with merging, is that right?

Correct.

[ChrisHines]

As an additional datapoint I used a replace directive to bring this branch into a project I have that uses kit/log throughout. Everything built fine. I also replaced a single import of kit/log with the new log repo, leaving all other imports in the project alone. Again everything built fine. That should verify the interchangeability of at least the log and log/level aliases.

[peterbourgon]

Some of the imports within go-kit/kit/log, for example in the tests, are still pointing to go-kit/kit/log —

ugh ~/src/kit (use-new-log) rg '/kit/log'
log/zap/zap_sugar_logger_test.go
5:      kitzap "github.com/go-kit/kit/log/zap"

log/deprecated_levels/levels_test.go
8:      levels "github.com/go-kit/kit/log/deprecated_levels"

log/term/example_test.go
7:      "github.com/go-kit/kit/log"
8:      "github.com/go-kit/kit/log/term"

...

Is that deliberate?

[ChrisHines]

Yes. kit/log/zap and kit/log/deprecated_levels were not kept in the new log repo, so those only exist in the kit repo. The imports in the example_test.go files intentionally refer to the kit/log packages as a small way to keep testing that the aliases work as intended.

[peterbourgon]

OK! I'll merge this tomorrow unless there's an objection.

[bboreham]

Note that this breaks DefaultCaller, as there is now one extra stack level for the forwarding function.

[ChrisHines]

Note that this breaks DefaultCaller, as there is now one extra stack level for the forwarding function.

Thanks for pointing that out. Feel free to submit a PR (if you want) to update DefaultCaller to the new value it should have.

[bboreham]

There is no single value that works for go-kit/log and go-kit/kit/log, without reverting this PR.

[peterbourgon]

Could we revert just the DefaultCaller definition away from the current alias and back to a definition in terms of a Caller?

[bboreham]

If someone used purely go-kit/kit/log, that would work.
If someone has a program where parts call go-kit/log and parts call go-kit/kit/log (and most of my programs are like that now), then one or the other will print something unhelpful for caller.

[peterbourgon]

My thinking is that, even if a program imported both the old and new module, a given log.DefaultCaller identifier would refer to precisely one of them, and it should (?) be possible to define both such that they produce the correct result when resolved.

Would this patch (or something like it) to this repo be insufficient?

- DefaultCaller = log.DefaultCaller
+ DefaultCaller = Caller(3)

(It is entirely possible that I am being dense.)

[peterbourgon]

Ah, maybe you're saying there's interplay with the normal usage via With which would make depth unpredictable?

[bboreham]

In my version of normal usage, there would be one With ... Caller.
There would be multiple calls to Log(), some of which have the desired line 3 stack frames up, some have it 4 frames up.

[ChrisHines]

Perhaps we can start with a test that reproduces the problem.

[bboreham]

I'm sorry, I was completely wrong.
I did do extensive testing to confirm the problem, but at a key point had edited go.mod to use v0.11.0 of go-kit.
Putting it back to v0.12.0 (after this PR) all is fine, because all the key calls go through interface dispatch to the new library.

[ChrisHines]

I'm sorry you had a scare, but I am very happy this turns out to be a non-issues. Thanks for following up.