From 231585fce669c4367d920f88aa2751f5fbbc50c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Sun, 7 Apr 2024 19:04:23 -0300 Subject: [PATCH] fix(perf): remove glob dependency --- lib/utils/log-file.js | 70 +++++++++++++++++++++++++------------- test/lib/utils/log-file.js | 52 ++++++++++++---------------- 2 files changed, 67 insertions(+), 55 deletions(-) diff --git a/lib/utils/log-file.js b/lib/utils/log-file.js index 8c06f5647e761..1dac366fdbfa6 100644 --- a/lib/utils/log-file.js +++ b/lib/utils/log-file.js @@ -1,7 +1,6 @@ const os = require('os') const { join, dirname, basename } = require('path') const { format } = require('util') -const { glob } = require('glob') const { Minipass } = require('minipass') const fsMiniPass = require('fs-minipass') const fs = require('fs/promises') @@ -9,7 +8,6 @@ const log = require('./log-shim') const Display = require('./display') const padZero = (n, length) => n.toString().padStart(length.toString().length, '0') -const globify = pattern => pattern.split('\\').join('/') class LogFiles { // Default to a plain minipass stream so we can buffer @@ -33,7 +31,7 @@ class LogFiles { #logsMax = null #files = [] - constructor ({ + constructor({ maxLogsPerFile = 50_000, maxFilesPerProcess = 5, } = {}) { @@ -42,7 +40,7 @@ class LogFiles { this.on() } - static format (count, level, title, ...args) { + static format(count, level, title, ...args) { let prefix = `${count} ${level}` if (title) { prefix += ` ${title}` @@ -53,21 +51,21 @@ class LogFiles { .map(Display.clean) .reduce((lines, line) => lines += prefix + (line ? ' ' : '') + line + os.EOL, - '' + '' ) } - on () { + on() { this.#logStream = new Minipass() process.on('log', this.#logHandler) } - off () { + off() { process.off('log', this.#logHandler) this.#endStream() } - load ({ path, logsMax = Infinity } = {}) { + load({ path, logsMax = Infinity } = {}) { // dir is user configurable and is required to exist so // this can error if the dir is missing or not configured correctly this.#path = path @@ -96,19 +94,19 @@ class LogFiles { return this.#cleanLogs() } - log (...args) { + log(...args) { this.#logHandler(...args) } - get files () { + get files() { return this.#files } - get #isBuffered () { + get #isBuffered() { return this.#logStream instanceof Minipass } - #endStream (output) { + #endStream(output) { if (this.#logStream) { this.#logStream.end(output) this.#logStream = null @@ -152,16 +150,16 @@ class LogFiles { } } - #formatLogItem (...args) { + #formatLogItem(...args) { this.#fileLogCount += 1 return LogFiles.format(this.#totalLogCount++, ...args) } - #getLogFilePath (count = '') { + #getLogFilePath(count = '') { return `${this.#path}debug-${count}.log` } - #openLogFile () { + #openLogFile() { // Count in filename will be 0 indexed const count = this.#files.length @@ -190,7 +188,7 @@ class LogFiles { } } - async #cleanLogs () { + async #cleanLogs() { // module to clean out the old log files // this is a best-effort attempt. if a rm fails, we just // log a message about it and move on. We do return a @@ -199,17 +197,41 @@ class LogFiles { try { const logPath = this.#getLogFilePath() - const logGlob = join(dirname(logPath), basename(logPath) + const patternFileName = basename(logPath) // tell glob to only match digits - .replace(/\d/g, '[0123456789]') + .replace(/\d/g, 'd') // Handle the old (prior to 8.2.0) log file names which did not have a // counter suffix - .replace(/-\.log$/, '*.log') - ) + .replace('-.log', '') + + let files = await fs.readdir( + dirname(logPath), { + withFileTypes: true, + encoding: 'utf-8', + }) + files = files.sort((a, b) => basename(a.name).localeCompare(basename(b.name), 'en')) + + const logFiles = [] + + for (const file of files) { + if (!file.isFile()) { + continue + } + + const genericFileName = file.name.replace(/\d/g, 'd') + const filePath = join(dirname(logPath), basename(file.name)) + + // Always ignore the currently written files + if ( + genericFileName.includes(patternFileName) + && genericFileName.endsWith('.log') + && !this.#files.includes(filePath) + ) { + logFiles.push(filePath) + } + } - // Always ignore the currently written files - const files = await glob(globify(logGlob), { ignore: this.#files.map(globify), silent: true }) - const toDelete = files.length - this.#logsMax + const toDelete = logFiles.length - this.#logsMax if (toDelete <= 0) { return @@ -217,7 +239,7 @@ class LogFiles { log.silly('logfile', `start cleaning logs, removing ${toDelete} files`) - for (const file of files.slice(0, toDelete)) { + for (const file of logFiles.slice(0, toDelete)) { try { await fs.rm(file, { force: true }) } catch (e) { diff --git a/test/lib/utils/log-file.js b/test/lib/utils/log-file.js index c02f338a84ee0..f34dda8f52433 100644 --- a/test/lib/utils/log-file.js +++ b/test/lib/utils/log-file.js @@ -57,8 +57,10 @@ const loadLogFile = async (t, { buffer = [], mocks, testdir = {}, ...options } = logFile, LogFile, readLogs: async () => { - const logDir = await fs.readdir(root) - const logFiles = logDir.map((f) => path.join(root, f)) + const logDir = await fs.readdir(root, { withFileTypes: true }) + const logFiles = logDir + .filter(f => f.isFile()) + .map((f) => path.join(root, f.name)) .filter((f) => _fs.existsSync(f)) return Promise.all(logFiles.map(async (f) => { const content = await fs.readFile(f, 'utf8') @@ -202,6 +204,22 @@ t.test('cleans logs', async t => { t.equal(logs.length, logsMax + 1) }) +t.test('cleans logs even when find folder inside logs folder', async t => { + const logsMax = 5 + const { readLogs } = await loadLogFile(t, { + logsMax, + testdir: { + ...makeOldLogs(10), + ignore_folder: { + 'ignored-file.txt': 'hello', + }, + }, + }) + + const logs = await readLogs() + t.equal(logs.length, logsMax + 1) +}) + t.test('doesnt clean current log by default', async t => { const logsMax = 1 const { readLogs, logFile } = await loadLogFile(t, { @@ -240,35 +258,6 @@ t.test('doesnt need to clean', async t => { t.equal(logs.length, oldLogs + 1) }) -t.test('glob error', async t => { - const { readLogs } = await loadLogFile(t, { - logsMax: 5, - mocks: { - glob: { glob: () => { - throw new Error('bad glob') - } }, - }, - }) - - const logs = await readLogs() - t.equal(logs.length, 1) - t.match(last(logs).content, /error cleaning log files .* bad glob/) -}) - -t.test('do not log cleaning errors when logging is disabled', async t => { - const { readLogs } = await loadLogFile(t, { - logsMax: 0, - mocks: { - glob: () => { - throw new Error('should not be logged') - }, - }, - }) - - const logs = await readLogs() - t.equal(logs.length, 0) -}) - t.test('cleans old style logs too', async t => { const logsMax = 5 const oldLogs = 10 @@ -290,6 +279,7 @@ t.test('rimraf error', async t => { testdir: makeOldLogs(oldLogs), mocks: { 'fs/promises': { + readdir: fs.readdir, rm: async (...args) => { if (count >= 3) { throw new Error('bad rimraf')