set the log level from the config file #92
Conversation
WalkthroughConfig gains a new LogLevel field and the logger Init function now accepts and applies a log level from configuration; logger initialization reads and parses that level and sets the logger accordingly, fatalling on parse error. Changes
Sequence DiagramsequenceDiagram
participant Config
participant Logger as Logger.Init
participant LogLib as log package
Config->>Config: load config file
Config->>Config: read `LogLevel`
Config->>Logger: Init(path, LogLevel)
alt parse succeeds
Logger->>LogLib: ParseLevel(level)
Logger->>LogLib: SetLevel(parsedLevel)
Logger->>LogLib: SetOutput(file)
else parse fails
Logger->>Logger: Fatal(error)
end
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: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
openhue/config.go (1)
76-77: Consider persistingLogLevelin the config file.The
Save()function currently only persistsBridgeandKeybut notLogLevel. If users need to set the log level programmatically (e.g., via a setup command), it won't be saved to the configuration file.If this is intentional (log level only set via manual config editing), this can be ignored. Otherwise, apply this diff:
viper.Set("Bridge", c.Bridge) viper.Set("Key", c.Key) + if c.LogLevel != "" { + viper.Set("log_level", c.LogLevel) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openhue/config.go(2 hunks)util/logger/logger.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openhue/config.go (1)
util/logger/logger.go (1)
Init(8-26)
| c.LogLevel = viper.GetString("log_level") | ||
| logger.Init(filepath.Join(configPath, "openhue.log"), c.LogLevel) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider providing a default log level.
If log_level is not present in the config file, viper.GetString returns an empty string, which will cause log.ParseLevel in logger.Init to fail and terminate the application. Consider setting a sensible default (e.g., "info") to prevent fatal errors when the field is missing.
Apply this diff to add a default log level:
c.Bridge = viper.GetString("Bridge")
c.Key = viper.GetString("Key")
c.LogLevel = viper.GetString("log_level")
+ if c.LogLevel == "" {
+ c.LogLevel = "info"
+ }
logger.Init(filepath.Join(configPath, "openhue.log"), c.LogLevel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.LogLevel = viper.GetString("log_level") | |
| logger.Init(filepath.Join(configPath, "openhue.log"), c.LogLevel) | |
| c.Bridge = viper.GetString("Bridge") | |
| c.Key = viper.GetString("Key") | |
| c.LogLevel = viper.GetString("log_level") | |
| if c.LogLevel == "" { | |
| c.LogLevel = "info" | |
| } | |
| logger.Init(filepath.Join(configPath, "openhue.log"), c.LogLevel) |
🤖 Prompt for AI Agents
In openhue/config.go around lines 62-63, the code reads c.LogLevel =
viper.GetString("log_level") and passes it to logger.Init which will fail if the
config key is missing; change this to provide a sensible default (e.g., "info")
by either calling viper.SetDefault("log_level", "info") before reading, or by
checking the returned string and falling back: c.LogLevel =
viper.GetString("log_level"); if c.LogLevel == "" { c.LogLevel = "info" } and
then call logger.Init with that value.
| logLevel, err := log.ParseLevel(level) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| level = log.InfoLevel.String() | ||
| } | ||
| log.SetLevel(logLevel) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify behavior when level is an empty string.
If the LogLevel field is not set in the config file, an empty string will be passed to log.ParseLevel. This could cause the application to terminate with a fatal error.
Run the following script to check how logrus handles empty string parsing:
🏁 Script executed:
#!/bin/bash
# Description: Check logrus ParseLevel behavior with empty string
cat > /tmp/test_loglevel.go << 'EOF'
package main
import (
"fmt"
log "github.com/sirupsen/logrus"
)
func main() {
// Test empty string
level, err := log.ParseLevel("")
if err != nil {
fmt.Printf("Empty string error: %v\n", err)
} else {
fmt.Printf("Empty string parsed as: %v\n", level)
}
// Test valid levels
for _, l := range []string{"trace", "debug", "info", "warn", "error", "fatal", "panic"} {
level, err := log.ParseLevel(l)
if err != nil {
fmt.Printf("%s error: %v\n", l, err)
} else {
fmt.Printf("%s parsed as: %v\n", l, level)
}
}
}
EOF
cd /tmp
go mod init test 2>/dev/null || true
go get github.com/sirupsen/[email protected]
go run test_loglevel.goLength of output: 635
I need to gather more information about how logrus handles empty strings in ParseLevel. Let me search for this and also examine the codebase context.
Now let me examine the codebase to understand the actual context of this logger code:
🏁 Script executed:
#!/bin/bash
# Find and examine the logger file to understand context
fd -t f logger.go | head -20Length of output: 21
🏁 Script executed:
#!/bin/bash
# Look for the specific file mentioned in the review
find . -name "logger.go" -path "*/util/logger/*" 2>/dev/nullLength of output: 23
🏁 Script executed:
#!/bin/bash
# Search for the code snippet in the review to find the exact context
rg -n "logLevel, err := log.ParseLevel" --type=go -B5 -A10Length of output: 763
🏁 Script executed:
#!/bin/bash
# Check if there's config loading and how empty strings are handled
rg -n "LogLevel" --type=go -B2 -A2 | head -50Length of output: 481
🏁 Script executed:
#!/bin/bash
# Look for yaml/config tag handling to understand field initialization
rg -n "yaml\|toml\|json" --type=go -B1 -A1 | grep -i "loglevel\|log.level" -B2 -A2Length of output: 0
Fix unreachable fallback logic and handle empty log level gracefully.
The issue is valid and critical for startup reliability. When the log_level config key is not set, viper.GetString("log_level") returns an empty string, which is passed to log.ParseLevel(). This causes the function to fail and call log.Fatal(err), terminating the application. The reassignment on line 19 (level = log.InfoLevel.String()) is unreachable dead code.
To fix this:
- Check if
levelis empty before parsing, and set a sensible default (e.g.,info) - Or move the reassignment before
log.Fatal()so it executes if parsing fails
Example:
logLevel, err := log.ParseLevel(level)
if err != nil {
level = log.InfoLevel.String()
logLevel, _ = log.ParseLevel(level)
}
log.SetLevel(logLevel)🤖 Prompt for AI Agents
In util/logger/logger.go around lines 16 to 21, the current logic calls
log.ParseLevel on a possibly empty string and then log.Fatal on error, making
the fallback assignment unreachable; change it so you first set a sensible
default when the config value is empty (e.g., "info"), then parse, and if
ParseLevel still returns an error do not call log.Fatal but fall back to info by
reassigning level and reparsing (or directly set log.InfoLevel) before calling
log.SetLevel; ensure no unreachable code remains and the application does not
exit on a bad or empty log level.
1 participant
I use openhue-cli inside a script for polybar and I have lots of accessories that are not lights so the log is growing to much.
With this simple change, the user can choose the log level he want from the config file
Log levels are strings from Logrus
Summary by CodeRabbit