-
Notifications
You must be signed in to change notification settings - Fork 4
#8 Winston Logger ECS Deployment Fix #9
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ export interface AppConfig { | |
DEFAULT_NODE_ENV: 'development' | 'production' | 'test'; | ||
DEFAULT_LOG_LEVEL: 'error' | 'warn' | 'info' | 'debug' | 'verbose'; | ||
DEFAULT_LOG_DIR: string; | ||
DEFAULT_ENABLE_FILE_LOGGING: boolean; | ||
} | ||
|
||
export interface KafkaConfig { | ||
|
@@ -51,6 +52,7 @@ export const CONFIG: Config = { | |
DEFAULT_NODE_ENV: 'development', | ||
DEFAULT_LOG_LEVEL: 'info', | ||
DEFAULT_LOG_DIR: 'logs', | ||
DEFAULT_ENABLE_FILE_LOGGING: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making |
||
}, | ||
KAFKA: { | ||
DEFAULT_CLIENT_ID: 'autopilot-service', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,51 +17,86 @@ export class LoggerService implements NestLoggerService { | |
} | ||
|
||
private createWinstonLogger(): Logger { | ||
const baseTransports: any[] = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using |
||
new transports.Console({ | ||
level: process.env.LOG_LEVEL || CONFIG.APP.DEFAULT_LOG_LEVEL, | ||
format: format.combine( | ||
format.colorize(), | ||
format.printf(({ timestamp, level, message, context, ...meta }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
const formattedTimestamp = | ||
timestamp instanceof Date | ||
? timestamp.toISOString() | ||
: String(timestamp); | ||
const formattedContext = | ||
typeof context === 'string' ? context : 'App'; | ||
const formattedMessage = | ||
typeof message === 'string' | ||
? message | ||
: typeof message === 'object' && message !== null | ||
? JSON.stringify(message) | ||
: String(message); | ||
|
||
return `${formattedTimestamp} [${formattedContext}] ${String(level)}: ${formattedMessage}${ | ||
Object.keys(meta).length | ||
? ' ' + JSON.stringify(meta, null, 1) | ||
: '' | ||
}`; | ||
}), | ||
), | ||
}), | ||
]; | ||
|
||
// Add file transports conditionally based on environment | ||
const shouldEnableFileLogging = this.shouldEnableFileLogging(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method |
||
if (shouldEnableFileLogging) { | ||
try { | ||
baseTransports.push( | ||
new transports.File({ | ||
filename: `${process.env.LOG_DIR || CONFIG.APP.DEFAULT_LOG_DIR}/error.log`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file path for the log files is constructed using environment variables or default configuration. Ensure that |
||
level: 'error', | ||
}), | ||
new transports.File({ | ||
filename: `${process.env.LOG_DIR || CONFIG.APP.DEFAULT_LOG_DIR}/combined.log`, | ||
}), | ||
); | ||
} catch (error) { | ||
// If file transport creation fails (e.g., read-only filesystem), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error handling for file transport creation logs a warning to the console. Consider using the existing logger to log this warning instead of |
||
// log a warning but continue with console-only logging | ||
console.warn( | ||
'Failed to initialize file transports, falling back to console-only logging:', | ||
error instanceof Error ? error.message : String(error), | ||
); | ||
} | ||
} | ||
|
||
return createLogger({ | ||
format: format.combine( | ||
format.timestamp(), | ||
format.errors({ stack: true }), | ||
format.json(), | ||
), | ||
defaultMeta: { context: this.context }, | ||
transports: [ | ||
new transports.Console({ | ||
level: process.env.LOG_LEVEL || CONFIG.APP.DEFAULT_LOG_LEVEL, | ||
format: format.combine( | ||
format.colorize(), | ||
format.printf(({ timestamp, level, message, context, ...meta }) => { | ||
const formattedTimestamp = | ||
timestamp instanceof Date | ||
? timestamp.toISOString() | ||
: String(timestamp); | ||
const formattedContext = | ||
typeof context === 'string' ? context : 'App'; | ||
const formattedMessage = | ||
typeof message === 'string' | ||
? message | ||
: typeof message === 'object' && message !== null | ||
? JSON.stringify(message) | ||
: String(message); | ||
|
||
return `${formattedTimestamp} [${formattedContext}] ${String(level)}: ${formattedMessage}${ | ||
Object.keys(meta).length | ||
? ' ' + JSON.stringify(meta, null, 1) | ||
: '' | ||
}`; | ||
}), | ||
), | ||
}), | ||
new transports.File({ | ||
filename: `${process.env.LOG_DIR || CONFIG.APP.DEFAULT_LOG_DIR}/error.log`, | ||
level: 'error', | ||
}), | ||
new transports.File({ | ||
filename: `${process.env.LOG_DIR || CONFIG.APP.DEFAULT_LOG_DIR}/combined.log`, | ||
}), | ||
], | ||
transports: baseTransports, | ||
}); | ||
} | ||
|
||
/** | ||
* Determines whether file logging should be enabled based on environment configuration | ||
*/ | ||
private shouldEnableFileLogging(): boolean { | ||
const nodeEnv = process.env.NODE_ENV; | ||
const enableFileLogging = process.env.ENABLE_FILE_LOGGING; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding validation for the |
||
|
||
// In production, only enable file logging if explicitly requested | ||
if (nodeEnv === 'production') { | ||
return enableFileLogging === 'true'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The condition |
||
} | ||
|
||
// In non-production environments (development, test), enable file logging by default | ||
// unless explicitly disabled | ||
return enableFileLogging !== 'false'; | ||
} | ||
|
||
private formatMessage(message: unknown): string { | ||
if (typeof message === 'string') { | ||
return message; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,6 @@ export default registerAs('app', () => ({ | |
logging: { | ||
level: process.env.LOG_LEVEL || 'info', | ||
directory: process.env.LOG_DIR || 'logs', | ||
enableFileLogging: process.env.ENABLE_FILE_LOGGING === 'true', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a more explicit check for the environment variable, such as |
||
}, | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider setting
ENABLE_FILE_LOGGING
tofalse
by default in the example environment file to prevent accidental file logging in production environments.