logger interface package

[mjibson]

No description provided.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[benesch]

but please wait for at least @tschottdorf's review, and maybe @bdarnell's too?

Reviewed 2 of 2 files at r1, 56 of 56 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/util/log/logger/logger.go, line 35 at r1 (raw file):

	SetExitFunc(hideStack bool, f func(int))
	// Warningf logs to the WARNING and INFO logs.
	Warningf(ctx context.Context, format string, args ...interface{})

Seems worth including Errorf and Infof, because we'll definitely need those soon.

I also think we should exclude ResetExitFunc() and SetExitFunc(...). Those are hideous functions. In the HLC tests that need them, I think you should create a concrete logger, and just call SetExitFunc on that custom logger. Eventually we won't even need either of these functions, since we can just install the exit function when we create the logger. Hooray for testable code without global mutable state!

---

pkg/util/log/logger/logger.go, line 20 at r2 (raw file):

import "context"

// Log defines a logger.

nit: wrong commit

[danhhz]

Is that plan that we'll migrate every usage of log to this at some point? If so, in the spirit of ben's recent refactor email, I'd love to hold off merging until cdc is no longer actively cherrypicking literally every commit.

[danhhz]

Is that plan that we'll migrate every usage of log to this at some point? If so, in the spirit of ben's recent refactor email, I'd love to hold off merging until cdc is no longer actively cherrypicking literally every commit.

[bdarnell]

What's the motivation for this? How far will it go? If this is going to turn into another round of adding a parameter to every method (as there was for context.Context), it needs a very strong justification. I don't mind it for isolated cases like this, but I don't like the idea of it spreading to everything that logs.

[bdarnell]

What's the motivation for this? How far will it go? If this is going to turn into another round of adding a parameter to every method (as there was for context.Context), it needs a very strong justification. I don't mind it for isolated cases like this, but I don't like the idea of it spreading to everything that logs.

[petermattis]

Rather than a logger member variable, could we have per-package logger globals, similar to grpc? I suppy I should understand the motivation better before offering alternatives.

…

On Mon, Oct 15, 2018 at 5:39 PM Ben Darnell ***@***.***> wrote: What's the motivation for this? How far will it go? If this is going to turn into another round of adding a parameter to every method (as there was for context.Context), it needs a very strong justification. I don't mind it for isolated cases like this, but I don't like the idea of it spreading to everything that logs. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#31403 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6f931bpM3dSt4VKZF1wKNa486swh_Tks5ulQCSgaJpZM4XdFgM> .

[petermattis]

Rather than a logger member variable, could we have per-package logger globals, similar to grpc? I suppy I should understand the motivation better before offering alternatives.

…

On Mon, Oct 15, 2018 at 5:39 PM Ben Darnell ***@***.***> wrote: What's the motivation for this? How far will it go? If this is going to turn into another round of adding a parameter to every method (as there was for context.Context), it needs a very strong justification. I don't mind it for isolated cases like this, but I don't like the idea of it spreading to everything that logs. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#31403 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6f931bpM3dSt4VKZF1wKNa486swh_Tks5ulQCSgaJpZM4XdFgM> .

[benesch]

There was some discussion in #engineering. The justification is twofold. The first reason is that forcing a dependency on util/log forces downstream clients to take a dependency on a logging package they probably don't want to use. util/hlc is actually likely to be quite useful outside of Cockroach. I think you could probably solve this problem with a per-package logger.

The second reason is that we can't run Go tests within a package in parallel right now because every in-process test server shares the same logger. We feel this most acutely in the acceptance package, which has a bunch of tests that are not at all resource intensive but are slow because of Docker. (See #21724 for more context.) I don't think you can solve this particular problem without plumbing a logger to every caller that needs it.

[benesch]

There was some discussion in #engineering. The justification is twofold. The first reason is that forcing a dependency on util/log forces downstream clients to take a dependency on a logging package they probably don't want to use. util/hlc is actually likely to be quite useful outside of Cockroach. I think you could probably solve this problem with a per-package logger.

The second reason is that we can't run Go tests within a package in parallel right now because every in-process test server shares the same logger. We feel this most acutely in the acceptance package, which has a bunch of tests that are not at all resource intensive but are slow because of Docker. (See #21724 for more context.) I don't think you can solve this particular problem without plumbing a logger to every caller that needs it.

[bdarnell]

I'm fine with this change for HLC to make it exportable.

Why exactly does the log singleton prevent you from running tests in parallel? Is it just that the intermingled logs are unintelligible? Parallelizing tests by running child processes might be better than allowing multiple loggers within one process.

[bdarnell]

I'm fine with this change for HLC to make it exportable.

Why exactly does the log singleton prevent you from running tests in parallel? Is it just that the intermingled logs are unintelligible? Parallelizing tests by running child processes might be better than allowing multiple loggers within one process.

[benesch]

Why exactly does the log singleton prevent you from running tests in parallel? Is it just that the intermingled logs are unintelligible?

Yes, precisely.

Parallelizing tests by running child processes might be better than allowing multiple loggers within one process.

Yeah, I think this works as long as the only thing under test is the cockroach binary, and everything else in the test is careful to never use util/log. That's not the case today, though: the acceptance test infrastructure itself uses util/log. I guess we could unsingletonize that section of the logging code.

Anyway, it sounds like there is consensus building around doing this for util/hlc and then evaluating future unsingletonization on a case-by-case basis.

[benesch]

Why exactly does the log singleton prevent you from running tests in parallel? Is it just that the intermingled logs are unintelligible?

Yes, precisely.

Parallelizing tests by running child processes might be better than allowing multiple loggers within one process.

Yeah, I think this works as long as the only thing under test is the cockroach binary, and everything else in the test is careful to never use util/log. That's not the case today, though: the acceptance test infrastructure itself uses util/log. I guess we could unsingletonize that section of the logging code.

Anyway, it sounds like there is consensus building around doing this for util/hlc and then evaluating future unsingletonization on a case-by-case basis.

[mjibson]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/util/log/logger/logger.go, line 35 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Seems worth including Errorf and Infof, because we'll definitely need those soon.

I also think we should exclude ResetExitFunc() and SetExitFunc(...). Those are hideous functions. In the HLC tests that need them, I think you should create a concrete logger, and just call SetExitFunc on that custom logger. Eventually we won't even need either of these functions, since we can just install the exit function when we create the logger. Hooray for testable code without global mutable state!

Added Errorf and Infof and removed ResetExitFunc and SetExitFunc (which I erroneously included because hlc_test.go used them).

[benesch]

Reviewed 57 of 57 files at r3, 55 of 55 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/util/log/logger/logger.go, line 31 at r3 (raw file):

	Infof(ctx context.Context, format string, args ...interface{})
	// Warningf logs to the WARNING and INFO logs.
	Warningf(ctx context.Context, format string, args ...interface{})

This is super clean. I love it.