feat(cli-repl): add configuration to set max log files size MONGOSH-1985 (#2361)

Author: gagik

package-lock.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",

packages/cli-repl/package.json

@@ -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(await cliRepl.getConfig('logRetentionGB')).equals(
+              testLogRetentionGB
+            );
+            expect(cliRepl.logManager?._options.retentionGB).equals(
+              testLogRetentionGB
+            );
+          });
+
           it('can set log max file count', async function () {
             const testMaxFileCount = 123;
             cliRepl.config.logMaxFileCount = testMaxFileCount;

packages/cli-repl/src/cli-repl.spec.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),

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

@@ -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",

packages/e2e-tests/package.json

@@ -1581,15 +1581,15 @@ 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(
               await shell.executeLine(
                 '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'
             );
           });
 
@@ -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,6 +1825,57 @@ describe('e2e', function () {
           });
         });
 
+        describe('with logRetentionGB', function () {
+          const customLogDir = useTmpdir();
+
+          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 MB and we will create prior 10 log files, 1 MB each
+              `mongosh:\n  logLocation: ${JSON.stringify(
+                customLogDir.path
+              )}\n  logRetentionGB: ${4 / 1024}`
+            );
+            const paths: string[] = [];
+            const offset = Math.floor(Date.now() / 1000);
+
+            // 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 * 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}`);
+
+            // Expect 6 files to be deleted and 4 to remain
+            // (including the new log file which should be <1 MB)
+            expect(await getFilesState(paths)).to.equal('00000001111');
+          });
+        });
+
         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();

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

@@ -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": {

packages/logging/package.json

@@ -31,6 +31,14 @@ 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 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('logMaxFileCount', 'foo')).to.equal(
       'logMaxFileCount must be a positive integer'
     );

packages/types/src/index.spec.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: