feat(logging): add option to disable logging MONGOSH-1988

[gagik]

Includes a refactor for better readability and to provide an easier interface for testing and debugging the functionality of this. This also moves the hookLoggers call to the point of initialization.

[gagik]

I think structuring it as a class provided much better readability and an easier interface for us to test and debug the functionality of this compared to a function. Open to using the function structure though if there's a reason I may be missing here, I started doing it that way and just realized the ability to stub and access the scope of the class may be worth some greater changes.

Naming of it I'm less sure of.

[addaleax]

A couple of thoughts:

• Turning this into a class seems fine as a way of acknowleding that this code has a bit of state that it manages on its own.
• "the ability to stub and access the scope of the class" is usually a bad reason to turn something into a class – exposing internal state as a way to test something is often a code smell that means that the API of what you're testing doesn't directly allow you to observe behavior that should be externally visible
• This kind of change should probably be mentioned in the tech design and ideally addressed as a separate item, so it's more obvious what the refactor and the functional changes are, each (probably not necessary at this point since disentangling this is probably more work than not) – and so the review process doesn't get too bloated 🙂

[gagik]

That makes sense, I did think it may be too big of a change. I do not mind writing this into a function, I was thinking of doing that first and then introducing an extra commit I could reverse but seemed easier for me to reason to have this state first.

[addaleax]

Yeah, to be clear, this is not something that needs to change again here :)

[gagik]

As in, it could be left as a class?

Also, with this:

exposing internal state as a way to test something is often a code smell that means that the API of what you're testing doesn't directly allow you to observe behavior that should be externally visible

do I understand correctly that the flaw here is that tests should focus on testing expected behavior and not some arbitrary internal state one exposes?

[addaleax]

Yeah, leaving it as a class seems fine. I'd maybe consider marking as many of the properties private as possible, but otherwise it's not like "function with a bunch of local variables" and "class with a bunch of properties" are terribly different concepts.

exposing internal state as a way to test something is often a code smell that means that the API of what you're testing doesn't directly allow you to observe behavior that should be externally visible

do I understand correctly that the flaw here is that tests should focus on testing expected behavior and not some arbitrary internal state one exposes?

Yeah, exactly. If I get to a point where I'd want to test internal state, that's usually a good point to wonder if maybe the class can be broken down into smaller pieces that can be unit tested individually, rather than starting to rely on the ability to inspect and/or mock internals. But testing externally visible behavior should generally take precedence anyway. 🙂

[gagik]

otherwise it's not like "function with a bunch of local variables" and "class with a bunch of properties" are terribly different concepts

good throwback to pre-ES6 JavaScript I guess.

sounds good, thank you for the explanation 😀

[addaleax]

A couple of thoughts:

• Turning this into a class seems fine as a way of acknowleding that this code has a bit of state that it manages on its own.
• "the ability to stub and access the scope of the class" is usually a bad reason to turn something into a class – exposing internal state as a way to test something is often a code smell that means that the API of what you're testing doesn't directly allow you to observe behavior that should be externally visible
• This kind of change should probably be mentioned in the tech design and ideally addressed as a separate item, so it's more obvious what the refactor and the functional changes are, each (probably not necessary at this point since disentangling this is probably more work than not) – and so the review process doesn't get too bloated 🙂

[addaleax]

You'll want to add an e2e test for this as well, maybe even including one that tests that the global config file can disable logging via this option

[gagik]

Seemed best to move all variables into the class state and keep the setup listeners function scope clean of state but if it's making the class too bloated, could reconsider (or maybe scope them under eventListenersState or something)