Skip to content

feat!: FFI logging options in Go open config; remove StartLogs#1535

Draft
rkuris wants to merge 4 commits intomainfrom
rkuris/logging-subsystem-improvements
Draft

feat!: FFI logging options in Go open config; remove StartLogs#1535
rkuris wants to merge 4 commits intomainfrom
rkuris/logging-subsystem-improvements

Conversation

@rkuris
Copy link
Member

@rkuris rkuris commented Dec 11, 2025

Adds WithLogPath and WithLogFilter functional options to initialize env_logger via FFI at database open time.

  • Full RUST_LOG filter support via env_logger::Builder::parse_filters()
  • Log path required; use /dev/stdout for stdout; no defaults
  • Logging initialized before DB open; remains initialized even if open fails
  • Global per-process logger; fails when already initialized
  • Remove deprecated StartLogs and LogConfig from Go; keep fwd_start_logs called internally
  • Add trace log on Db::new() and comprehensive TestLogging covering error/success cases

BREAKING CHANGE: StartLogs API removed; use WithLogPath/WithLogFilter options on New()

Why this should be merged

  • We need this immediately to help with debugging the rust side
  • Logging must be configured at open to enable future per-database log strategies.
  • Aligns Go-side configuration with Rust env_logger semantics using RUST_LOG.
  • Unblocks downstream integration (e.g., coreth) to set logging level and output programmatically.

Now we support any logging option as configured from the go side, which means we probably need some configuration options in coreth to set up logging the way we want.

How this works

  • Go: New(path, WithLogPath("/dev/stdout"), WithLogFilter("info")) initializes logging before DB open.
  • Rust FFI: LogArgs validates UTF-8; env_logger::Builder parses RUST_LOG filters and targets the provided path.
  • Logging is global per-process; subsequent init attempts return an error.
  • If DB open fails, logger remains initialized (explicitly documented).

How this was tested

  • TestLogging (Go):
  • Empty path: logging disabled, DB opens
  • Invalid path: initialization fails with clear error
  • Valid path + trace: log file created and non-empty; contains “Opening Firewood database …”
  • Second init: fails with “attempted to set a logger after the logging system was already initialized”

Workspace checks

  • cargo fmt
  • cargo test --workspace --features ethhash,logger --all-targets
  • cargo clippy --workspace --features ethhash,logger --all-targets -- -D warnings
  • cargo doc --no-deps

Adds WithLogPath and WithLogFilter functional options to initialize env_logger via FFI at database open time.
- Full `RUST_LOG` filter support via env_logger::Builder::parse_filters()
- Log path required; use /dev/stdout for stdout; no defaults
- Logging initialized before DB open; remains initialized even if open fails
- Global per-process logger; fails when already initialized
- Remove deprecated StartLogs and LogConfig from Go; keep fwd_start_logs called internally
- Add trace log on Db::new() and comprehensive TestLogging covering error/success cases

BREAKING CHANGE: StartLogs API removed; use WithLogPath/WithLogFilter options on New()
@rkuris rkuris requested a review from RodrigoVillar December 11, 2025 23:32
@rkuris rkuris self-assigned this Dec 11, 2025
@rkuris rkuris added enhancement New feature or request go Pull requests that update go code vibecoded labels Dec 11, 2025
Copy link
Contributor

@alarso16 alarso16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do this until logs are supported per-database. Otherwise, we can't even make multiple databases in tests (since coreth Firewood doesn't know it's a test at instantiation). If this is added for the next release, I think I would just have to remove logging all together

/// The file path where logs for this process are stored.
///
/// If empty, this is set to `${TMPDIR}/firewood-log.txt`.
/// This is required and must not be empty. Use "/dev/stdout" for stdout logging.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't recommend use of /dev/stdout as it will cause interleaving of the stdout from the parent process -- even if the parent process is also implemented Rust sharing the same stdlib statics.

  • Any synchronization in Go around printing to stdout will be bypassed.
  • Because File::open("/dev/stdout") returns a new fd, any synchronization in Rust will also be bypassed.

.filter_level(level)
.target(Pipe(Box::new(file)))
let mut builder = env_logger::Builder::new();
builder.target(Pipe(Box::new(file)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should wrap file with std::io::BufWriter to make using stdout slightly more copacetic (doesn't prevent interleaving but makes it harder if we only write whole lines). env_logger calls flush after writing out every event.

@rkuris
Copy link
Member Author

rkuris commented Dec 15, 2025

I don't think we can do this until logs are supported per-database. Otherwise, we can't even make multiple databases in tests (since coreth Firewood doesn't know it's a test at instantiation). If this is added for the next release, I think I would just have to remove logging all together

We could still do this. If the database open fails because the logger is already initialized, just create the database with no logging. That would be a workaround until we implement per-database logging correctly.

@rkuris rkuris marked this pull request as draft December 15, 2025 17:17
- Resolved conflicts in ffi/firewood.go (combined rootStore bool with logPath/logFilter)
- Resolved conflicts in firewood/src/db.rs (changed db_path to db_dir, kept trace logging)
- Updated DbConfig to use root_store: bool instead of root_store_dir: Option<PathBuf>
- Updated all test helper methods to use new_with_config pattern
@rkuris rkuris force-pushed the rkuris/logging-subsystem-improvements branch from ca9aa37 to a571bfb Compare December 15, 2025 18:21
- Resolved conflicts in ffi/firewood.go: Combined rootStore bool with logPath/logFilter from feature branch
- Resolved conflicts in firewood/src/db.rs: Kept trace! logging from feature branch
- Pulled in new changes from main including nextest config, CI workflow updates, and documentation improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request go Pull requests that update go code vibecoded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants