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

Will add some more E2E and unit tests.

Author: gagik

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

@@ -238,6 +238,27 @@ describe('CliRepl', function () {
         });
       });
 
+      it('does not write to log syntax errors if logging is disabled', async function () {
+        expect(
+          (await log()).filter((entry) =>
+            entry.attr?.stack?.startsWith('SyntaxError:')
+          )
+        ).to.have.lengthOf(0);
+        input.write('config.set("disableLogging", true)\n');
+        await waitEval(cliRepl.bus);
+        expect(output).includes('Setting "disableLogging" has been changed');
+
+        input.write('<cat>\n');
+        await waitBus(cliRepl.bus, 'mongosh:error');
+        await eventually(async () => {
+          expect(
+            (await log()).filter((entry) =>
+              entry.attr?.stack?.startsWith('SyntaxError:')
+            )
+          ).to.have.lengthOf(0);
+        });
+      });
+
       it('writes JS errors to the log file', async function () {
         input.write('throw new Error("plain js error")\n');
         await waitBus(cliRepl.bus, 'mongosh:error');
@@ -1365,29 +1386,16 @@ describe('CliRepl', function () {
 
         context('logging configuration', function () {
           it('logging is enabled by default', async function () {
-            const onLoggerInitialized = sinon.stub();
-            cliRepl.bus.on('mongosh:logger-initialized', onLoggerInitialized);
+            const onLogInitialized = sinon.stub();
+            cliRepl.bus.on('mongosh:log-initialized', onLogInitialized);
 
             await cliRepl.start(await testServer.connectionString(), {});
 
             expect(await cliRepl.getConfig('disableLogging')).is.false;
 
-            expect(onLoggerInitialized).calledOnce;
+            expect(onLogInitialized).calledOnce;
             expect(cliRepl.logWriter).is.instanceOf(MongoLogWriter);
           });
-
-          it('does not start logging when it is disabled', async function () {
-            cliRepl.config.disableLogging = true;
-            const onLoggerInitialized = sinon.stub();
-            cliRepl.bus.on('mongosh:logger-initialized', onLoggerInitialized);
-
-            await cliRepl.start(await testServer.connectionString(), {});
-
-            expect(await cliRepl.getConfig('disableLogging')).is.true;
-            expect(onLoggerInitialized).not.called;
-
-            expect(cliRepl.logWriter).is.undefined;
-          });
         });
 
         it('times out fast', async function () {

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

@@ -30,11 +30,11 @@ import type { MongoLogWriter } from 'mongodb-log-writer';
 import { MongoLogManager, mongoLogId } from 'mongodb-log-writer';
 import type { MongoshNodeReplOptions, MongoshIOProvider } from './mongosh-repl';
 import MongoshNodeRepl from './mongosh-repl';
+import { MongoshLoggingAndTelemetry } from '@mongosh/logging';
 import {
   ToggleableAnalytics,
   ThrottledAnalytics,
   SampledAnalytics,
-  MongoshLoggingAndTelemetry,
 } from '@mongosh/logging';
 import type { MongoshBus } from '@mongosh/types';
 import {
@@ -119,7 +119,7 @@ export class CliRepl implements MongoshIOProvider {
   config: CliUserConfigOnDisk;
   globalConfig: Partial<CliUserConfig> | null = null;
   globalConfigPaths: string[];
-  logManager: MongoLogManager;
+  logManager?: MongoLogManager;
   logWriter?: MongoLogWriter;
   input: Readable;
   output: Writable;
@@ -140,6 +140,8 @@ export class CliRepl implements MongoshIOProvider {
   fetchMongoshUpdateUrlRegardlessOfCiEnvironment = false; // for testing
   cachedGlibcVersion: null | string | undefined = null;
 
+  private loggingAndTelemetry: MongoshLoggingAndTelemetry | undefined;
+
   /**
    * Instantiate the new CLI Repl.
    */
@@ -189,16 +191,6 @@ export class CliRepl implements MongoshIOProvider {
         });
       });
 
-    this.logManager = new MongoLogManager({
-      directory: this.shellHomeDirectory.localPath('.'),
-      retentionDays: 30,
-      maxLogFileCount: +(
-        process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100
-      ),
-      onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
-      onwarn: (err: Error, path: string) =>
-        this.warnAboutInaccessibleFile(err, path),
-    });
     this.agent = useOrCreateAgent(this.proxyOptions);
     this.updateNotificationManager = new UpdateNotificationManager({
       proxyOptions: this.agent,
@@ -257,9 +249,24 @@ export class CliRepl implements MongoshIOProvider {
   }
 
   /** Setup log writer and start logging. */
-  private async startLogging(
-    loggingAndTelemetry: MongoshLoggingAndTelemetry
-  ): Promise<void> {
+  private async startLogging(): Promise<void> {
+    if (!this.loggingAndTelemetry) {
+      throw new Error('Logging and telemetry not setup');
+    }
+
+    if (!this.logManager) {
+      this.logManager = new MongoLogManager({
+        directory: this.shellHomeDirectory.localPath('.'),
+        retentionDays: 30,
+        maxLogFileCount: +(
+          process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100
+        ),
+        onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
+        onwarn: (err: Error, path: string) =>
+          this.warnAboutInaccessibleFile(err, path),
+      });
+    }
+
     await this.logManager.cleanupOldLogFiles();
     markTime(TimingCategories.Logging, 'cleaned up log files');
     const logger = await this.logManager.createLogWriter();
@@ -269,7 +276,7 @@ export class CliRepl implements MongoshIOProvider {
     }
 
     this.logWriter = logger;
-    loggingAndTelemetry.setupLogger(logger);
+    this.loggingAndTelemetry.attachLogger(logger);
 
     markTime(TimingCategories.Logging, 'instantiated log writer');
     logger.info('MONGOSH', mongoLogId(1_000_000_000), 'log', 'Starting log', {
@@ -343,7 +350,7 @@ export class CliRepl implements MongoshIOProvider {
 
     markTime(TimingCategories.Telemetry, 'created analytics instance');
 
-    const loggingAndTelemetry = new MongoshLoggingAndTelemetry(
+    this.loggingAndTelemetry = new MongoshLoggingAndTelemetry(
       this.bus,
       this.toggleableAnalytics,
       {
@@ -354,7 +361,7 @@ export class CliRepl implements MongoshIOProvider {
       },
       version
     );
-    loggingAndTelemetry.setup();
+    this.loggingAndTelemetry.setup();
 
     markTime(TimingCategories.Telemetry, 'completed telemetry setup');
 
@@ -374,10 +381,7 @@ export class CliRepl implements MongoshIOProvider {
     this.globalConfig = await this.loadGlobalConfigFile();
     markTime(TimingCategories.UserConfigLoading, 'read global config files');
 
-    const disableLogging = await this.getConfig('disableLogging');
-    if (disableLogging !== true) {
-      await this.startLogging(loggingAndTelemetry);
-    }
+    await this.setLoggingEnabled(!(await this.getConfig('disableLogging')));
 
     // Needs to happen after loading the mongosh config file(s)
     void this.fetchMongoshUpdateUrl();
@@ -622,6 +626,19 @@ export class CliRepl implements MongoshIOProvider {
     );
   }
 
+  async setLoggingEnabled(enabled: boolean): Promise<void> {
+    if (enabled) {
+      if (!this.logWriter) {
+        await this.startLogging();
+      }
+      // Do nothing if there is already an active log writer.
+    } else {
+      this.loggingAndTelemetry?.detachLogger();
+      this.logWriter?.destroy();
+      this.logWriter = undefined;
+    }
+  }
+
   setTelemetryEnabled(enabled: boolean): void {
     if (this.globalConfig === null) {
       // This happens when the per-user config file is loaded before we have
@@ -908,6 +925,9 @@ export class CliRepl implements MongoshIOProvider {
         anonymousId: this.config.telemetryAnonymousId,
       });
     }
+    if (key === 'disableLogging') {
+      await this.setLoggingEnabled(!value);
+    }
     try {
       await this.configDirectory.writeConfigFile(this.config);
     } catch (err: any) {

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';
 
 const jsContextFlagCombinations: `--jsContext=${'plain-vm' | 'repl'}`[][] = [
   [],
@@ -1360,10 +1361,9 @@ describe('e2e', function () {
     let shell: TestShell;
     let configPath: string;
     let logBasePath: string;
-    let logPath: string;
     let historyPath: string;
     let readConfig: () => Promise<any>;
-    let readLogfile: () => Promise<any[]>;
+    let readLogfile: () => Promise<LogEntry[]>;
     let startTestShell: (...extraArgs: string[]) => Promise<TestShell>;
 
     beforeEach(function () {
@@ -1400,7 +1400,13 @@ describe('e2e', function () {
       }
       readConfig = async () =>
         EJSON.parse(await fs.readFile(configPath, 'utf8'));
-      readLogfile = async () => readReplLogfile(logPath);
+      readLogfile = async () => {
+        if (!shell.logId) {
+          throw new Error('Shell does not have a logId associated with it');
+        }
+        const logPath = path.join(logBasePath, `${shell.logId}_log`);
+        return readReplLogfile(logPath);
+      };
       startTestShell = async (...extraArgs: string[]) => {
         const shell = this.startTestShell({
           args: ['--nodb', ...extraArgs],
@@ -1431,7 +1437,6 @@ describe('e2e', function () {
       beforeEach(async function () {
         await fs.mkdir(homedir, { recursive: true });
         shell = await startTestShell();
-        logPath = path.join(logBasePath, `${shell.logId}_log`);
       });
 
       describe('config file', function () {
@@ -1510,14 +1515,61 @@ describe('e2e', function () {
       });
 
       describe('log file', function () {
+        it('does not create a log if global config has disableLogging', async function () {
+          const globalConfig = path.join(homedir, 'globalconfig.conf');
+          await fs.writeFile(globalConfig, 'mongosh:\n  disableLogging: true');
+          shell = this.startTestShell({
+            args: ['--nodb'],
+            env: {
+              ...env,
+              MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig,
+            },
+            forceTerminal: true,
+          });
+          await shell.waitForPrompt();
+          expect(
+            await shell.executeLine('config.get("disableLogging")')
+          ).to.include('true');
+          shell.assertNoErrors();
+
+          expect(await shell.executeLine('print(123 + 456)')).to.include('579');
+          expect(shell.logId).equals(null);
+        });
+
         it('creates a log file that keeps track of session events', async function () {
           expect(await shell.executeLine('print(123 + 456)')).to.include('579');
-          await eventually(async () => {
-            const log = await readLogfile();
-            expect(
-              log.filter((logEntry) => /Evaluating input/.test(logEntry.msg))
-            ).to.have.lengthOf(1);
-          });
+          const log = await readLogfile();
+          expect(
+            // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
+            log.filter(
+              (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
+            )
+          ).to.have.lengthOf(1);
+        });
+
+        it('does not write to the log file after disableLogging is set to true', async function () {
+          expect(await shell.executeLine('print(123 + 456)')).to.include('579');
+          const log = await readLogfile();
+          expect(
+            log.filter(
+              (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
+            )
+          ).to.have.lengthOf(1);
+
+          await shell.executeLine(`config.set("disableLogging", true)`);
+          expect(await shell.executeLine('print(579 - 123)')).to.include('456');
+
+          const logAfterDisabling = await readLogfile();
+          expect(
+            logAfterDisabling.filter(
+              (logEntry) => logEntry.attr?.input === 'print(579 - 123)'
+            )
+          ).to.have.lengthOf(0);
+          expect(
+            logAfterDisabling.filter(
+              (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
+            )
+          ).to.have.lengthOf(1);
         });
 
         it('includes information about the driver version', async function () {