feat: implement comprehensive logging system with tracing #94

mohsen1 · 2025-09-19T09:27:03Z

Summary

This PR implements Milestone 1.3: Logging System as described in issue #59.

Changes Made

Core Logging Infrastructure

Replaced log crate with tracing for structured logging
Added tracing-subscriber with env-filter support for configurable log levels
Implemented log file rotation using tracing-appender with daily rotation to logs/bodo.log
Added dual output: console logging for user interaction and JSON file logging for structured analysis

CLI Enhancements

Added --verbose flag for debug-level logging
Added --quiet flag for error-only logging
Enhanced log level control with environment variable support (RUST_LOG)

Performance Monitoring

Added tracing spans to key functions:
- GraphManager::build_graph() - tracks graph construction performance
- GraphManager::run_plugins() - tracks plugin execution performance
- ExecutionPlugin::on_after_run() - tracks task execution performance
- ExecutionPlugin::run_node() - tracks individual node execution

Code Quality

Migrated all logging from log:: macros to tracing:: throughout the codebase
Updated tests to use tracing where appropriate
Fixed all clippy warnings and ensured clean formatting
Maintained backward compatibility with existing functionality

Technical Implementation

Logging Configuration

// Set up log file rotation
let file_appender = tracing_appender::rolling::daily("logs", "bodo.log");
let (non_blocking, _guard) = tracing_appender::non_blocking(file_appender);

// Create layers for console and file output
let console_layer = tracing_subscriber::fmt::layer()...
let file_layer = tracing_subscriber::fmt::layer().json()...

// Initialize with both layers
tracing_subscriber::registry()
    .with(tracing_subscriber::EnvFilter::from_default_env())
    .with(console_layer)
    .with(file_layer)
    .init();

Performance Instrumentation

#[instrument(skip(self, config))]
pub fn build_graph(&mut self, config: BodoConfig) -> Result<&Graph> {
    // Implementation with automatic span tracking
}

Testing

✅ All tests pass (cargo test --all --all-features)
✅ No clippy warnings (cargo clippy --all-targets --all-features -- -D warnings)
✅ Clean formatting (cargo fmt --all --check)
✅ Fixed test isolation issues with environment variables

Success Criteria Met

✅ Logging provides actionable debugging information
✅ Structured logs enable better monitoring (JSON format in files)
✅ Configurable log levels and filtering (--verbose, --quiet, RUST_LOG)
✅ Performance overhead minimized with efficient tracing
✅ Log aggregation for concurrent tasks (structured spans)

Closes #59

- Replace log crate with tracing for structured logging - Add tracing-subscriber with env-filter support - Implement log file rotation using tracing-appender - Add --verbose and --quiet CLI flags for log level control - Add performance metrics with tracing spans on key functions - Replace all log:: macros with tracing:: throughout codebase - Update tests to use tracing where appropriate - Fix clippy warnings and ensure all tests pass Addresses issue #59 - Milestone 1.3: Logging System

greptile-apps

Greptile Summary

This PR implements a comprehensive logging system migration from the log crate to tracing throughout the entire codebase. The changes replace basic logging with structured logging capabilities, adding significant observability features while maintaining backward compatibility.

Core Infrastructure Changes:

Logging Migration: Systematically replaced log:: macros with tracing:: equivalents across all source files (main.rs, process.rs, graph.rs, plugin files, etc.)
Dual Output System: Implemented console logging for user interaction and JSON file logging for structured analysis
Log File Rotation: Added daily log rotation to logs/bodo.log using tracing-appender with non-blocking writes
Dependency Updates: Updated Cargo.toml to replace log + env_logger with tracing, tracing-subscriber, and tracing-appender

CLI Enhancements:

Added --verbose (-v) flag for debug-level logging alongside existing --debug
Added --quiet (-q) flag for error-only logging
Enhanced log level control with priority: verbose/debug > quiet > default info > RUST_LOG environment variable

Performance Monitoring:
The PR adds tracing instrumentation to critical performance paths:

GraphManager::build_graph() - tracks dependency graph construction time
GraphManager::run_plugins() - monitors plugin execution performance
ExecutionPlugin::on_after_run() and run_node() - tracks individual task execution spans

These #[instrument] attributes automatically create spans that capture timing and context information for debugging and monitoring.

Code Quality Improvements:

Fixed numerous clippy warnings through idiomatic Rust improvements (using contains_key() vs get().is_some(), slice parameters vs Vec parameters)
Cleaned up redundant test assertions (assert!(true) statements)
Improved test isolation by fixing environment variable cleanup in watch plugin tests
Updated all test files to include new CLI fields (verbose and quiet) in Args struct initializations

The migration maintains full functional compatibility - all existing logging behavior is preserved while enabling advanced structured logging features for better debugging and operational monitoring.

Confidence score: 4/5

This PR is generally safe to merge but requires attention to the logging infrastructure changes
Score reflects the comprehensive nature of changes across many files, though individual changes are straightforward
Pay close attention to the main.rs logging setup and test environment variable handling

_{45 files reviewed, 1 comment}

_{Edit Code Review Bot Settings | Greptile}

greptile-apps · 2025-09-19T09:28:13Z

tests/watch_plugin_test.rs

+    // Ensure clean state - set to empty string then remove
+    std::env::set_var("BODO_NO_WATCH", "");
    std::env::remove_var("BODO_NO_WATCH");


style: the double operation (set then remove) is redundant - std::env::remove_var alone would suffice

Copilot

Pull Request Overview

This PR implements a comprehensive logging system migration from the log crate to tracing for structured logging and performance monitoring. The changes include CLI enhancements with verbose/quiet flags, dual output to console and JSON log files with daily rotation, and performance instrumentation across key execution paths.

Key Changes:

Migration from log to tracing with structured logging capabilities
Added --verbose and --quiet CLI flags for granular log level control
Implemented dual logging output: console for user interaction, JSON files for analysis

Reviewed Changes

Copilot reviewed 45 out of 46 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Cargo.toml	Updated dependencies replacing log/env_logger with tracing ecosystem
src/main.rs	Implemented tracing-subscriber setup with dual console/file output
src/cli.rs	Added verbose and quiet CLI flags for log level control
src/manager.rs	Added tracing instrumentation to core graph operations
src/plugins/execution_plugin.rs	Added performance tracing spans to execution methods
src/process.rs	Migrated all log macros to tracing equivalents
src/plugins/watch_plugin.rs	Updated logging calls and improved environment variable handling
src/plugins/timeout_plugin.rs	Migrated debug logging to tracing
src/plugins/print_list_plugin.rs	Updated info logging to tracing
src/graph.rs	Migrated debug logging and added clippy allow for large enum variant
src/script_loader.rs	Improved function parameter types using slice references
tests/*	Updated test code to include new CLI fields and fix clippy warnings

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T09:28:20Z

src/plugins/watch_plugin.rs

+                        if std::env::var("BODO_NO_WATCH").map_or(true, |v| v != "1") {
                            self.watch_mode = true;
                            break;


[nitpick] Consider using a more explicit approach for checking environment variables. The current logic with map_or(true, |v| v != "1") could be confusing. A clearer approach would be to explicitly check if the variable is set to "1" or any truthy value.

Suggested change

if std::env::var("BODO_NO_WATCH").map_or(true, |v| v != "1") {

self.watch_mode = true;

break;

match std::env::var("BODO_NO_WATCH") {

Ok(ref v) if v == "1" => { /* do not enable watch mode */ }

_ => {

self.watch_mode = true;

break;

}

Copilot · 2025-09-19T09:28:21Z

src/script_loader.rs

-        script: &Vec<String>,
-        task: &Vec<String>,
-    ) -> Vec<String> {
+    pub fn merge_exec_paths(global: &[String], script: &[String], task: &[String]) -> Vec<String> {


Good improvement changing from &Vec<String> to &[String] for function parameters. This follows Rust best practices by accepting the more general slice type rather than the specific vector type.

Copilot · 2025-09-19T09:28:21Z

tests/watch_plugin_test.rs

+    // Restore original environment state
+    match original_value {
+        Some(value) => std::env::set_var("BODO_NO_WATCH", value),
+        None => std::env::remove_var("BODO_NO_WATCH"),
+    }


Excellent improvement to test isolation by properly restoring the original environment state. This prevents tests from affecting each other when run in parallel.

Copilot · 2025-09-19T09:28:21Z

src/main.rs

+
+    // Set up log file rotation
+    let file_appender = tracing_appender::rolling::daily("logs", "bodo.log");
+    let (non_blocking, _guard) = tracing_appender::non_blocking(file_appender);


The _guard variable is prefixed with underscore but should be properly handled. The guard needs to be kept alive for the duration of the program or logging will stop working. Consider storing it in a static or ensuring it lives for the program duration.

Suggested change

let (non_blocking, _guard) = tracing_appender::non_blocking(file_appender);

let (non_blocking, guard) = tracing_appender::non_blocking(file_appender);

- Keep both verbose/quiet and dry_run CLI flags - Maintain tracing instrumentation on run_node function - Use slice parameters for better clippy compliance - Add dry_run field to all test Args structs - Remove deleted designer test files

- Fix watch plugin environment variable handling with explicit match pattern - Properly scope tracing appender guard to prevent logging from stopping - Update test expectations for find_base_directory function - Fix merge conflict markers in test files - Ensure all tests pass with proper environment variable cleanup - Address code quality improvements from PR reviews

Copilot

Pull Request Overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/main.rs:67

BODO_NO_WATCH semantics here disable watch whenever the variable is set to any value, but WatchPlugin only disables when BODO_NO_WATCH == "1". This inconsistency can lead to watch mode being re-enabled by the plugin when the env var is set to other values (e.g., "0"). Align main to only disable when the value is "1".

    let watch_mode = if std::env::var("BODO_NO_WATCH").is_ok() {
        false
    } else if args.auto_watch {
        true
    } else {
        args.watch
    };

Copilot · 2025-09-19T15:14:40Z

src/main.rs

+    if let Err(e) = run(args, guard) {
+        tracing::error!("Error: {}", e);
        exit(1);
    }
 }

-fn run(args: Args) -> Result<(), BodoError> {
+fn run(args: Args, _guard: tracing_appender::non_blocking::WorkerGuard) -> Result<(), BodoError> {


WorkerGuard is moved into run and dropped before the error log in main, so the file appender may not flush the final error message to the log file. Keep the guard alive in main for the entire program and do not pass it into run.

Copilot · 2025-09-19T15:14:41Z

src/plugins/watch_plugin.rs

+                        // Found auto_watch == true, enable watch mode only if BODO_NO_WATCH is not set to "1"
+                        match std::env::var("BODO_NO_WATCH") {
+                            Ok(ref v) if v == "1" => {
+                                // BODO_NO_WATCH is set to "1", do not enable watch mode
+                            }
+                            _ => {
+                                self.watch_mode = true;
+                                break;
+                            }
                        }


This logic only disables watch when BODO_NO_WATCH == "1" but main currently disables when the var is set to any value, creating conflicting behavior. Update main to mirror this exact check so both places agree.

Copilot · 2025-09-19T15:14:41Z

src/main.rs

+    if let Err(e) = run(args, guard) {
+        tracing::error!("Error: {}", e);
        exit(1);
    }
 }

-fn run(args: Args) -> Result<(), BodoError> {
+fn run(args: Args, _guard: tracing_appender::non_blocking::WorkerGuard) -> Result<(), BodoError> {


"Suggested concrete fix for WorkerGuard lifetime: keep the guard in main and restore the original run signature. For example:

Copilot AI review requested due to automatic review settings September 19, 2025 09:27

greptile-apps bot reviewed Sep 19, 2025

View reviewed changes

Copilot AI reviewed Sep 19, 2025

View reviewed changes

Test User added 2 commits September 19, 2025 11:42

merge: resolve conflicts with main branch

da28389

- Keep both verbose/quiet and dry_run CLI flags - Maintain tracing instrumentation on run_node function - Use slice parameters for better clippy compliance - Add dry_run field to all test Args structs - Remove deleted designer test files

Copilot AI review requested due to automatic review settings September 19, 2025 15:08

Copilot AI reviewed Sep 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: implement comprehensive logging system with tracing #94

feat: implement comprehensive logging system with tracing #94

Uh oh!

mohsen1 commented Sep 19, 2025

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Sep 19, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Uh oh!

	let (non_blocking, _guard) = tracing_appender::non_blocking(file_appender);
	let (non_blocking, guard) = tracing_appender::non_blocking(file_appender);

feat: implement comprehensive logging system with tracing #94

Are you sure you want to change the base?

feat: implement comprehensive logging system with tracing #94

Uh oh!

Conversation

mohsen1 commented Sep 19, 2025

Summary

Changes Made

Core Logging Infrastructure

CLI Enhancements

Performance Monitoring

Code Quality

Technical Implementation

Logging Configuration

Performance Instrumentation

Testing

Success Criteria Met

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Summary

Confidence score: 4/5

Uh oh!

greptile-apps bot Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!