Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions packages/cli-repl/src/cli-repl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,18 @@ export class CliRepl implements MongoshIOProvider {
async start(
driverUri: string,
driverOptions: DevtoolsConnectOptions
): Promise<void> {
try {
return await this._start(driverUri, driverOptions);
} catch (err) {
await this.close();
throw err;
}
}

private async _start(
driverUri: string,
driverOptions: DevtoolsConnectOptions
): Promise<void> {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const { version }: { version: string } = require('../package.json');
Expand Down Expand Up @@ -659,7 +671,7 @@ export class CliRepl implements MongoshIOProvider {
if (enabled) {
await this.startLogging();
} else {
this.loggingAndTelemetry?.detachLogger();
await this.loggingAndTelemetry?.detachLogger();
}
}

Expand Down Expand Up @@ -1172,7 +1184,7 @@ export class CliRepl implements MongoshIOProvider {
const analytics = this.toggleableAnalytics;
let flushError: string | null = null;
let flushDuration: number | null = null;
this.loggingAndTelemetry?.flush();
await this.loggingAndTelemetry?.flush();

if (analytics) {
const flushStart = Date.now();
Expand Down
47 changes: 41 additions & 6 deletions packages/e2e-tests/test/e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2142,12 +2142,7 @@ describe('e2e', function () {
await shell.executeLine("log.info('This is a custom entry')");
expect(shell.assertNoErrors());
await eventually(async () => {
const log = await readLogFile<
MongoLogEntryFromFile & {
c: string;
ctx: string;
}
>();
const log = await readLogFile<MongoLogEntryFromFile>();
const customLogEntry = log.filter((logEntry) =>
logEntry.msg.includes('This is a custom entry')
);
Expand Down Expand Up @@ -2182,6 +2177,46 @@ describe('e2e', function () {
).to.have.lengthOf(1);
});
});

it('flushes log file (normal exit)', async function () {
const shell = this.startTestShell({
args: [
'--nodb',
'--eval',
'for(let i=0; i < 10; i++) log.info("logging", {i}); log.getPath();',
],
});
expect(await shell.waitForAnyExit()).to.equal(0);
const log = await readReplLogFile(shell.output.trim());
for (let i = 0; i < 10; i++) {
expect(
log.some(
(entry) => entry.msg === 'logging' && entry.attr?.i === i
)
).to.be.true;
}
});

it('flushes log file (exception thrown)', async function () {
const shell = this.startTestShell({
args: [
'--nodb',
'--eval',
'for(let i=0; i < 10; i++) log.info("logging", {i}); print(log.getPath()); throw new Error("uh oh")',
],
});
expect(await shell.waitForAnyExit()).to.equal(1);
const log = await readReplLogFile(
shell.output.replace(/Error: uh oh/g, '').trim()
);
Comment on lines +2209 to +2211
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message filtering using regex replacement is fragile and could inadvertently remove legitimate log content. Consider a more robust approach to extract the log path from shell output.

Suggested change
const log = await readReplLogFile(
shell.output.replace(/Error: uh oh/g, '').trim()
);
// Extract the log path from the shell output by finding the line that looks like a log file path
const logPathLine = shell.output
.split('\n')
.map(line => line.trim())
.find(line => line.length > 0 && /\.log$/.test(line));
expect(logPathLine, 'Could not find log path in shell output').to.be.a('string');
const log = await readReplLogFile(logPathLine);

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I had similar thoughts here as well, although I was ultimately feeling like this is just fine for a test

for (let i = 0; i < 10; i++) {
expect(
log.some(
(entry) => entry.msg === 'logging' && entry.attr?.i === i
)
).to.be.true;
}
});
});

describe('history file', function () {
Expand Down
8 changes: 5 additions & 3 deletions packages/logging/src/logging-and-telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,15 @@ export class LoggingAndTelemetry implements MongoshLoggingAndTelemetry {
this.isSetup = true;
}

public flush(): void {
public async flush(): Promise<void> {
// Run any other pending events with the set or dummy log for telemetry purposes.
this.runAndClearPendingBusEvents();

// Abort setup, which will cause the device ID to be set to 'unknown'
// and run any remaining telemetry events
this.telemetrySetupAbort.abort();

await this.log.flush();
}

private async setupTelemetry(): Promise<void> {
Expand Down Expand Up @@ -177,10 +179,10 @@ export class LoggingAndTelemetry implements MongoshLoggingAndTelemetry {
this.bus.emit('mongosh:log-initialized');
}

public detachLogger() {
public async detachLogger(): Promise<void> {
this.log = LoggingAndTelemetry.dummyLogger;

this.flush();
await this.flush();
}

private runAndClearPendingBusEvents() {
Expand Down
4 changes: 2 additions & 2 deletions packages/logging/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import type { MultiSet } from './helpers';

export interface MongoshLoggingAndTelemetry {
attachLogger(logger: MongoLogWriter): void;
detachLogger(): void;
detachLogger(): Promise<void>;
/** Flush any remaining log or telemetry events. */
flush(): void;
flush(): Promise<void>;
}

export type MongoshLoggingAndTelemetryArguments = {
Expand Down