Conversation
src/util/logger.ts
Outdated
| translateTime: 'SYS:HH:MM:ss', | ||
| }, | ||
| const loggerOptions = typeof options === 'string' ? { name: options } : options; | ||
| const defaultLevel = loggerOptions.level ?? 'info'; |
There was a problem hiding this comment.
I believe it's good opportunity to introduce log level
src/config.ts
Outdated
| LOG_FILE_PATH: z | ||
| .string() | ||
| .optional() | ||
| .describe('Full path to log file (e.g., /var/log/app.log or ./logs/app.log)'), |
There was a problem hiding this comment.
Please add information if no value is provided then file logging is disabled
src/util/logger.ts
Outdated
| { level: defaultLevel, stream: prettyTransport }, | ||
| ]; | ||
|
|
||
| return pino({ ...loggerOptions, level: defaultLevel }, pino.multistream(streams)); |
There was a problem hiding this comment.
is this log going to include PID in each log? I would say it's necessary so we can distinguish it for each deployment
There was a problem hiding this comment.
by default already had pid
| // Cache for transport instances to prevent duplicate worker processes | ||
| const transportCache = new Map<string, ReturnType<typeof pino.transport>>(); | ||
|
|
||
| export const createLogger = (options: string | LoggerOptions): Logger => { |
There was a problem hiding this comment.
this function became too big, can you split it into smaller ones so it's more readable?
There was a problem hiding this comment.
also what I'm thinking you should add logs to some buffer while you createLogger and later print logger configuration (paths etc.)
src/util/logger.ts
Outdated
| // If path ends with / or \, append default filename | ||
| if (logFilePath.endsWith('/') || logFilePath.endsWith('\\')) { | ||
| logFilePath = path.join(logFilePath, 'app.log'); | ||
| } else if (path.extname(logFilePath) === '') { |
There was a problem hiding this comment.
why not use .length === 0 or ideally .trim().length === 0
src/util/logger.ts
Outdated
|
|
||
| // Parse path for pino-roll configuration | ||
| const parsedPath = path.parse(logFilePath); | ||
| const baseFileName = parsedPath.name; // filename without extension |
There was a problem hiding this comment.
why do we need to reassign here?
src/util/logger.ts
Outdated
| // Parse path for pino-roll configuration | ||
| const parsedPath = path.parse(logFilePath); | ||
| const baseFileName = parsedPath.name; // filename without extension | ||
| const extension = parsedPath.ext !== '' ? parsedPath.ext : '.log'; |
There was a problem hiding this comment.
same as above comment about length
src/routes/admin.ts
Outdated
| }); | ||
|
|
||
| // Perform graceful shutdown after response is sent | ||
| setTimeout(() => { |
There was a problem hiding this comment.
I think it's better to just send SIGTERM and add event listener on it.
|
|
||
| // Send response immediately before shutdown | ||
| res.status(200).json({ | ||
| success: true, |
There was a problem hiding this comment.
200 indicates that request was successful
There was a problem hiding this comment.
but IMO it's incorrect - we don't know if shutdown was successful
There was a problem hiding this comment.
WNA had query check server are up/down.
| }, 100); // Small delay to ensure response is sent | ||
| } catch (error) { | ||
| adminLogger.error({ error }, 'error initiating shutdown'); | ||
| res.status(500).json({ |
There was a problem hiding this comment.
this will lead to error where response is sent twice
There was a problem hiding this comment.
had checking in shutdown.ts.
if (isShuttingDown) { logger.warn('shutdown already in progress'); return; }
src/shutdown.ts
Outdated
| // 5. Clear timeout and complete | ||
| clearTimeout(shutdownTimeout); | ||
| logger.info('graceful shutdown completed'); | ||
| process.exit(0); |
There was a problem hiding this comment.
you need to drain also fastq queues
hejkerooo
left a comment
There was a problem hiding this comment.
health module should not be responsible for any of this
src/health/health.ts
Outdated
| const solutionInfos: SolutionInfo[] = solutions.map((solution) => { | ||
| // Extract name from solutionId (format: "name.uuid") | ||
| // Example: "newTestGPSaaS.1348d595-ccc4-4a38-85ad-f0e31cc7f410" -> "newTestGPSaaS" | ||
| const name = solution.solutionId.includes('.') |
There was a problem hiding this comment.
why this functionality? what is the purpose? It's not deterministic
There was a problem hiding this comment.
that get solution name only for UI display
src/health/health.ts
Outdated
| }; | ||
| }; | ||
|
|
||
| const getOperatorInfo = async (timeoutMs: number = 5000): Promise<OperatorInfo | undefined> => { |
There was a problem hiding this comment.
move it to some different place and export it as public function.
There was a problem hiding this comment.
any place that suitable. try not create new file for this
4d7c708 to
f6df7bc
Compare
Feat/wna server
fix: handle network unavailability throw error
Total scope:
GET /tokento return access token from pallet auth server,Docker tag:
feat-auth-0.1