Skip to content
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

bug: Improve logs command and configuration coherence #10

Open
6 tasks
raamsri opened this issue Dec 20, 2024 · 0 comments
Open
6 tasks

bug: Improve logs command and configuration coherence #10

raamsri opened this issue Dec 20, 2024 · 0 comments
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers Needs Fix The issue is understood and ready for code to be written.

Comments

@raamsri
Copy link
Contributor

raamsri commented Dec 20, 2024

Bug Report

Current Behavior

  1. Log command implementation has several issues:

    • Logic is directly in cmd package instead of pkg
    • Tails stale log files even when current instance runs in non-daemon mode
    • Lacks proper coordination between logs.path and logs.output settings
  2. Configuration inconsistencies:

    • Unclear precedence between logs.path and logs.output
    • No validation of configuration combinations
    • Missing proper defaults for different execution modes

Expected Behavior

  1. Move logging logic to pkg/logs:

    package logs
    
    type LogManager struct {
        config *config.Config
        logger logger.Logger
    }
    
    func (lm *LogManager) GetLogLocation() (string, error)
    func (lm *LogManager) ReadLogs(lines int, follow bool) error
    func (lm *LogManager) ValidateConfig() error
  2. Configuration precedence rules:

  • logs.output=stdout: Always write to stdout regardless of path
  • logs.output=file: Require valid logs.path
  • Daemon mode: Force logs.output=file with valid path
  1. Add validation for configuration combinations:
func ValidateLogsConfig(cfg *Config) error {
    if cfg.Logs.Output == "file" && cfg.Logs.Path == "" {
        return fmt.Errorf("logs.path required when output is file")
    }
    if cfg.Server.Daemonize && cfg.Logs.Output == "stdout" {
        return fmt.Errorf("daemon mode requires file output")
    }
    return nil
}

Implementation Plan

  1. Create new package structure:
pkg/
  logs/
    logs.go      # Core logging logic
    manager.go   # Log management functionality
    config.go    # Configuration validation
    reader.go    # Log reading/tailing
  1. Update configuration validation:
  • Add pre-start validation in serve command
  • Validate configuration combinations
  • Set appropriate defaults based on execution mode
  1. Improve logs command:
  • Move implementation to pkg/logs
  • Add proper process detection
  • Support multiple output modes
  • Add better error handling
  1. Configuration updates:
logs:
  output: "file"  # stdout or file
  path: "/var/log/rodent/rodent.log"
  retention: "7d"
  level: "info"

Code Changes

  1. Update logs.go
func NewLogsCmd() *cobra.Command {
    cmd := &cobra.Command{
        RunE: func(cmd *cobra.Command, args []string) error {
            manager := logs.NewManager(config.GetConfig())
            
            // Validate current instance
            if !manager.IsProcessRunning() {
                return fmt.Errorf("no active rodent process found")
            }
            
            return manager.ReadLogs(lines, follow)
        },
    }
}
  1. Add pkg/logs/manager.go:
type Manager struct {
    config *config.Config
    logger logger.Logger
}

func (m *Manager) IsProcessRunning() bool {
    pidFile := constants.RodentPIDFilePath
    pid, err := lifecycle.ReadPID(pidFile)
    if err != nil {
        return false
    }
    return lifecycle.IsProcessRunning(pid)
}

Migration Plan

Phase 1: Add new package structure
Phase 2: Update configuration validation
Phase 3: Migrate logs command
Phase 4: Add tests
Phase 5: Documentation updates

Acceptance Criteria

  • Log command works correctly in all execution modes
  • Configuration validation prevents invalid combinations
  • Proper process detection before tailing logs
  • Clear documentation on configuration precedence
  • Comprehensive test coverage
  • Migration guide for existing users

Additional Notes

  1. Consider adding:
  • Log rotation support
  • Structured logging
  • Log filtering options
  • Multiple log file support
  1. Future enhancements:
  • Remote log access
  • Log aggregation
  • Log shipping support
@raamsri raamsri added bug Something isn't working enhancement New feature or request good first issue Good for newcomers Needs Fix The issue is understood and ready for code to be written. labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request good first issue Good for newcomers Needs Fix The issue is understood and ready for code to be written.
Projects
None yet
Development

No branches or pull requests

1 participant