feat(logging): add configuration to disable logging MONGOSH-1988 #2325

gagik · 2025-01-22T14:18:00Z

Adds ability to disable logging completely both through global file and local configurations.

packages/logging/src/logging-and-telemetry.ts

packages/cli-repl/src/cli-repl.spec.ts

packages/cli-repl/src/cli-repl.ts

packages/logging/src/logging-and-telemetry.ts

addaleax · 2025-01-27T11:23:36Z

packages/cli-repl/src/cli-repl.ts

+    const disableLogging = this.getConfig('disableLogging');
+    if (disableLogging !== true) {
+      await this.startLogging(loggingAndTelemetry);
+    }


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 · 2025-01-27T12:47:11Z

packages/logging/src/logging-and-telemetry.ts

+   * be aware of this distinction. */
+  private hasStartedMongoshRepl = false;
+
+  private apiCallTracking = {


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)

gagik · 2025-01-30T13:39:00Z

packages/cli-repl/src/cli-repl.ts

+    } else {
+      this.loggingAndTelemetry?.detachLogger();
+      this.logWriter?.destroy();
+      this.logWriter = undefined;


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")

packages/cli-repl/src/cli-repl.ts

packages/logging/src/logging-and-telemetry.ts

addaleax · 2025-01-30T15:21:50Z

packages/logging/src/logging-and-telemetry.ts

+  private userId: string | undefined;
+  private isDummyLog = true;
+  private isSetup = false;
+  private hookedExternalListenersLogId: string | undefined = undefined;


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

packages/logging/src/logging-and-telemetry.ts

packages/cli-repl/src/cli-repl.ts

gagik · 2025-01-31T12:19:54Z

packages/logging/src/logging-and-telemetry.ts

-    return {
-      anonymousId: this.telemetryAnonymousId ?? (this.userId as string),
+  /** Information used and set by different bus events. */
+  private busEventState: LoggingAndTelemetryBusEventState = {


seemed best for separating these kind of properties from the state though might need a better name.
can also be moved inside setupBusEventListeners

gagik · 2025-01-31T12:21:30Z

packages/logging/src/logging-and-telemetry.ts

+    hookLogger(
+      this.bus,
+      {
+        info: (...args: Parameters<typeof this.log.info>) => {


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?

Do we want to specify them here or potentially update LogWriter i.e. use an interface?

I think that would be a very nice follow-up, but not required for this PR since hookLogger already defines the interface it needs

addaleax

Looks great! Just a few (final 🤞) comments

addaleax · 2025-02-04T14:43:06Z

packages/e2e-tests/test/e2e.spec.ts

@@ -22,6 +22,7 @@ import { once } from 'events';
 import type { AddressInfo } from 'net';
 const { EJSON } = bson;
 import { sleep } from './util-helpers';
+import type { LogEntry } from '@mongosh/shell-api/src/log-entry';


We generally don't do cross-package imports, if we want this, we should either export it from shell-api or (preferably) move it to @mongosh/types and share it from there.

All that being said – it's the wrong type 🙂 LogEntry uses full-form names like timestamp, severity and message, but actual log entries use short identifiers like t, s and msg

good catch, will replace with MongoLogEntry
https://github.com/mongodb-js/devtools-shared/blob/01e5c1c9c3563b47b1eefcfb6f0ffd6b9a2ef0f7/packages/mongodb-log-writer/src/mongo-log-writer.ts#L25

packages/e2e-tests/test/e2e.spec.ts

packages/logging/src/logging-and-telemetry.ts

addaleax · 2025-02-04T15:58:43Z

packages/logging/src/logging-and-telemetry.ts

+    hookLogger(
+      this.bus,
+      {
+        info: (...args: Parameters<typeof this.log.info>) => {


Do we want to specify them here or potentially update LogWriter i.e. use an interface?

I think that would be a very nice follow-up, but not required for this PR since hookLogger already defines the interface it needs

gagik · 2025-02-05T09:26:07Z

@addaleax @alenakhineika
do we usually merge stacked PRs i.e. #2355 like this or do we want to revert that, get this one into main and then merge the #2355 into main as well?

alenakhineika · 2025-02-05T09:46:30Z

From my side, I would merge them all into one and then merge them all into the main to be able to test the whole thing first. Let's wait for @addaleax thoughts about it.

addaleax · 2025-02-05T10:38:17Z

From my side, I would merge them all into one and then merge them all into the main to be able to test the whole thing first. Let's wait for @addaleax thoughts about it.

I don't see why we'd be more or less able to test this depending on how we merge things?

If this was just a "feature branch" PR, I think that would be okay, but right now it's a) a "feature branch" PR plus b) a refactor PR plus c) an actual feature PR, and that's something I'd like to get merged as quickly as possible because otherwise there's no way to properly the review the feature and refactor in isolation

packages/e2e-tests/test/e2e-tls.spec.ts

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.

…ing of the logs

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.

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.

* feat(cli-repl): do not wait for log clean-up MONGOSH-1990 * test: add test for log manager throwing error * test: remove only * fix: mark time after log cleanup

Change typing of LogEntry and use EJSON.parse, remove unnecessary eslint comment

… log undefined check

We use a type from the mongodb-log-writer but it was not in the dependencies, so we add it back to fix the check test.

gagik mentioned this pull request Jan 22, 2025

WIP feat!(mongodb-log-writer): add ability to disable logging MONGOSH-1988 mongodb-js/devtools-shared#500

Closed

gagik force-pushed the gagik/add-disable-logging branch 2 times, most recently from 93661c5 to 73ad25a Compare January 22, 2025 14:50

gagik changed the title ~~feat(cli-repl): add option to disable logging MONGOSH-1988~~ WIP feat(cli-repl): add option to disable logging MONGOSH-1988 Jan 22, 2025

gagik marked this pull request as draft January 22, 2025 16:20

gagik force-pushed the gagik/add-disable-logging branch 6 times, most recently from 3b38cc7 to ef79cf4 Compare January 24, 2025 16:44

gagik changed the title ~~WIP feat(cli-repl): add option to disable logging MONGOSH-1988~~ feat(cli-repl): add option to disable logging MONGOSH-1988 Jan 24, 2025

gagik commented Jan 26, 2025

View reviewed changes

packages/logging/src/logging-and-telemetry.ts Outdated Show resolved Hide resolved

packages/logging/src/logging-and-telemetry.ts Outdated Show resolved Hide resolved

gagik force-pushed the gagik/add-disable-logging branch from 054080c to 5147039 Compare January 26, 2025 22:00

gagik marked this pull request as ready for review January 26, 2025 22:00

addaleax requested changes Jan 27, 2025

View reviewed changes

gagik changed the title ~~feat(cli-repl): add option to disable logging MONGOSH-1988~~ feat(logging): add option to disable logging MONGOSH-1988 Jan 27, 2025

gagik commented Jan 27, 2025

View reviewed changes

gagik marked this pull request as draft January 29, 2025 13:04

gagik force-pushed the gagik/add-disable-logging branch 2 times, most recently from 275b7ad to 214ca17 Compare January 30, 2025 13:31

gagik commented Jan 30, 2025

View reviewed changes

gagik requested a review from addaleax January 30, 2025 13:43

gagik marked this pull request as ready for review January 30, 2025 13:43

gagik commented Jan 30, 2025

View reviewed changes

packages/logging/src/logging-and-telemetry.ts Outdated Show resolved Hide resolved

gagik force-pushed the gagik/add-disable-logging branch from 51ef2b7 to 6f30993 Compare January 30, 2025 14:57

addaleax reviewed Jan 30, 2025

View reviewed changes

gagik commented Jan 31, 2025

View reviewed changes

gagik force-pushed the gagik/add-disable-logging branch 2 times, most recently from d063090 to df96f8d Compare January 31, 2025 12:30

gagik force-pushed the gagik/add-disable-logging branch 2 times, most recently from 1bbbfb0 to adf5199 Compare January 31, 2025 12:41

addaleax reviewed Feb 4, 2025

View reviewed changes

gagik changed the title ~~feat(logging): add option to disable logging MONGOSH-1988~~ feat(logging): add configuration to disable logging MONGOSH-1988 Feb 5, 2025

gagik force-pushed the gagik/add-disable-logging branch 2 times, most recently from e7486f8 to d42d455 Compare February 5, 2025 09:56

addaleax reviewed Feb 5, 2025

View reviewed changes

packages/e2e-tests/test/e2e-tls.spec.ts Outdated Show resolved Hide resolved

gagik force-pushed the gagik/add-disable-logging branch from 1c799b2 to a743897 Compare February 5, 2025 11:45

addaleax approved these changes Feb 5, 2025

View reviewed changes

gagik and others added 15 commits February 5, 2025 14:00

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

b8f16f5

Some tests still need to be fixed

fix(tests): add validation for CliUserConfig

71ce8b5

Adds validation for the disableLogging field and fixes the equivalent test.

refactor(cli-repl): apply changes from review

23d3f21

Use async getConfig, add return types, remove unnecessary eslint comments, move bus variables into class state, use explicit private and public fields.

tests: add initial set of tests

e5ae4dd

Will follow up with more end to end tests.

feat: add ability to detach logger and use only telemetry and more tests

da2b5b3

Will add some more E2E and unit tests.

tests: add e2e tests for global config and midway disabling and enabl…

0858257

…ing of the logs

refactor: move custom log entries into new structure

6311256

refactor: use a setup function and simplify state

f0b46ce

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.

feat(cli-repl): do not wait for log clean-up MONGOSH-1990 (#2355)

fb3e0a2

* feat(cli-repl): do not wait for log clean-up MONGOSH-1990 * test: add test for log manager throwing error * test: remove only * fix: mark time after log cleanup

fix: suggestions from code review

ebb7e16

Change typing of LogEntry and use EJSON.parse, remove unnecessary eslint comment

fix: rename readReplLogfile to readReplLogFile and remove unnecessary…

a9fd752

… log undefined check

fix(tests): readjust type of readReplLogFile

417c411

fix(tests): add log-writer to resolve depcheck error

0ffae4c

We use a type from the mongodb-log-writer but it was not in the dependencies, so we add it back to fix the check test.

gagik force-pushed the gagik/add-disable-logging branch from ce7e5b9 to 0ffae4c Compare February 5, 2025 13:01

gagik merged commit b340d79 into main Feb 5, 2025
52 of 55 checks passed

gagik deleted the gagik/add-disable-logging branch February 5, 2025 13:52

feat(logging): add configuration to disable logging MONGOSH-1988 #2325

feat(logging): add configuration to disable logging MONGOSH-1988 #2325

Uh oh!

Conversation

gagik commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

gagik Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

gagik Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

addaleax Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gagik Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

gagik Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

gagik commented Feb 5, 2025

Uh oh!

alenakhineika commented Feb 5, 2025

Uh oh!

addaleax commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gagik commented Jan 22, 2025 •

edited

Loading

gagik Jan 31, 2025 •

edited

Loading

gagik Jan 31, 2025 •

edited

Loading

gagik Feb 5, 2025 •

edited

Loading