From 87318c0b5525e776e25525cf5b46f27e778e53bb Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 11 Feb 2025 14:34:22 +0100 Subject: [PATCH 1/4] feat(cli-repl): add configuration to set max log files size MONGOSH-1985 --- package-lock.json | 44 +++++++++++++++++++++-- packages/cli-repl/package.json | 2 +- packages/cli-repl/src/cli-repl.spec.ts | 14 ++++++++ packages/cli-repl/src/cli-repl.ts | 1 + packages/e2e-tests/package.json | 2 +- packages/e2e-tests/test/e2e.spec.ts | 50 ++++++++++++++++++++++++++ packages/logging/package.json | 2 +- packages/types/src/index.spec.ts | 6 ++++ packages/types/src/index.ts | 8 ++++- 9 files changed, 122 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index d2800bb01d..0df8f6fa5b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21445,6 +21445,7 @@ "resolved": "https://registry.npmjs.org/mongodb-log-writer/-/mongodb-log-writer-2.1.0.tgz", "integrity": "sha512-ZlNts/L9fs6gQNRuqLcB0+yjjfeyapbxjdkDpeb2bEYOUUThG0iOEhzIFejv0g3TX1SSAsdrT2aGYnFqoQILgQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "heap-js": "^2.3.0" }, @@ -29304,7 +29305,7 @@ "is-recoverable-error": "^1.0.3", "js-yaml": "^4.1.0", "mongodb-connection-string-url": "^3.0.1", - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "numeral": "^2.0.6", "pretty-repl": "^4.0.1", "semver": "^7.5.4", @@ -29360,6 +29361,18 @@ "js-yaml": "bin/js-yaml.js" } }, + "packages/cli-repl/node_modules/mongodb-log-writer": { + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/mongodb-log-writer/-/mongodb-log-writer-2.3.0.tgz", + "integrity": "sha512-EfraRB3vfzxRUSPKlrO2g5kSdEy8wwD49jxX3FP4Yye1R88aQZAQ3g+0VACyJvJ4cWU1yqZcFpbvRaLQK4GC3g==", + "license": "Apache-2.0", + "dependencies": { + "heap-js": "^2.3.0" + }, + "peerDependencies": { + "bson": "6.x" + } + }, "packages/connectivity-tests": { "name": "@mongosh/connectivity-tests", "version": "2.4.0", @@ -29393,7 +29406,7 @@ "lodash": "^4.17.21", "moment": "^2.29.1", "mongodb": "^6.12.0", - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "node-fetch": "^3.3.2", "prettier": "^2.8.8", "rimraf": "^3.0.2" @@ -29440,6 +29453,19 @@ "node": ">= 12" } }, + "packages/e2e-tests/node_modules/mongodb-log-writer": { + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/mongodb-log-writer/-/mongodb-log-writer-2.3.0.tgz", + "integrity": "sha512-EfraRB3vfzxRUSPKlrO2g5kSdEy8wwD49jxX3FP4Yye1R88aQZAQ3g+0VACyJvJ4cWU1yqZcFpbvRaLQK4GC3g==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "heap-js": "^2.3.0" + }, + "peerDependencies": { + "bson": "6.x" + } + }, "packages/e2e-tests/node_modules/node-fetch": { "version": "3.3.2", "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-3.3.2.tgz", @@ -29671,7 +29697,7 @@ "@mongosh/errors": "2.4.0", "@mongosh/history": "2.4.2", "@mongosh/types": "3.2.0", - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "mongodb-redact": "^1.1.5" }, "devDependencies": { @@ -29686,6 +29712,18 @@ "node": ">=14.15.1" } }, + "packages/logging/node_modules/mongodb-log-writer": { + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/mongodb-log-writer/-/mongodb-log-writer-2.3.0.tgz", + "integrity": "sha512-EfraRB3vfzxRUSPKlrO2g5kSdEy8wwD49jxX3FP4Yye1R88aQZAQ3g+0VACyJvJ4cWU1yqZcFpbvRaLQK4GC3g==", + "license": "Apache-2.0", + "dependencies": { + "heap-js": "^2.3.0" + }, + "peerDependencies": { + "bson": "6.x" + } + }, "packages/logging/node_modules/mongodb-redact": { "version": "1.1.5", "resolved": "https://registry.npmjs.org/mongodb-redact/-/mongodb-redact-1.1.5.tgz", diff --git a/packages/cli-repl/package.json b/packages/cli-repl/package.json index 4c44aed24d..9e2501cba3 100644 --- a/packages/cli-repl/package.json +++ b/packages/cli-repl/package.json @@ -85,7 +85,7 @@ "is-recoverable-error": "^1.0.3", "js-yaml": "^4.1.0", "mongodb-connection-string-url": "^3.0.1", - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "numeral": "^2.0.6", "pretty-repl": "^4.0.1", "semver": "^7.5.4", diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 35ac806535..10c23ee695 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -326,6 +326,7 @@ describe('CliRepl', function () { 'logRetentionDays', 'logMaxFileCount', 'logCompressionEnabled', + 'logRetentionGB', ] satisfies (keyof CliUserConfig)[]); }); @@ -1460,6 +1461,19 @@ describe('CliRepl', function () { ); }); + it('can set log retention GB', async function () { + const testLogRetentionGB = 10; + cliRepl.config.logRetentionGB = testLogRetentionGB; + await cliRepl.start(await testServer.connectionString(), {}); + + expect(cliRepl.getConfig('logRetentionGB')).equals( + testLogRetentionGB + ); + expect(cliRepl.logManager?._options.logRetentionGB).equals( + testLogRetentionGB + ); + }); + it('can set log max file count', async function () { const testMaxFileCount = 123; cliRepl.config.logMaxFileCount = testMaxFileCount; diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts index d040b189c8..67179d1d58 100644 --- a/packages/cli-repl/src/cli-repl.ts +++ b/packages/cli-repl/src/cli-repl.ts @@ -262,6 +262,7 @@ export class CliRepl implements MongoshIOProvider { retentionDays: await this.getConfig('logRetentionDays'), gzip: await this.getConfig('logCompressionEnabled'), maxLogFileCount: await this.getConfig('logMaxFileCount'), + retentionGB: await this.getConfig('logRetentionGB'), onerror: (err: Error) => this.bus.emit('mongosh:error', err, 'log'), onwarn: (err: Error, path: string) => this.warnAboutInaccessibleFile(err, path), diff --git a/packages/e2e-tests/package.json b/packages/e2e-tests/package.json index 35ae98d88c..851668fe88 100644 --- a/packages/e2e-tests/package.json +++ b/packages/e2e-tests/package.json @@ -33,7 +33,7 @@ "strip-ansi": "^6.0.0" }, "devDependencies": { - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "@mongodb-js/eslint-config-mongosh": "^1.0.0", "@mongodb-js/oidc-mock-provider": "^0.10.2", "@mongodb-js/prettier-config-devtools": "^1.0.1", diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 9316dfa166..0d7527c24a 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1825,6 +1825,56 @@ describe('e2e', function () { }); }); + describe('with custom log retention max logs size', function () { + const customLogDir = useTmpdir(); + + it('should delete files once it is above the logs retention GB', async function () { + const globalConfig = path.join(homedir, 'globalconfig.conf'); + await fs.writeFile( + globalConfig, + // Set logRetentionGB to 4 KB + `mongosh:\n logLocation: ${JSON.stringify( + customLogDir.path + )}\n logRetentionGB: ${4 / 1024 / 1024}` + ); + const paths: string[] = []; + const offset = Math.floor(Date.now() / 1000); + + // Create 10 log files, 1kb each + for (let i = 9; i >= 0; i--) { + const filename = path.join( + customLogDir.path, + ObjectId.createFromTime(offset - i).toHexString() + '_log' + ); + await fs.writeFile(filename, '0'.repeat(1024)); + paths.push(filename); + } + + // All 10 existing log files exist. + expect(await getFilesState(paths)).to.equal('1111111111'); + shell = this.startTestShell({ + args: ['--nodb'], + env, + globalConfigPath: globalConfig, + forceTerminal: true, + }); + + await shell.waitForPrompt(); + + // Add the newly created log to the file list. + paths.push( + path.join(customLogDir.path, `${shell.logId as string}_log`) + ); + + expect( + await shell.executeLine('config.get("logRetentionGB")') + ).contains(`${4 / 1024 / 1024}`); + + // Expect 6 files to be deleted and 5 to remain (including the new log file) + expect(await getFilesState(paths)).to.equal('00000011111'); + }); + }); + 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(); diff --git a/packages/logging/package.json b/packages/logging/package.json index 87f1a21115..f845c5f368 100644 --- a/packages/logging/package.json +++ b/packages/logging/package.json @@ -21,7 +21,7 @@ "@mongosh/errors": "2.4.0", "@mongosh/history": "2.4.2", "@mongosh/types": "3.2.0", - "mongodb-log-writer": "^2.1.0", + "mongodb-log-writer": "^2.3.0", "mongodb-redact": "^1.1.5" }, "devDependencies": { diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index cb4d5ad57f..8bd3ee3591 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -31,6 +31,12 @@ describe('config validation', function () { expect(await validate('logRetentionDays', -1)).to.equal( 'logRetentionDays must be a positive integer' ); + expect(await validate('logRetentionGB', 'foo')).to.equal( + 'logRetentionGB must be a positive integer' + ); + expect(await validate('logRetentionDays', -1)).to.equal( + 'logRetentionGB must be a positive integer' + ); expect(await validate('logMaxFileCount', 'foo')).to.equal( 'logMaxFileCount must be a positive integer' ); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index e6ad502efb..361061acfd 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -515,6 +515,7 @@ export class CliUserConfig extends SnippetShellUserConfig { logRetentionDays = 30; logMaxFileCount = 100; logCompressionEnabled = false; + logRetentionGB: number | undefined = undefined; } export class CliUserConfigValidator extends SnippetShellUserConfigValidator { @@ -543,6 +544,11 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { return `${key} must be a positive integer`; } return null; + case 'logRetentionGB': + if (value !== undefined && (typeof value !== 'number' || value < 0)) { + return `${key} must be a positive number or undefined`; + } + return null; case 'disableLogging': case 'forceDisableTelemetry': case 'showStackTraces': @@ -595,7 +601,7 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { - return `${key} must be a valid absolute path or empty`; + return `${key} must be a valid absolute path or undefined`; } return null; default: From 069cab2f5032d906f4a2db1f4fbe139824853380 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 11 Feb 2025 16:57:18 +0100 Subject: [PATCH 2/4] fix(tests): update tests to match validator and config changes --- packages/cli-repl/src/cli-repl.spec.ts | 4 ++-- packages/e2e-tests/test/e2e.spec.ts | 4 ++-- packages/types/src/index.spec.ts | 7 ++++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 10c23ee695..99014f6f67 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1466,10 +1466,10 @@ describe('CliRepl', function () { cliRepl.config.logRetentionGB = testLogRetentionGB; await cliRepl.start(await testServer.connectionString(), {}); - expect(cliRepl.getConfig('logRetentionGB')).equals( + expect(await cliRepl.getConfig('logRetentionGB')).equals( testLogRetentionGB ); - expect(cliRepl.logManager?._options.logRetentionGB).equals( + expect(cliRepl.logManager?._options.retentionGB).equals( testLogRetentionGB ); }); diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 0d7527c24a..7c5a9e7a4a 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1581,7 +1581,7 @@ describe('e2e', function () { await shell.waitForPrompt(); shell.assertContainsOutput('Ignoring config option "logLocation"'); shell.assertContainsOutput( - 'must be a valid absolute path or empty' + 'must be a valid absolute path or undefined' ); expect( @@ -1589,7 +1589,7 @@ describe('e2e', function () { 'config.set("logLocation", "[123123123123]")' ) ).contains( - 'Cannot set option "logLocation": logLocation must be a valid absolute path or empty' + 'Cannot set option "logLocation": logLocation must be a valid absolute path or undefined' ); }); diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index 8bd3ee3591..154bad2e95 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -32,8 +32,13 @@ describe('config validation', function () { 'logRetentionDays must be a positive integer' ); expect(await validate('logRetentionGB', 'foo')).to.equal( - 'logRetentionGB must be a positive integer' + 'logRetentionGB must be a positive number or undefined' + ); + expect(await validate('logRetentionGB', -1)).to.equal( + 'logRetentionGB must be a positive number or undefined' ); + expect(await validate('logRetentionGB', undefined)).to.equal(null); + expect(await validate('logRetentionGB', 100)).to.equal(null); expect(await validate('logRetentionDays', -1)).to.equal( 'logRetentionGB must be a positive integer' ); From f58ecb86fac44fda2db80d9c17f367a073f500e4 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 11 Feb 2025 22:42:52 +0100 Subject: [PATCH 3/4] fix(tests): use 10kb files for the test and update validation --- packages/e2e-tests/test/e2e.spec.ts | 27 ++++++++++++++------------- packages/types/src/index.spec.ts | 3 --- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 7c5a9e7a4a..32f76d347d 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1722,10 +1722,10 @@ describe('e2e', function () { }); }); - describe('with custom log retention days', function () { + describe('with logRetentionDays', function () { const customLogDir = useTmpdir(); - it('should delete older files according to the setting', async function () { + it('should delete older files older than logRetentionDays', async function () { const paths: string[] = []; const today = Math.floor(Date.now() / 1000); const tenDaysAgo = today - 10 * 24 * 60 * 60; @@ -1776,10 +1776,10 @@ describe('e2e', function () { }); }); - describe('with custom log retention max file count', function () { + describe('with logMaxFileCount', function () { const customLogDir = useTmpdir(); - it('should delete files once it is above the max file count limit', async function () { + it('should delete files once it is above logMaxFileCount', async function () { const globalConfig = path.join(homedir, 'globalconfig.conf'); await fs.writeFile( globalConfig, @@ -1825,28 +1825,28 @@ describe('e2e', function () { }); }); - describe('with custom log retention max logs size', function () { + describe('with logRetentionGB', function () { const customLogDir = useTmpdir(); - it('should delete files once it is above the logs retention GB', async function () { + it('should delete files once it is above logRetentionGB', async function () { const globalConfig = path.join(homedir, 'globalconfig.conf'); await fs.writeFile( globalConfig, - // Set logRetentionGB to 4 KB + // Set logRetentionGB to 40 KB and we will create prior 10 log files, 10 KB each `mongosh:\n logLocation: ${JSON.stringify( customLogDir.path - )}\n logRetentionGB: ${4 / 1024 / 1024}` + )}\n logRetentionGB: ${40 / 1024 / 1024}` ); const paths: string[] = []; const offset = Math.floor(Date.now() / 1000); - // Create 10 log files, 1kb each + // Create 10 log files, 10kb each for (let i = 9; i >= 0; i--) { const filename = path.join( customLogDir.path, ObjectId.createFromTime(offset - i).toHexString() + '_log' ); - await fs.writeFile(filename, '0'.repeat(1024)); + await fs.writeFile(filename, '0'.repeat(1024 * 10)); paths.push(filename); } @@ -1868,10 +1868,11 @@ describe('e2e', function () { expect( await shell.executeLine('config.get("logRetentionGB")') - ).contains(`${4 / 1024 / 1024}`); + ).contains(`${40 / 1024 / 1024}`); - // Expect 6 files to be deleted and 5 to remain (including the new log file) - expect(await getFilesState(paths)).to.equal('00000011111'); + // Expect 6 files to be deleted and 4 to remain + // (including the new log file which should be <10 kb) + expect(await getFilesState(paths)).to.equal('00000001111'); }); }); diff --git a/packages/types/src/index.spec.ts b/packages/types/src/index.spec.ts index 154bad2e95..4cd08156a4 100644 --- a/packages/types/src/index.spec.ts +++ b/packages/types/src/index.spec.ts @@ -39,9 +39,6 @@ describe('config validation', function () { ); expect(await validate('logRetentionGB', undefined)).to.equal(null); expect(await validate('logRetentionGB', 100)).to.equal(null); - expect(await validate('logRetentionDays', -1)).to.equal( - 'logRetentionGB must be a positive integer' - ); expect(await validate('logMaxFileCount', 'foo')).to.equal( 'logMaxFileCount must be a positive integer' ); From 70ce60bed4fbf1ad1f3fff8030d268c13c4042d9 Mon Sep 17 00:00:00 2001 From: gagik Date: Wed, 12 Feb 2025 09:48:06 +0100 Subject: [PATCH 4/4] fix: adjust size to be a megabyte --- packages/e2e-tests/test/e2e.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 32f76d347d..1ae15b1c30 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1832,21 +1832,21 @@ describe('e2e', function () { const globalConfig = path.join(homedir, 'globalconfig.conf'); await fs.writeFile( globalConfig, - // Set logRetentionGB to 40 KB and we will create prior 10 log files, 10 KB each + // Set logRetentionGB to 4 MB and we will create prior 10 log files, 1 MB each `mongosh:\n logLocation: ${JSON.stringify( customLogDir.path - )}\n logRetentionGB: ${40 / 1024 / 1024}` + )}\n logRetentionGB: ${4 / 1024}` ); const paths: string[] = []; const offset = Math.floor(Date.now() / 1000); - // Create 10 log files, 10kb each + // Create 10 log files, around 1 mb each for (let i = 9; i >= 0; i--) { const filename = path.join( customLogDir.path, ObjectId.createFromTime(offset - i).toHexString() + '_log' ); - await fs.writeFile(filename, '0'.repeat(1024 * 10)); + await fs.writeFile(filename, '0'.repeat(1024 * 1024)); paths.push(filename); } @@ -1868,10 +1868,10 @@ describe('e2e', function () { expect( await shell.executeLine('config.get("logRetentionGB")') - ).contains(`${40 / 1024 / 1024}`); + ).contains(`${4 / 1024}`); // Expect 6 files to be deleted and 4 to remain - // (including the new log file which should be <10 kb) + // (including the new log file which should be <1 MB) expect(await getFilesState(paths)).to.equal('00000001111'); }); });