build: add context-based logging infrastructure - #24
Conversation
Summary of ChangesHello @Roasbeef, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's logging infrastructure by introducing a context-based logger propagation mechanism. This change aims to improve code cleanliness and maintainability by allowing loggers to be implicitly passed through Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust context-based logging infrastructure, which is a great step towards better testability and cleaner code by avoiding passing loggers through every function. The implementation of context-based loggers and the build-tag system for controlling log levels are well-executed. I have one suggestion to refactor the NewSubLogger function in build/log.go to improve its clarity, remove some code duplication, and make it more robust by handling a potential error. Overall, this is a solid contribution that improves the project's logging capabilities.
aa4c8f3 to
411b1d8
Compare
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
Nice! LGTM, pending linter fixes 🎉
This commit introduces the core logging infrastructure ported from lnd's build package. The LogType enum defines the three logging modes supported: LogTypeNone (no logging), LogTypeStdOut (stdout only), and LogTypeDefault (stdout plus optional log rotator). The NewSubLogger function serves as a factory for creating subsystem loggers. It examines the current deployment mode (Production vs Development) and logging type to determine how to construct the logger. For production builds, it delegates to an optional generator function. For development builds with stdlog enabled, it creates a stdout logger with the build-tag-configured log level. This design allows tests to easily enable logging while keeping logs disabled by default.
This commit adds eight files that use Go build tags to control the default log level at compile time. The mechanism allows selecting different verbosity levels without runtime configuration changes. The loglevel_default.go file activates when no specific level tag is set, providing "info" as the default. The remaining files each handle their respective level: trace, debug, info, warn, error, critical, and off. Each level-specific file requires both the "dev" tag and its level tag (e.g., "dev && trace"). This pattern, borrowed from lnd, enables testing with verbose output by simply adding build tags: "go test -tags='dev stdlog trace' ./..." will enable trace-level logging to stdout.
This commit adds three files that control where log output is directed based on build tags. The log_default.go file sets LoggingType to LogTypeDefault when neither stdlog nor nolog tags are present, suitable for production daemon use where logs go to both stdout and a log file rotator. The log_stdlog.go file activates with the "stdlog" tag and directs all output to stdout only, which is ideal for unit tests where you want to see log output in the test output. The log_nolog.go file activates with the "nolog" tag and disables all logging entirely, which is the default for tests to avoid noise.
This commit introduces utilities for passing loggers through the call stack via context.Context, providing a middle ground between global loggers and explicit parameter passing. The ContextWithLogger function attaches a logger to a context, while LoggerFromContext extracts it. If no logger is present, it returns btclog.Disabled which safely no-ops all log calls, making it safe to use without nil checks. A MustLoggerFromContext variant is also provided for code paths where a logger must be present, panicking if one is not found. This approach reduces the need to thread loggers through every function parameter while maintaining explicit control over which logger is used. The pattern works well with the existing per-instance logging from PR #7, allowing subsystem loggers to be attached to request contexts.
This commit adds a "log" parameter to the Makefile that allows overriding the default LOG_TAGS value. By default, tests run with "nolog" to keep output clean, but developers can enable logging for debugging with: make unit log="stdlog trace" pkg=db The parameter accepts any combination of log type and level tags, making it easy to toggle verbose logging during test development without modifying the Makefile itself.
411b1d8 to
7240858
Compare
Update CLAUDE.md and AGENTS.md to document the log= parameter for enabling debug logging during unit tests. This replaces the previously documented unit-debug target with the more flexible parameter approach.
7240858 to
d7cf785
Compare
PR Review: Context-Based Logging InfrastructureSummaryThis PR introduces a well-designed context-based logging system ported from lnd, providing a practical middle ground between global loggers and explicit parameter passing. The implementation is solid overall with good test coverage. ✅ StrengthsArchitecture & Design
Code Quality
Makefile Integration
🔍 Issues Found1. Typo in log.go:9 comment// LogType is an indicating the type of logging specified by the build flag.Should be: "LogType is an enum indicating..." or "LogType indicates..." Severity: Low (documentation clarity) 2. Unused code: Log compressor utilities (lines 37-61)The constants Reasoning: Since this PR focuses on context-based logging and test log control, the compressor code appears to be forward-looking infrastructure. If it's not immediately needed:
Severity: Medium (code cleanliness) 3. Error ignored in log.go:101level, _ := btclog.LevelFromString(LogLevel)If Recommendation: Add error handling or at least a comment explaining why errors are safe to ignore (e.g., if LogLevel is guaranteed valid by build tags). level, err := btclog.LevelFromString(LogLevel)
if err != nil {
// LogLevel is guaranteed valid by build tags, but log if something
// goes wrong in case build system is misconfigured.
panic(fmt.Sprintf("invalid log level from build tags: %v", err))
}Severity: Medium (potential runtime issues) 4. Missing test: NewSubLogger behaviorWhile
Severity: Low (test coverage gap) 🔒 SecurityNo security concerns identified. Context keys are properly isolated and logger propagation is safe. ⚡ PerformanceNo performance concerns. Context value lookups are O(1) and logger creation is lightweight. 📋 RecommendationsMust Fix
Should Consider
Minor Improvements
📝 Commit QualityCommits follow project guidelines well:
VerdictLGTM with minor fixes recommended ✅ The PR delivers solid infrastructure that will improve codebase ergonomics. The issues identified are minor and don't block merging, but addressing them would increase code quality. The architecture is sound and follows lnd patterns successfully. |
PR Review: Context-Based Logging InfrastructureSummaryThis PR introduces context-based logging utilities and a build-tag system for controlling log levels during development. The implementation is well-designed and follows Go best practices. Overall, this is a solid addition that will improve testability and debugging workflows. Strengths
Issues and Concerns1. Comment Typo in build/log.go:9
2. Unused Variables in build/log.go:39-60
3. Ignored Error in build/log.go:101
4. Makefile: LOG_TAGS Default Value
Suggestions for Improvement
Security and Performance
Checklist Before Merge
Overall AssessmentVerdict: Approve with Minor Changes This is a well-implemented feature that aligns with the project's goals. The code quality is high, test coverage is excellent, and the design is idiomatic. The identified issues are minor and easily addressed. Once the typo is fixed and the unused code is resolved, this is ready to merge. Great work! |
3 participants
PR #7 moved us away from global logging toward per-instance loggers, which is the right direction for testability and avoiding shared mutable state. However, threading a logger through every function parameter can become tedious, especially in deep call stacks where most functions just pass the logger along without using it.
This PR introduces a compromise: context-based logging utilities that let us attach a logger to a context and extract it later. Functions that already take a context can access the logger without additional parameters, while still maintaining explicit control over which logger instance is used. The pattern works naturally with request-scoped contexts, where each request can carry its own logger with relevant metadata.
The PR also ports lnd's build-tag system for controlling log levels and output destinations at compile time. This makes it easy to enable verbose logging during test debugging with
make unit log="stdlog trace" pkg=dbwhile keeping tests quiet by default.