Skip to content

Commit df96f8d

Browse files
gagikgagik
committed
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.
1 parent 6f30993 commit df96f8d

File tree

11 files changed

+469
-446
lines changed

11 files changed

+469
-446
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
import {
1919
expect,
2020
fakeTTYProps,
21-
readReplLogfile,
21+
readReplLogFile,
2222
tick,
2323
useTmpdir,
2424
waitBus,
@@ -56,7 +56,7 @@ describe('CliRepl', function () {
5656
async function log(): Promise<any[]> {
5757
if (!cliRepl.logWriter?.logFilePath) return [];
5858
await cliRepl.logWriter.flush(); // Ensure any pending data is written first
59-
return readReplLogfile(cliRepl.logWriter.logFilePath);
59+
return readReplLogFile(cliRepl.logWriter.logFilePath);
6060
}
6161

6262
async function startWithExpectedImmediateExit(
@@ -1559,11 +1559,11 @@ describe('CliRepl', function () {
15591559

15601560
it('includes a statement about flushed telemetry in the log', async function () {
15611561
await cliRepl.start(await testServer.connectionString(), {});
1562-
const { logFilePath } = cliRepl.logWriter!;
1562+
const { logFilePath } = cliRepl.logWriter as MongoLogWriter;
15631563
input.write('db.hello()\n');
15641564
input.write('exit\n');
15651565
await waitBus(cliRepl.bus, 'mongosh:closed');
1566-
const flushEntry = (await readReplLogfile(logFilePath)).find(
1566+
const flushEntry = (await readReplLogFile(logFilePath)).find(
15671567
(entry: any) => entry.id === 1_000_000_045
15681568
);
15691569
expect(flushEntry.attr.flushError).to.equal(null);

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

Lines changed: 41 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ import type { MongoLogWriter } from 'mongodb-log-writer';
3030
import { MongoLogManager, mongoLogId } from 'mongodb-log-writer';
3131
import type { MongoshNodeReplOptions, MongoshIOProvider } from './mongosh-repl';
3232
import MongoshNodeRepl from './mongosh-repl';
33-
import { MongoshLoggingAndTelemetry } from '@mongosh/logging';
33+
import type { MongoshLoggingAndTelemetry } from '@mongosh/logging';
34+
import { setupLoggingAndTelemetry } from '@mongosh/logging';
3435
import {
3536
ToggleableAnalytics,
3637
ThrottledAnalytics,
@@ -254,36 +255,44 @@ export class CliRepl implements MongoshIOProvider {
254255
throw new Error('Logging and telemetry not setup');
255256
}
256257

257-
if (!this.logManager) {
258-
this.logManager = new MongoLogManager({
259-
directory: this.shellHomeDirectory.localPath('.'),
260-
retentionDays: 30,
261-
maxLogFileCount: +(
262-
process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100
263-
),
264-
onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
265-
onwarn: (err: Error, path: string) =>
266-
this.warnAboutInaccessibleFile(err, path),
267-
});
268-
}
258+
this.logManager ??= new MongoLogManager({
259+
directory: this.shellHomeDirectory.localPath('.'),
260+
retentionDays: 30,
261+
maxLogFileCount: +(
262+
process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100
263+
),
264+
onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'),
265+
onwarn: (err: Error, path: string) =>
266+
this.warnAboutInaccessibleFile(err, path),
267+
});
269268

270269
await this.logManager.cleanupOldLogFiles();
271270
markTime(TimingCategories.Logging, 'cleaned up log files');
272-
const logger = await this.logManager.createLogWriter();
273-
const { quiet } = CliRepl.getFileAndEvalInfo(this.cliOptions);
274-
if (!quiet) {
275-
this.output.write(`Current Mongosh Log ID:\t${logger.logId}\n`);
271+
272+
if (!this.logWriter) {
273+
this.logWriter ??= await this.logManager.createLogWriter();
274+
275+
const { quiet } = CliRepl.getFileAndEvalInfo(this.cliOptions);
276+
if (!quiet) {
277+
this.output.write(`Current Mongosh Log ID:\t${this.logWriter.logId}\n`);
278+
}
279+
280+
markTime(TimingCategories.Logging, 'instantiated log writer');
276281
}
277282

278-
this.logWriter = logger;
279-
this.loggingAndTelemetry.attachLogger(logger);
283+
this.loggingAndTelemetry.attachLogger(this.logWriter);
280284

281-
markTime(TimingCategories.Logging, 'instantiated log writer');
282-
logger.info('MONGOSH', mongoLogId(1_000_000_000), 'log', 'Starting log', {
283-
execPath: process.execPath,
284-
envInfo: redactSensitiveData(this.getLoggedEnvironmentVariables()),
285-
...(await buildInfo()),
286-
});
285+
this.logWriter.info(
286+
'MONGOSH',
287+
mongoLogId(1_000_000_000),
288+
'log',
289+
'Starting log',
290+
{
291+
execPath: process.execPath,
292+
envInfo: redactSensitiveData(this.getLoggedEnvironmentVariables()),
293+
...(await buildInfo()),
294+
}
295+
);
287296
markTime(TimingCategories.Logging, 'logged initial message');
288297
}
289298

@@ -350,18 +359,17 @@ export class CliRepl implements MongoshIOProvider {
350359

351360
markTime(TimingCategories.Telemetry, 'created analytics instance');
352361

353-
this.loggingAndTelemetry = new MongoshLoggingAndTelemetry(
354-
this.bus,
355-
this.toggleableAnalytics,
356-
{
362+
this.loggingAndTelemetry = setupLoggingAndTelemetry({
363+
bus: this.bus,
364+
analytics: this.toggleableAnalytics,
365+
userTraits: {
357366
platform: process.platform,
358367
arch: process.arch,
359368
is_containerized: this.isContainerizedEnvironment,
360369
...(await getOsInfo()),
361370
},
362-
version
363-
);
364-
this.loggingAndTelemetry.setup();
371+
mongoshVersion: version,
372+
});
365373

366374
markTime(TimingCategories.Telemetry, 'completed telemetry setup');
367375

@@ -628,14 +636,9 @@ export class CliRepl implements MongoshIOProvider {
628636

629637
async setLoggingEnabled(enabled: boolean): Promise<void> {
630638
if (enabled) {
631-
if (!this.logWriter) {
632-
await this.startLogging();
633-
}
634-
// Do nothing if there is already an active log writer.
639+
await this.startLogging();
635640
} else {
636641
this.loggingAndTelemetry?.detachLogger();
637-
this.logWriter?.destroy();
638-
this.logWriter = undefined;
639642
}
640643
}
641644

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ const fakeTTYProps: Partial<ReadStream & WriteStream> = {
8787
},
8888
};
8989

90-
async function readReplLogfile(
90+
async function readReplLogFile(
9191
logPath?: string | null | undefined
9292
): Promise<any[]> {
9393
if (!logPath) return [];
@@ -107,5 +107,5 @@ export {
107107
waitEval,
108108
waitCompletion,
109109
fakeTTYProps,
110-
readReplLogfile,
110+
readReplLogFile,
111111
};

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { startTestServer } from '../../../testing/integration-testing-hooks';
55
import {
66
useTmpdir,
77
setTemporaryHomeDirectory,
8-
readReplLogfile,
8+
readReplLogFile,
99
getCertPath,
1010
connectionStringWithLocalhost,
1111
} from './repl-helpers';
@@ -241,7 +241,7 @@ describe('e2e TLS', function () {
241241
expect(prompt.state).to.equal('prompt');
242242

243243
const logPath = path.join(logBasePath, `${shell.logId}_log`);
244-
const logContents = await readReplLogfile(logPath);
244+
const logContents = await readReplLogFile(logPath);
245245
expect(
246246
logContents.find((line) => line.id === 1_000_000_049).attr
247247
.asyncFallbackError
@@ -278,7 +278,7 @@ describe('e2e TLS', function () {
278278
expect(prompt.state).to.equal('prompt');
279279

280280
const logPath = path.join(logBasePath, `${shell.logId}_log`);
281-
const logContents = await readReplLogfile(logPath);
281+
const logContents = await readReplLogFile(logPath);
282282
expect(
283283
logContents.find((line) => line.id === 1_000_000_049).attr
284284
.asyncFallbackError
@@ -308,7 +308,7 @@ describe('e2e TLS', function () {
308308
expect(prompt.state).to.equal('exit');
309309

310310
const logPath = path.join(logBasePath, `${shell.logId}_log`);
311-
const logContents = await readReplLogfile(logPath);
311+
const logContents = await readReplLogFile(logPath);
312312
expect(logContents.find((line) => line.id === 1_000_000_049)).to.exist;
313313
});
314314
}

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

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ import { promises as fs, createReadStream } from 'fs';
1414
import { promisify } from 'util';
1515
import path from 'path';
1616
import os from 'os';
17-
import { readReplLogfile, setTemporaryHomeDirectory } from './repl-helpers';
17+
import {
18+
expectLogEntries,
19+
readReplLogFile,
20+
setTemporaryHomeDirectory,
21+
} from './repl-helpers';
1822
import { bson } from '@mongosh/service-provider-core';
1923
import type { Server as HTTPServer } from 'http';
2024
import { createServer as createHTTPServer } from 'http';
@@ -1405,7 +1409,7 @@ describe('e2e', function () {
14051409
throw new Error('Shell does not have a logId associated with it');
14061410
}
14071411
const logPath = path.join(logBasePath, `${shell.logId}_log`);
1408-
return readReplLogfile(logPath);
1412+
return readReplLogFile(logPath);
14091413
};
14101414
startTestShell = async (...extraArgs: string[]) => {
14111415
const shell = this.startTestShell({
@@ -1579,32 +1583,30 @@ describe('e2e', function () {
15791583
it('does not write to the log after disableLogging is set to true', async function () {
15801584
expect(await shell.executeLine('print(123 + 456)')).to.include('579');
15811585
const log = await readLogFile();
1582-
expect(
1583-
log.filter(
1584-
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
1585-
)
1586-
).to.have.lengthOf(1);
1586+
expectLogEntries(log, 'print(123 + 456)', 1);
15871587

15881588
await shell.executeLine(`config.set("disableLogging", true)`);
15891589
expect(await shell.executeLine('print(579 - 123)')).to.include('456');
15901590

15911591
const logAfterDisabling = await readLogFile();
1592+
expectLogEntries(logAfterDisabling, 'print(579 - 123)', 0);
1593+
expectLogEntries(logAfterDisabling, 'print(123 + 456)', 1);
1594+
});
1595+
1596+
it('starts writing to the same log from the point where disableLogging is set to false', async function () {
1597+
expect(await shell.executeLine('print(222 - 111)')).to.include('111');
1598+
1599+
let log = await readLogFile();
15921600
expect(
1593-
logAfterDisabling.filter(
1594-
(logEntry) => logEntry.attr?.input === 'print(579 - 123)'
1595-
)
1596-
).to.have.lengthOf(0);
1597-
expect(
1598-
logAfterDisabling.filter(
1599-
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
1601+
log.filter(
1602+
(logEntry) => logEntry.attr?.input === 'print(222 - 111)'
16001603
)
16011604
).to.have.lengthOf(1);
1602-
});
16031605

1604-
it('starts writing to a new log from the point where disableLogging is set to false', async function () {
16051606
await shell.executeLine(`config.set("disableLogging", true)`);
16061607
expect(await shell.executeLine('print(123 + 456)')).to.include('579');
1607-
const log = await readLogFile();
1608+
1609+
log = await readLogFile();
16081610
const oldLogId = shell.logId;
16091611
expect(oldLogId).not.null;
16101612

@@ -1624,15 +1626,21 @@ describe('e2e', function () {
16241626

16251627
const newLogId = shell.logId;
16261628
expect(newLogId).not.null;
1627-
expect(oldLogId).not.equal(newLogId);
1628-
const logsAfterEnabling = await readLogFile();
1629+
expect(oldLogId).equals(newLogId);
1630+
log = await readLogFile();
1631+
16291632
expect(
1630-
logsAfterEnabling.filter(
1633+
log.filter(
1634+
(logEntry) => logEntry.attr?.input === 'print(222 - 111)'
1635+
)
1636+
).to.have.lengthOf(1);
1637+
expect(
1638+
log.filter(
16311639
(logEntry) => logEntry.attr?.input === 'print(579 - 123)'
16321640
)
16331641
).to.have.lengthOf(1);
16341642
expect(
1635-
logsAfterEnabling.filter(
1643+
log.filter(
16361644
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
16371645
)
16381646
).to.have.lengthOf(0);

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@ import { promises as fs } from 'fs';
22
import { promisify } from 'util';
33
import path from 'path';
44
import rimraf from 'rimraf';
5-
import chai from 'chai';
5+
import chai, { expect } from 'chai';
66
import sinonChai from 'sinon-chai';
77
import chaiAsPromised from 'chai-as-promised';
88
import type { MongodSetup } from '../../../testing/integration-testing-hooks';
9+
import type { LogEntry } from '@mongosh/shell-api/src/log-entry';
910

1011
chai.use(sinonChai);
1112
chai.use(chaiAsPromised);
@@ -47,13 +48,24 @@ function useTmpdir(): { readonly path: string } {
4748
};
4849
}
4950

50-
async function readReplLogfile(logPath: string) {
51+
async function readReplLogFile(logPath: string) {
5152
return (await fs.readFile(logPath, 'utf8'))
5253
.split('\n')
5354
.filter((line) => line.trim())
5455
.map((line) => JSON.parse(line));
5556
}
5657

58+
// eslint-disable-next-line mocha/no-exports
59+
export function expectLogEntries(
60+
log: LogEntry[],
61+
input: string,
62+
length: number
63+
) {
64+
expect(
65+
log.filter((logEntry) => logEntry.attr?.input === input)
66+
).to.have.lengthOf(length);
67+
}
68+
5769
const fakeExternalEditor = async ({
5870
output,
5971
expectedExtension,
@@ -161,7 +173,7 @@ async function connectionStringWithLocalhost(
161173
// eslint-disable-next-line mocha/no-exports
162174
export {
163175
useTmpdir,
164-
readReplLogfile,
176+
readReplLogFile,
165177
fakeExternalEditor,
166178
setTemporaryHomeDirectory,
167179
getCertPath,

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -362,15 +362,12 @@ export class TestShell {
362362
}
363363

364364
get logId(): string | null {
365-
const matches = this._output.match(
366-
/^Current Mongosh Log ID:\s*([a-z0-9]{24})$/gm
365+
const match = /^Current Mongosh Log ID:\s*(?<logId>[a-z0-9]{24})$/m.exec(
366+
this._output
367367
);
368-
if (!matches || matches.length === 0) {
368+
if (!match) {
369369
return null;
370370
}
371-
const lastMatch = /^Current Mongosh Log ID:\s*([a-z0-9]{24})$/.exec(
372-
matches[matches.length - 1]
373-
);
374-
return lastMatch ? lastMatch[1] : null;
371+
return match.groups!.logId;
375372
}
376373
}

‎packages/logging/src/index.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
export { MongoshLoggingAndTelemetry } from './logging-and-telemetry';
21
export {
32
MongoshAnalytics,
43
ToggleableAnalytics,
54
SampledAnalytics,
65
NoopAnalytics,
76
ThrottledAnalytics,
87
} from './analytics-helpers';
8+
export { MongoshLoggingAndTelemetry } from './types';
9+
export { setupLoggingAndTelemetry } from './logging-and-telemetry';

0 commit comments

Comments
 (0)