Skip to content

Commit 1bbbfb0

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 1bbbfb0

File tree

8 files changed

+444
-433
lines changed

8 files changed

+444
-433
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1559,7 +1559,7 @@ 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');

‎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/e2e-tests/test/e2e.spec.ts‎

Lines changed: 28 additions & 20 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';
@@ -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/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';

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

Lines changed: 8 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import { EventEmitter } from 'events';
55
import { MongoshInvalidInputError } from '@mongosh/errors';
66
import type { MongoshBus } from '@mongosh/types';
77
import type { Writable } from 'stream';
8-
import { MongoshLoggingAndTelemetry } from './logging-and-telemetry';
8+
import type { MongoshLoggingAndTelemetry } from '.';
9+
import { setupLoggingAndTelemetry } from '.';
910

1011
describe('MongoshLoggingAndTelemetry', function () {
1112
let logOutput: any[];
@@ -36,15 +37,15 @@ describe('MongoshLoggingAndTelemetry', function () {
3637
analyticsOutput = [];
3738
bus = new EventEmitter();
3839

39-
loggingAndTelemetry = new MongoshLoggingAndTelemetry(
40+
loggingAndTelemetry = setupLoggingAndTelemetry({
4041
bus,
4142
analytics,
42-
{
43+
userTraits: {
4344
platform: process.platform,
4445
arch: process.arch,
4546
},
46-
'1.0.0'
47-
);
47+
mongoshVersion: '1.0.0',
48+
});
4849

4950
logger = new MongoLogWriter(logId, `/tmp/${logId}_log`, {
5051
write(chunk: string, cb: () => void) {
@@ -62,37 +63,20 @@ describe('MongoshLoggingAndTelemetry', function () {
6263
logger.destroy();
6364
});
6465

65-
it('throws when running setup twice', function () {
66-
loggingAndTelemetry.setup();
67-
68-
expect(() => loggingAndTelemetry.setup()).throws(
69-
'Setup can only be called once.'
70-
);
71-
});
72-
73-
it('throws when trying to setup writer prematurely', function () {
74-
expect(() => loggingAndTelemetry.attachLogger(logger)).throws(
75-
'Run setup() before setting up the log writer.'
76-
);
77-
});
78-
7966
it('throws when running attachLogger twice without detaching', function () {
80-
loggingAndTelemetry.setup();
8167
loggingAndTelemetry.attachLogger(logger);
8268
expect(() => loggingAndTelemetry.attachLogger(logger)).throws(
8369
'Previously set logger has not been detached. Run detachLogger() before setting.'
8470
);
8571
});
8672

8773
it('does not throw when attaching and detaching loggers', function () {
88-
loggingAndTelemetry.setup();
8974
loggingAndTelemetry.attachLogger(logger);
9075
loggingAndTelemetry.detachLogger();
9176
expect(() => loggingAndTelemetry.attachLogger(logger)).does.not.throw();
9277
});
9378

9479
it('tracks new local connection events', function () {
95-
loggingAndTelemetry.setup();
9680
loggingAndTelemetry.attachLogger(logger);
9781

9882
expect(logOutput).to.have.lengthOf(0);
@@ -147,7 +131,6 @@ describe('MongoshLoggingAndTelemetry', function () {
147131
});
148132

149133
it('tracks new atlas connection events', function () {
150-
loggingAndTelemetry.setup();
151134
loggingAndTelemetry.attachLogger(logger);
152135

153136
expect(logOutput).to.have.lengthOf(0);
@@ -206,7 +189,6 @@ describe('MongoshLoggingAndTelemetry', function () {
206189
});
207190

208191
it('detaching logger leads to no logging but persists analytics', function () {
209-
loggingAndTelemetry.setup();
210192
loggingAndTelemetry.attachLogger(logger);
211193

212194
expect(logOutput).to.have.lengthOf(0);
@@ -222,7 +204,6 @@ describe('MongoshLoggingAndTelemetry', function () {
222204
});
223205

224206
it('detaching logger applies to devtools-connect events', function () {
225-
loggingAndTelemetry.setup();
226207
loggingAndTelemetry.attachLogger(logger);
227208

228209
bus.emit('devtools-connect:connect-fail-early');
@@ -241,11 +222,11 @@ describe('MongoshLoggingAndTelemetry', function () {
241222
loggingAndTelemetry.attachLogger(logger);
242223

243224
bus.emit('devtools-connect:connect-fail-early');
244-
expect(logOutput).to.have.lengthOf(3);
225+
bus.emit('devtools-connect:connect-fail-early');
226+
expect(logOutput).to.have.lengthOf(4);
245227
});
246228

247229
it('detaching logger mid-way leads to no logging but persists analytics', function () {
248-
loggingAndTelemetry.setup();
249230
loggingAndTelemetry.attachLogger(logger);
250231

251232
expect(logOutput).to.have.lengthOf(0);
@@ -266,7 +247,6 @@ describe('MongoshLoggingAndTelemetry', function () {
266247
});
267248

268249
it('detaching logger is recoverable', function () {
269-
loggingAndTelemetry.setup();
270250
loggingAndTelemetry.attachLogger(logger);
271251

272252
expect(logOutput).to.have.lengthOf(0);
@@ -294,7 +274,6 @@ describe('MongoshLoggingAndTelemetry', function () {
294274
});
295275

296276
it('tracks a sequence of events', function () {
297-
loggingAndTelemetry.setup();
298277
loggingAndTelemetry.attachLogger(logger);
299278

300279
expect(logOutput).to.have.lengthOf(0);
@@ -753,7 +732,6 @@ describe('MongoshLoggingAndTelemetry', function () {
753732
});
754733

755734
it('buffers deprecated API calls', function () {
756-
loggingAndTelemetry.setup();
757735
loggingAndTelemetry.attachLogger(logger);
758736

759737
expect(logOutput).to.have.lengthOf(0);
@@ -929,7 +907,6 @@ describe('MongoshLoggingAndTelemetry', function () {
929907
});
930908

931909
it('does not track database calls outside of evaluate-{started,finished}', function () {
932-
loggingAndTelemetry.setup();
933910
loggingAndTelemetry.attachLogger(logger);
934911

935912
expect(logOutput).to.have.lengthOf(0);
@@ -957,7 +934,6 @@ describe('MongoshLoggingAndTelemetry', function () {
957934
expect(logOutput).to.have.lengthOf(0);
958935
expect(analyticsOutput).to.be.empty;
959936

960-
loggingAndTelemetry.setup();
961937
loggingAndTelemetry.attachLogger(logger);
962938

963939
bus.emit('mongosh:connect', {

0 commit comments

Comments
 (0)