Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(logging): add option to disable logging MONGOSH-1988 #2325

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Prev Previous commit
Next Next commit
tests: add e2e tests for global config and midway disabling and enabl…
…ing of the logs
  • Loading branch information
gagik committed Jan 30, 2025
commit 214ca17ca9b882cbd2bbfc2bb4c4f47731337e76
15 changes: 14 additions & 1 deletion packages/cli-repl/src/cli-repl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1385,7 +1385,7 @@ describe('CliRepl', function () {
});

context('logging configuration', function () {
it('logging is enabled by default', async function () {
it('logging is enabled by default and event is called', async function () {
const onLogInitialized = sinon.stub();
cliRepl.bus.on('mongosh:log-initialized', onLogInitialized);

Expand All @@ -1396,6 +1396,19 @@ describe('CliRepl', function () {
expect(onLogInitialized).calledOnce;
expect(cliRepl.logWriter).is.instanceOf(MongoLogWriter);
});

it('does not initialize logging when it is disabled', async function () {
cliRepl.config.disableLogging = true;
const onLogInitialized = sinon.stub();
cliRepl.bus.on('mongosh:log-initialized', onLogInitialized);

await cliRepl.start(await testServer.connectionString(), {});

expect(await cliRepl.getConfig('disableLogging')).is.true;
expect(onLogInitialized).not.called;

expect(cliRepl.logWriter).is.undefined;
});
});

it('times out fast', async function () {
Expand Down
82 changes: 74 additions & 8 deletions packages/e2e-tests/test/e2e.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1363,7 +1363,7 @@ describe('e2e', function () {
let logBasePath: string;
let historyPath: string;
let readConfig: () => Promise<any>;
let readLogfile: () => Promise<LogEntry[]>;
let readLogFile: () => Promise<LogEntry[]>;
let startTestShell: (...extraArgs: string[]) => Promise<TestShell>;

beforeEach(function () {
Expand Down Expand Up @@ -1400,7 +1400,7 @@ describe('e2e', function () {
}
readConfig = async () =>
EJSON.parse(await fs.readFile(configPath, 'utf8'));
readLogfile = async () => {
readLogFile = async () => {
if (!shell.logId) {
throw new Error('Shell does not have a logId associated with it');
}
Expand Down Expand Up @@ -1515,7 +1515,7 @@ describe('e2e', function () {
});

describe('log file', function () {
it('does not create a log if global config has disableLogging', async function () {
it('does not get created 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({
Expand All @@ -1536,9 +1536,38 @@ describe('e2e', function () {
expect(shell.logId).equals(null);
});

it('gets created if global config has disableLogging set to false', async function () {
const globalConfig = path.join(homedir, 'globalconfig.conf');
await fs.writeFile(globalConfig, 'mongosh:\n disableLogging: false');
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('false');
shell.assertNoErrors();

expect(await shell.executeLine('print(123 + 456)')).to.include('579');
expect(shell.logId).not.equal(null);

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('creates a log file that keeps track of session events', async function () {
expect(await shell.executeLine('print(123 + 456)')).to.include('579');
const log = await readLogfile();
const log = await readLogFile();
expect(
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
log.filter(
Expand All @@ -1547,9 +1576,9 @@ describe('e2e', function () {
).to.have.lengthOf(1);
});

it('does not write to the log file after disableLogging is set to true', async 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();
const log = await readLogFile();
expect(
log.filter(
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
Expand All @@ -1559,7 +1588,7 @@ describe('e2e', function () {
await shell.executeLine(`config.set("disableLogging", true)`);
expect(await shell.executeLine('print(579 - 123)')).to.include('456');

const logAfterDisabling = await readLogfile();
const logAfterDisabling = await readLogFile();
expect(
logAfterDisabling.filter(
(logEntry) => logEntry.attr?.input === 'print(579 - 123)'
Expand All @@ -1572,6 +1601,43 @@ describe('e2e', function () {
).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();
const oldLogId = shell.logId;
expect(oldLogId).not.null;

expect(
log.filter(
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
)
).to.have.lengthOf(0);

await shell.executeLine(`config.set("disableLogging", false)`);

expect(
await shell.executeLine('config.get("disableLogging")')
).to.include('false');

expect(await shell.executeLine('print(579 - 123)')).to.include('456');

const newLogId = shell.logId;
expect(newLogId).not.null;
expect(oldLogId).not.equal(newLogId);
const logsAfterEnabling = await readLogFile();
expect(
logsAfterEnabling.filter(
(logEntry) => logEntry.attr?.input === 'print(579 - 123)'
)
).to.have.lengthOf(1);
expect(
logsAfterEnabling.filter(
(logEntry) => logEntry.attr?.input === 'print(123 + 456)'
)
).to.have.lengthOf(0);
});

it('includes information about the driver version', async function () {
const connectionString = await testServer.connectionString();
expect(
Expand All @@ -1580,7 +1646,7 @@ describe('e2e', function () {
)
).to.include('test');
await eventually(async () => {
const log = await readLogfile();
const log = await readLogFile();
expect(
log.filter(
(logEntry) => typeof logEntry.attr?.driver?.version === 'string'
Expand Down
12 changes: 8 additions & 4 deletions packages/e2e-tests/test/test-shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,16 @@ export class TestShell {
}

get logId(): string | null {
const match = /^Current Mongosh Log ID:\s*(?<logId>[a-z0-9]{24})$/m.exec(
this._output
const matches = this._output.match(
/^Current Mongosh Log ID:\s*([a-z0-9]{24})$/gm
);
if (!match) {
if (!matches || matches.length === 0) {
return null;
}
return match.groups!.logId;
const lastMatch = matches[matches.length - 1];
const logIdMatch = /^Current Mongosh Log ID:\s*([a-z0-9]{24})$/.exec(
lastMatch
);
return logIdMatch ? logIdMatch[1] : null;
}
}
15 changes: 10 additions & 5 deletions packages/logging/src/logging-and-telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,17 +118,22 @@ export class MongoshLoggingAndTelemetry {
redactURICredentials(uri)
);

for (const pendingEvent of this.pendingLogEvents) {
pendingEvent();
}
this.pendingLogEvents = [];
this.runAndCleanPendingEvents();

this.bus.emit('mongosh:log-initialized');
}

public detachLogger() {
this.pendingLogEvents = [];
this.log = null;
gagik marked this conversation as resolved.
Show resolved Hide resolved
// Still run any remaining pending events for telemetry purposes.
this.runAndCleanPendingEvents();
}

private runAndCleanPendingEvents() {
for (const pendingEvent of this.pendingLogEvents) {
pendingEvent();
}
this.pendingLogEvents = [];
gagik marked this conversation as resolved.
Show resolved Hide resolved
}

private _getTelemetryUserIdentity(): MongoshAnalyticsIdentity {
Expand Down
Loading