feat(logging): add option to disable logging MONGOSH-1988 #2325
Conversation
gagik
force-pushed
the
gagik/add-disable-logging
branch
2 times, most recently
from
93661c5 to
73ad25a
Compare
gagik
changed the title
gagik
force-pushed
the
gagik/add-disable-logging
branch
6 times, most recently
from
3b38cc7 to
ef79cf4
Compare
gagik
changed the title
gagik
force-pushed
the
gagik/add-disable-logging
branch
from
054080c to
5147039
Compare
packages/cli-repl/src/cli-repl.ts
Outdated
| const disableLogging = this.getConfig('disableLogging'); | ||
| if (disableLogging !== true) { | ||
| await this.startLogging(loggingAndTelemetry); | ||
| } |
There was a problem hiding this comment.
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
changed the title
| * be aware of this distinction. */ | ||
| private hasStartedMongoshRepl = false; | ||
|
|
||
| private apiCallTracking = { |
There was a problem hiding this comment.
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)
Some tests still need to be fixed
…initialization I think this refactor provides much better readability and provides an easier interface for us to test and debug the functionality of this. This also moves the hookLoggers call to the point of initialization as
Adds validation for the disableLogging field and fixes the equivalent test.
Use async getConfig, add return types, remove unnecessary eslint comments, move bus variables into class state, use explicit private and public fields.
Will follow up with more end to end tests.
Will add some more E2E and unit tests.
gagik
force-pushed
the
gagik/add-disable-logging
branch
2 times, most recently
from
275b7ad to
214ca17
Compare
packages/cli-repl/src/cli-repl.ts
Outdated
| } else { | ||
| this.loggingAndTelemetry?.detachLogger(); | ||
| this.logWriter?.destroy(); | ||
| this.logWriter = undefined; |
There was a problem hiding this comment.
So we create a new logger (and new logId) each time logging gets reenabled, which also means if someone disable and then enables logging, a new file is created.
I think that is reasonable unless we'd rather make sure it writes to the same file again (thinking of it more of a "pause")
Wrapping the bus can allow us to inject onBus onto the logger which can help us prevent logs being created from there. That said, we still have the problem of having to either re-register those hooks each time, so we may want to instead update hookLogger to take function for getting the latest log rather than just a variable.
gagik
force-pushed
the
gagik/add-disable-logging
branch
from
51ef2b7 to
6f30993
Compare
| private userId: string | undefined; | ||
| private isDummyLog = true; | ||
| private isSetup = false; | ||
| private hookedExternalListenersLogId: string | undefined = undefined; |
There was a problem hiding this comment.
I feel like this class's state space exploded a bit now, which makes this pretty hard to review and probably also hard to work with in the future
e.g. it's hard to tell whether isDummyLog = false, isSetup = true, log = null, pendingLogEvents = [...], hookedExternalListenersLogId = 'foo' is a valid state for this class or not and what that means specifically
I'll try to get more specific at individual points here but I think we might want to rethink this and look for opportunities to make this simpler
| return { | ||
| anonymousId: this.telemetryAnonymousId ?? (this.userId as string), | ||
| /** Information used and set by different bus events. */ | ||
| private busEventState: LoggingAndTelemetryBusEventState = { |
There was a problem hiding this comment.
seemed best for separating these kind of properties from the state though might need a better name.
can also be moved inside setupBusEventListeners
| hookLogger( | ||
| this.bus, | ||
| { | ||
| info: (...args: Parameters<typeof this.log.info>) => { |
There was a problem hiding this comment.
technically there are more methods in this... quite a few needed to actually satisfy the MongoLogWriter type since it has a bunch of internal / private methods exposed. Do we want to specify them here or potentially update LogWriter i.e. use an interface?
gagik
force-pushed
the
gagik/add-disable-logging
branch
3 times, most recently
from
df96f8d to
1bbbfb0
Compare
Moved public-facing API to a function and interface. Reuse the same log writer in cli-repl. Removed session_id as a stored state and instead provide it dynamically through this.log.
gagik
force-pushed
the
gagik/add-disable-logging
branch
from
1bbbfb0 to
adf5199
Compare
Adds ability to disable logging completely both through global file and local configurations.