-
Notifications
You must be signed in to change notification settings - Fork 0
feat(logger): add IsLevelEnabled and convenience level check methods #147
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
Conversation
Add level checking capabilities to the Logger interface: - IsLevelEnabled(level LogLevel) bool - generic level check - IsTraceEnabled(), IsDebugEnabled(), IsInfoEnabled(), IsWarnEnabled(), IsErrorEnabled() - convenience methods Implemented in all logger types: consoleLogger, jsonLogger, otelLogger, TestLogger. Enables fast-path optimization to avoid expensive operations when logging is disabled.
WalkthroughThe PR adds level-enablement query methods to the Logger interface and its implementations, plus a unit test for console logger checks and a small test change in DNS resolv conf tests. No existing logging output behavior is modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logger/otel.go (1)
218-222: Critical bug:levelsparameter ignored.The function accepts a
levelsparameter but always initializes the logger withLevelTrace, ignoring the provided value. This prevents callers from configuring the log level for the otel logger.🔎 Proposed fix
func NewOtelLogger(otelsLogger otelLog.Logger, levels LogLevel) Logger { return &otelLogger{ otelLogger: otelsLogger, - logLevel: LevelTrace, + logLevel: levels, } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
logger/console.gologger/console_test.gologger/init.gologger/json.gologger/otel.gologger/test.go
🧰 Additional context used
🧬 Code graph analysis (5)
logger/test.go (2)
logger/init.go (1)
LogLevel(12-12)env/env.go (1)
LogLevel(414-427)
logger/json.go (2)
logger/init.go (6)
LogLevel(12-12)LevelTrace(15-15)LevelDebug(16-16)LevelInfo(17-17)LevelWarn(18-18)LevelError(19-19)env/env.go (1)
LogLevel(414-427)
logger/init.go (1)
env/env.go (1)
LogLevel(414-427)
logger/console.go (2)
logger/init.go (6)
LogLevel(12-12)LevelTrace(15-15)LevelDebug(16-16)LevelInfo(17-17)LevelWarn(18-18)LevelError(19-19)env/env.go (1)
LogLevel(414-427)
logger/otel.go (2)
logger/init.go (6)
LogLevel(12-12)LevelTrace(15-15)LevelDebug(16-16)LevelInfo(17-17)LevelWarn(18-18)LevelError(19-19)env/env.go (1)
LogLevel(414-427)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
logger/console_test.go (1)
322-396: LGTM! Comprehensive test coverage.The table-driven test thoroughly validates the new level-enablement API across all log levels, checking both the generic
IsLevelEnabledmethod and all convenience wrappers.logger/json.go (1)
214-236: LGTM! Correct dual-threshold implementation.The
IsLevelEnabledmethod correctly accounts for both console and sink log levels, matching the behavior of the existingLogmethod. The convenience wrappers appropriately delegate to the generic method.logger/test.go (1)
122-144: LGTM! Appropriate test fixture behavior.Returning
truefor all level checks is the correct approach for a test fixture, ensuring the test logger doesn't interfere with test logic that relies on level enablement.logger/init.go (1)
66-77: LGTM! Clean interface additions.The new methods are well-documented and provide both a generic
IsLevelEnabledcheck and convenient per-level wrappers, enhancing the API ergonomics without breaking existing code.logger/otel.go (1)
194-216: LGTM! Consistent implementation.The
IsLevelEnabledmethod correctly checks only the logger's threshold (no sink logic), which matches the behavior of thelogmethod at line 111. The convenience wrappers properly delegate to the generic method.logger/console.go (1)
244-266: LGTM! Correct dual-threshold implementation.The
IsLevelEnabledmethod correctly accounts for both console and sink log levels, matching the behavior of the existingLogmethod at line 155. The convenience wrappers appropriately delegate to the generic method.
Summary
Add level checking capabilities to the Logger interface to allow programs to optimize fast-path checks.
Changes
Interface additions:
IsLevelEnabled(level LogLevel) bool- generic level checkIsTraceEnabled(),IsDebugEnabled(),IsInfoEnabled(),IsWarnEnabled(),IsErrorEnabled()- convenience methodsImplementations updated:
consoleLoggerjsonLoggerotelLoggerTestLogger(always returns true for testing)Usage
Enables fast-path optimization to avoid expensive operations when logging is disabled:
Testing
Added
TestConsoleLoggerIsLevelEnabledtest covering all log levels.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.