From b64cc72578ea10966703af2ad87a77e603a49139 Mon Sep 17 00:00:00 2001 From: mshanemc Date: Thu, 29 Jun 2023 15:33:07 -0500 Subject: [PATCH] style: comments from self-review --- src/logger/cleanup.ts | 13 +++++++++++-- src/logger/filters.ts | 7 ++++--- src/logger/logger.ts | 24 +++++++++++------------- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/logger/cleanup.ts b/src/logger/cleanup.ts index a18e0cc11b..c0c02e7b09 100644 --- a/src/logger/cleanup.ts +++ b/src/logger/cleanup.ts @@ -5,7 +5,7 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ import * as fs from 'node:fs'; -import path = require('node:path'); +import { join } from 'node:path'; import { Global } from '../global'; import { Logger } from './logger'; @@ -20,6 +20,15 @@ const MAX_FILE_AGE_MS = 1000 * 60 * 60 * 24 * MAX_FILE_AGE_DAYS; const shouldClean = Math.random() * CLEAN_ODDS > CLEAN_ODDS - 1; +/** + * New logger (Summer 2023) changes how file rotation works. Each day, the logger writes to a new file + * To get old files cleaned up, this can be called when a new root logger is instantiated + * based on CLEAN_ODDS, it could exit OR delete some old log files + * + * to start this without waiting, use void cleanup() + * + * accepts params to override the default behavior (used to cleanup huge log file during perf tests) + */ export const cleanup = async (maxMs = MAX_FILE_AGE_MS, force = false): Promise => { if (shouldClean || force) { try { @@ -27,7 +36,7 @@ export const cleanup = async (maxMs = MAX_FILE_AGE_MS, force = false): Promise fs.promises.unlink(path.join(Global.SF_DIR, f)))); + await Promise.all(filesToDelete.map((f) => fs.promises.unlink(join(Global.SF_DIR, f)))); } catch (e) { // we never, ever, ever throw since we're not awaiting this promise, so just log a warning (await Logger.child('cleanup')).warn('Failed to cleanup old log files', e); diff --git a/src/logger/filters.ts b/src/logger/filters.ts index 3864114870..1bace000b9 100644 --- a/src/logger/filters.ts +++ b/src/logger/filters.ts @@ -5,7 +5,7 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ import { isArray, isObject, isString } from '@salesforce/ts-types'; -import { accessTokenRegex, sfdxAuthUrlRegex } from '../exported'; +import { accessTokenRegex, sfdxAuthUrlRegex } from '../util/sfdc'; export const HIDDEN = 'HIDDEN'; type FilteredKeyDefinition = { name: string; regex?: string }; @@ -28,7 +28,7 @@ const buildKeyRegex = (expElement: string): RegExp => 'gi' ); -// Ok to log clientid +// This will redact values when the keys match certain patterns const FILTERED_KEYS: FilteredKeyDefinition[] = [ { name: 'sid' }, { name: 'Authorization' }, @@ -54,7 +54,8 @@ const replacementFunctions = FILTERED_KEYS_FOR_PROCESSING.flatMap( (input: string): string => input.replace(key.keyRegex, `$1${key.hiddenAttrMessage}`), ] ).concat([ - // plus any "generalized" functions that are matching data and not keys + // plus any "generalized" functions that are matching contents regardless of keys + // use these for secrets with known patterns (input: string): string => input .replace(new RegExp(accessTokenRegex, 'g'), '') diff --git a/src/logger/logger.ts b/src/logger/logger.ts index 77c7bd6b68..c1d3a33bed 100644 --- a/src/logger/logger.ts +++ b/src/logger/logger.ts @@ -66,10 +66,12 @@ export interface LoggerOptions { */ level?: LoggerLevelValue; /** + * @deprecated don't pass in streams. this will no do anything * A stream to write to. */ stream?: Writable; /** + * @deprecated don't pass in streams. this will no do anything * An array of streams to write to. */ streams?: LoggerStream[]; @@ -79,7 +81,7 @@ export interface LoggerOptions { */ fields?: Fields; - /** log to memory instead of to a file */ + /** log to memory instead of to a file. Intended for Unit Testing */ useMemoryLogger?: boolean; } @@ -229,6 +231,7 @@ export class Logger { public readonly logRotationCount = new Env().getNumber('SF_LOG_ROTATION_COUNT') ?? 2; /** + * @deprecated. Has no effect, will always be false * Whether debug is enabled for this Logger. */ public debugEnabled = false; @@ -256,9 +259,8 @@ export class Logger { // if there is a rootLogger, use its Pino instance if (Logger.rootLogger) { - // modify the name this.pinoLogger = Logger.rootLogger.pinoLogger.child({ ...options.fields, name: options.name }); - this.memoryLogger = Logger.rootLogger.memoryLogger; + this.memoryLogger = Logger.rootLogger.memoryLogger; // if the root was constructed with memory logging, keep that this.pinoLogger.trace(`Created '${options.name}' child logger instance`); } else { const level = new Env().getString('SF_LOG_LEVEL') ?? pino.levels.labels[options.level ?? Logger.DEFAULT_LEVEL]; @@ -297,13 +299,12 @@ export class Logger { } Logger.rootLogger = this; - // run, but don't care about the result } } /** * - * Gets the root logger with the default level, file stream, and DEBUG enabled. + * Gets the root logger. It's a singleton * See also getRawLogger if you don't need the root logger */ public static async root(): Promise { @@ -311,7 +312,7 @@ export class Logger { } /** - * Gets the root logger with the default level, file stream, and DEBUG enabled. + * Gets the root logger. It's a singleton */ public static getRoot(): Logger { if (this.rootLogger) { @@ -333,7 +334,7 @@ export class Logger { } /** - * Create a child of the root logger, inheriting this instance's configuration such as `level`, `streams`, etc. + * Create a child of the root logger, inheriting this instance's configuration such as `level`, transports, etc. * * @param name The name of the child logger. * @param fields Additional fields included in all log lines. @@ -343,7 +344,7 @@ export class Logger { } /** - * Create a child of the root logger, inheriting this instance's configuration such as `level`, `streams`, etc. + * Create a child of the root logger, inheriting this instance's configuration such as `level`, transports, etc. * * @param name The name of the child logger. * @param fields Additional fields included in all log lines. @@ -532,7 +533,7 @@ export class Logger { /** * Create a child logger, typically to add a few log record fields. For convenience this object is returned. * - * @param name The name of the child logger that is emitted w/ log line as `log:`. + * @param name The name of the child logger that is emitted w/ log line. Will be prefixed with the parent logger name and `:` * @param fields Additional fields included in all log lines for the child logger. */ public child(name: string, fields: Fields = {}): Logger { @@ -638,8 +639,7 @@ export class Logger { public enableDEBUG(): void {} /** return various streams that the logger could send data to, depending on the options and env */ } -// eslint-disable-next-line @typescript-eslint/explicit-function-return-type -const getWriteStream = (level = 'warn') => { +const getWriteStream = (level = 'warn'): pino.TransportSingleOptions => { // used when debug mode, writes to stdout (colorized) if (process.env.DEBUG) { return { @@ -666,5 +666,3 @@ const getWriteStream = (level = 'warn') => { }, }; }; - -// TODO: telemetry as custom level (1)