refactor: use a setup function and simplify state

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.

Author: gagik

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

@@ -18,7 +18,7 @@ import {
 import {
   expect,
   fakeTTYProps,
-  readReplLogfile,
+  readReplLogFile,
   tick,
   useTmpdir,
   waitBus,
@@ -56,7 +56,7 @@ describe('CliRepl', function () {
   async function log(): Promise<any[]> {
     if (!cliRepl.logWriter?.logFilePath) return [];
     await cliRepl.logWriter.flush(); // Ensure any pending data is written first
-    return readReplLogfile(cliRepl.logWriter.logFilePath);
+    return readReplLogFile(cliRepl.logWriter.logFilePath);
   }
 
   async function startWithExpectedImmediateExit(
@@ -1559,11 +1559,11 @@ describe('CliRepl', function () {
 
         it('includes a statement about flushed telemetry in the log', async function () {
           await cliRepl.start(await testServer.connectionString(), {});
-          const { logFilePath } = cliRepl.logWriter!;
+          const { logFilePath } = cliRepl.logWriter as MongoLogWriter;
           input.write('db.hello()\n');
           input.write('exit\n');
           await waitBus(cliRepl.bus, 'mongosh:closed');
-          const flushEntry = (await readReplLogfile(logFilePath)).find(
+          const flushEntry = (await readReplLogFile(logFilePath)).find(
             (entry: any) => entry.id === 1_000_000_045
           );
           expect(flushEntry.attr.flushError).to.equal(null);

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

@@ -30,7 +30,8 @@ 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 type { MongoshLoggingAndTelemetry } from '@mongosh/logging';
+import { setupLoggingAndTelemetry } from '@mongosh/logging';
 import {
   ToggleableAnalytics,
   ThrottledAnalytics,
@@ -254,36 +255,44 @@ export class CliRepl implements MongoshIOProvider {
       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),
-      });
-    }
+    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();
-    const { quiet } = CliRepl.getFileAndEvalInfo(this.cliOptions);
-    if (!quiet) {
-      this.output.write(`Current Mongosh Log ID:\t${logger.logId}\n`);
+
+    if (!this.logWriter) {
+      this.logWriter ??= await this.logManager.createLogWriter();
+
+      const { quiet } = CliRepl.getFileAndEvalInfo(this.cliOptions);
+      if (!quiet) {
+        this.output.write(`Current Mongosh Log ID:\t${this.logWriter.logId}\n`);
+      }
+
+      markTime(TimingCategories.Logging, 'instantiated log writer');
     }
 
-    this.logWriter = logger;
-    this.loggingAndTelemetry.attachLogger(logger);
+    this.loggingAndTelemetry.attachLogger(this.logWriter);
 
-    markTime(TimingCategories.Logging, 'instantiated log writer');
-    logger.info('MONGOSH', mongoLogId(1_000_000_000), 'log', 'Starting log', {
-      execPath: process.execPath,
-      envInfo: redactSensitiveData(this.getLoggedEnvironmentVariables()),
-      ...(await buildInfo()),
-    });
+    this.logWriter.info(
+      'MONGOSH',
+      mongoLogId(1_000_000_000),
+      'log',
+      'Starting log',
+      {
+        execPath: process.execPath,
+        envInfo: redactSensitiveData(this.getLoggedEnvironmentVariables()),
+        ...(await buildInfo()),
+      }
+    );
     markTime(TimingCategories.Logging, 'logged initial message');
   }
 
@@ -350,18 +359,17 @@ export class CliRepl implements MongoshIOProvider {
 
     markTime(TimingCategories.Telemetry, 'created analytics instance');
 
-    this.loggingAndTelemetry = new MongoshLoggingAndTelemetry(
-      this.bus,
-      this.toggleableAnalytics,
-      {
+    this.loggingAndTelemetry = setupLoggingAndTelemetry({
+      bus: this.bus,
+      analytics: this.toggleableAnalytics,
+      userTraits: {
         platform: process.platform,
         arch: process.arch,
         is_containerized: this.isContainerizedEnvironment,
         ...(await getOsInfo()),
       },
-      version
-    );
-    this.loggingAndTelemetry.setup();
+      mongoshVersion: version,
+    });
 
     markTime(TimingCategories.Telemetry, 'completed telemetry setup');
 
@@ -628,14 +636,9 @@ 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.
+      await this.startLogging();
     } else {
       this.loggingAndTelemetry?.detachLogger();
-      this.logWriter?.destroy();
-      this.logWriter = undefined;
     }
   }
 

packages/cli-repl/test/repl-helpers.ts

@@ -87,7 +87,7 @@ const fakeTTYProps: Partial<ReadStream & WriteStream> = {
   },
 };
 
-async function readReplLogfile(
+async function readReplLogFile(
   logPath?: string | null | undefined
 ): Promise<any[]> {
   if (!logPath) return [];
@@ -107,5 +107,5 @@ export {
   waitEval,
   waitCompletion,
   fakeTTYProps,
-  readReplLogfile,
+  readReplLogFile,
 };

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

@@ -5,7 +5,7 @@ import { startTestServer } from '../../../testing/integration-testing-hooks';
 import {
   useTmpdir,
   setTemporaryHomeDirectory,
-  readReplLogfile,
+  readReplLogFile,
   getCertPath,
   connectionStringWithLocalhost,
 } from './repl-helpers';
@@ -241,7 +241,7 @@ describe('e2e TLS', function () {
         expect(prompt.state).to.equal('prompt');
 
         const logPath = path.join(logBasePath, `${shell.logId}_log`);
-        const logContents = await readReplLogfile(logPath);
+        const logContents = await readReplLogFile(logPath);
         expect(
           logContents.find((line) => line.id === 1_000_000_049).attr
             .asyncFallbackError
@@ -278,7 +278,7 @@ describe('e2e TLS', function () {
         expect(prompt.state).to.equal('prompt');
 
         const logPath = path.join(logBasePath, `${shell.logId}_log`);
-        const logContents = await readReplLogfile(logPath);
+        const logContents = await readReplLogFile(logPath);
         expect(
           logContents.find((line) => line.id === 1_000_000_049).attr
             .asyncFallbackError
@@ -308,7 +308,7 @@ describe('e2e TLS', function () {
         expect(prompt.state).to.equal('exit');
 
         const logPath = path.join(logBasePath, `${shell.logId}_log`);
-        const logContents = await readReplLogfile(logPath);
+        const logContents = await readReplLogFile(logPath);
         expect(logContents.find((line) => line.id === 1_000_000_049)).to.exist;
       });
     }

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

@@ -14,7 +14,11 @@ import { promises as fs, createReadStream } from 'fs';
 import { promisify } from 'util';
 import path from 'path';
 import os from 'os';
-import { readReplLogfile, setTemporaryHomeDirectory } from './repl-helpers';
+import {
+  expectLogEntries,
+  readReplLogFile,
+  setTemporaryHomeDirectory,
+} from './repl-helpers';
 import { bson } from '@mongosh/service-provider-core';
 import type { Server as HTTPServer } from 'http';
 import { createServer as createHTTPServer } from 'http';
@@ -1405,7 +1409,7 @@ describe('e2e', function () {
           throw new Error('Shell does not have a logId associated with it');
         }
         const logPath = path.join(logBasePath, `${shell.logId}_log`);
-        return readReplLogfile(logPath);
+        return readReplLogFile(logPath);
       };
       startTestShell = async (...extraArgs: string[]) => {
         const shell = this.startTestShell({
@@ -1579,32 +1583,30 @@ describe('e2e', function () {
         it('does not write to the log 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);
+          expectLogEntries(log, 'print(123 + 456)', 1);
 
           await shell.executeLine(`config.set("disableLogging", true)`);
           expect(await shell.executeLine('print(579 - 123)')).to.include('456');
 
           const logAfterDisabling = await readLogFile();
+          expectLogEntries(logAfterDisabling, 'print(579 - 123)', 0);
+          expectLogEntries(logAfterDisabling, 'print(123 + 456)', 1);
+        });
+
+        it('starts writing to the same log from the point where disableLogging is set to false', async function () {
+          expect(await shell.executeLine('print(222 - 111)')).to.include('111');
+
+          let log = 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)'
+            log.filter(
+              (logEntry) => logEntry.attr?.input === 'print(222 - 111)'
             )
           ).to.have.lengthOf(1);
-        });
 
-        it('starts writing to a new log from the point where disableLogging is set to false', async function () {
           await shell.executeLine(`config.set("disableLogging", true)`);
           expect(await shell.executeLine('print(123 + 456)')).to.include('579');
-          const log = await readLogFile();
+
+          log = await readLogFile();
           const oldLogId = shell.logId;
           expect(oldLogId).not.null;
 
@@ -1624,15 +1626,21 @@ describe('e2e', function () {
 
           const newLogId = shell.logId;
           expect(newLogId).not.null;
-          expect(oldLogId).not.equal(newLogId);
-          const logsAfterEnabling = await readLogFile();
+          expect(oldLogId).equals(newLogId);
+          log = await readLogFile();
+
           expect(
-            logsAfterEnabling.filter(
+            log.filter(
+              (logEntry) => logEntry.attr?.input === 'print(222 - 111)'
+            )
+          ).to.have.lengthOf(1);
+          expect(
+            log.filter(
               (logEntry) => logEntry.attr?.input === 'print(579 - 123)'
             )
           ).to.have.lengthOf(1);
           expect(
-            logsAfterEnabling.filter(
+            log.filter(
               (logEntry) => logEntry.attr?.input === 'print(123 + 456)'
             )
           ).to.have.lengthOf(0);

packages/e2e-tests/test/repl-helpers.ts

@@ -2,10 +2,11 @@ import { promises as fs } from 'fs';
 import { promisify } from 'util';
 import path from 'path';
 import rimraf from 'rimraf';
-import chai from 'chai';
+import chai, { expect } from 'chai';
 import sinonChai from 'sinon-chai';
 import chaiAsPromised from 'chai-as-promised';
 import type { MongodSetup } from '../../../testing/integration-testing-hooks';
+import type { LogEntry } from '@mongosh/shell-api/src/log-entry';
 
 chai.use(sinonChai);
 chai.use(chaiAsPromised);
@@ -47,13 +48,24 @@ function useTmpdir(): { readonly path: string } {
   };
 }
 
-async function readReplLogfile(logPath: string) {
+async function readReplLogFile(logPath: string) {
   return (await fs.readFile(logPath, 'utf8'))
     .split('\n')
     .filter((line) => line.trim())
     .map((line) => JSON.parse(line));
 }
 
+// eslint-disable-next-line mocha/no-exports
+export function expectLogEntries(
+  log: LogEntry[],
+  input: string,
+  length: number
+) {
+  expect(
+    log.filter((logEntry) => logEntry.attr?.input === input)
+  ).to.have.lengthOf(length);
+}
+
 const fakeExternalEditor = async ({
   output,
   expectedExtension,
@@ -161,7 +173,7 @@ async function connectionStringWithLocalhost(
 // eslint-disable-next-line mocha/no-exports
 export {
   useTmpdir,
-  readReplLogfile,
+  readReplLogFile,
   fakeExternalEditor,
   setTemporaryHomeDirectory,
   getCertPath,

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

@@ -362,15 +362,12 @@ export class TestShell {
   }
 
   get logId(): string | null {
-    const matches = this._output.match(
-      /^Current Mongosh Log ID:\s*([a-z0-9]{24})$/gm
+    const match = /^Current Mongosh Log ID:\s*(?<logId>[a-z0-9]{24})$/m.exec(
+      this._output
     );
-    if (!matches || matches.length === 0) {
+    if (!match) {
       return null;
     }
-    const lastMatch = /^Current Mongosh Log ID:\s*([a-z0-9]{24})$/.exec(
-      matches[matches.length - 1]
-    );
-    return lastMatch ? lastMatch[1] : null;
+    return match.groups!.logId;
   }
 }

packages/logging/src/index.ts

@@ -1,8 +1,9 @@
-export { MongoshLoggingAndTelemetry } from './logging-and-telemetry';
 export {
   MongoshAnalytics,
   ToggleableAnalytics,
   SampledAnalytics,
   NoopAnalytics,
   ThrottledAnalytics,
 } from './analytics-helpers';
+export { MongoshLoggingAndTelemetry } from './types';
+export { setupLoggingAndTelemetry } from './logging-and-telemetry';