Skip to content

feat: improve logging accross the apps#78

Merged
rulasfia merged 2 commits intodevelopfrom
better-logging
Mar 31, 2026
Merged

feat: improve logging accross the apps#78
rulasfia merged 2 commits intodevelopfrom
better-logging

Conversation

@rulasfia
Copy link
Copy Markdown
Owner

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

Code Review Summary

Overview

This PR introduces a centralized logging package using pino and refactors the codebase to use structured logging instead of console.log. Overall, it's a solid improvement that brings consistency and better observability.

Issues Found

1. Potential Bug: Missing Log Level Validation

packages/logger/src/index.ts:38

return getEnvOrDefault<LogLevel>(LOG_LEVEL_ENV, isProduction() ? 'info' : 'debug');

If LOG_LEVEL environment variable contains an invalid value (e.g., "verbose"), it will be accepted as-is but pino may behave unexpectedly. Consider validating the log level value against allowed levels.

2. Minor Issue: Redundant Log Level Type

packages/logger/src/index.ts:12

type LogLevel = 'trace' | 'debug' | 'info' | 'warn' | 'error' | 'fatal';

This duplicates the type already defined in CreateLoggerOptions.level. Consider deriving it from there instead.

3. Potential Issue: Error Handling in getEnvOrDefault

packages/logger/src/index.ts:18-29
The parser catches any error and returns the default, which could silently ignore configuration errors. Consider logging a warning when parsing fails.

4. Code Quality: Missing Tests

The new @reafrac/logger package has no tests. Given it's a foundational package used across the monorepo, consider adding unit tests for:

  • createLogger with different options
  • Environment variable resolution
  • Log level validation

Suggestions

  1. Add Log Level Validation:
const VALID_LOG_LEVELS: LogLevel[] = ['trace', 'debug', 'info', 'warn', 'error', 'fatal'];
function resolveLogLevel(explicit?: LogLevel): LogLevel {
  if (explicit) return explicit;
  const envLevel = process.env[LOG_LEVEL_ENV] as LogLevel;
  if (envLevel && VALID_LOG_LEVELS.includes(envLevel)) return envLevel;
  return isProduction() ? 'info' : 'debug';
}
  1. Derive LogLevel type from options interface:
type LogLevel = CreateLoggerOptions['level'];
  1. Consider adding a silent mode: For test environments, it would be useful to have a silent/quiet mode that suppresses all logs.

  2. Export a default logger: For simple use cases, export a default logger instance so consumers don't always need to call createLogger.

Positive Notes

✅ Good separation of concerns with the logger package
✅ Proper use of structured logging (objects instead of string interpolation)
✅ Consistent logging patterns across all applications
✅ Environment-aware defaults (pretty in dev, JSON in production)
✅ Child logger support for contextual logging
✅ Type safety maintained throughout

Verdict

Approve with minor suggestions. The PR is well-structured and improves the codebase significantly. The log level validation issue should be addressed before merging, but it's not a blocker.

New%20session%20-%202026-03-31T09%3A41%3A57.648Z
opencode session  |  github run

@rulasfia rulasfia merged commit 3154f6c into develop Mar 31, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant