Enhanced logging with additional levels and distinct log file format #3976
Conversation
User can now choose None, Error, Info, or Debug.
|
🥷 Code experts: Jack251970, onesounds Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughExpand log-level enum, change SetLogLevel to map public levels to NLog via the file rule and reconfigure existing loggers, rename log file pattern to include date with .log extension, add localization keys for NONE and ERROR, and insert a BOM in Settings.cs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Settings UI
participant Settings as UserSettings
participant Log as Log.SetLogLevel
participant NLog as NLog Config
User->>UI: select log level (NONE/ERROR/INFO/DEBUG)
UI->>Settings: persist selected level
Settings->>Log: SetLogLevel(selected)
Log->>NLog: FindRuleByName("file")
Log->>NLog: Map public LOGLEVEL -> NLog level (Off/Error/Info/Debug)
Log->>NLog: rule.SetLoggingLevels(mappedLevel, LogLevel.Fatal)
Log->>NLog: LogManager.ReconfigExistingLoggers()
Note right of Log: Emit Debug message about the selected level
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests
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 Pre-merge checks❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
70-86: Harden SetLogLevel: null-check rule, align DEBUG target, fix logger name
- Add a null-check for the "file" rule to avoid NRE if config changes.
- In DEBUG builds, mirror the same level onto the debug target rule so developer output respects the selected level.
- Use nameof(Log) instead of nameof(Logger) to log the correct class name.
Apply:
public static void SetLogLevel(LOGLEVEL level) { - var rule = LogManager.Configuration.FindRuleByName("file"); + var rule = LogManager.Configuration.FindRuleByName("file"); + if (rule is null) + { + Warn(nameof(Log), "NLog rule 'file' not found. Skipping log-level change."); + return; + } var nlogLevel = level switch { LOGLEVEL.None => LogLevel.Off, LOGLEVEL.Error => LogLevel.Error, LOGLEVEL.Debug => LogLevel.Debug, _ => LogLevel.Info }; rule.SetLoggingLevels(nlogLevel, LogLevel.Fatal); - LogManager.ReconfigExistingLoggers(); + #if DEBUG + var debugRule = LogManager.Configuration.FindRuleByName("debug"); + debugRule?.SetLoggingLevels(nlogLevel, LogLevel.Fatal); + #endif + + LogManager.ReconfigExistingLoggers(); - // We can't log Info when level is set to Error or None, so we use Debug - Debug(nameof(Logger), $"Using log level: {level}."); + // Keep noise low at Error/None; this will only emit when allowed by current level. + Debug(nameof(Log), $"Using log level: {level}."); }Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
343-343: Default log level → Info — confirm backwards compatibility with old saved valuesLGTM. Please confirm existing user settings containing "DEBUG"/"INFO" (old enum names) deserialize to the new values (usually case-insensitive with JsonStringEnumConverter). If not, we’ll need a one-time migration or a tolerant converter.
You can validate with a quick unit test or by loading a settings JSON containing
"LogLevel": "DEBUG"and"LogLevel": "INFO".I can add a small deserialization test or a migration shim if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/Logger/Log.cs(5 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Applied to files:
Flow.Launcher/Languages/en.xaml
🧬 Code graph analysis (2)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
Info(157-160)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
Flow.Launcher.Infrastructure/Stopwatch.cs (2)
Debug(13-22)Info(38-47)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Infrastructure/Logger/Log.cs
[warning] 37-37:
MMdd is not a recognized word. (unrecognized-spelling)
[warning] 80-80:
nlog is not a recognized word. (unrecognized-spelling)
⏰ 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: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher/Languages/en.xaml (1)
462-462: Whitespace-only change — OKNo functional change.
Flow.Launcher.Infrastructure/Logger/Log.cs (2)
170-174: Approve — enum expansion OKrg search of C# files for '\b(DEBUG|INFO)\b' found only preprocessor
#if DEBUGoccurrences (noINFOmatches or runtime "DEBUG"/"INFO" string checks).
35-39: Log filename changed — confirm cleanup/rotation still matchesRepo search shows only the new FileTarget in Flow.Launcher.Infrastructure/Logger/Log.cs:
FileName = CurrentLogDirectory.Replace(@"", "/") + "/Flow.Launcher.${date:format=yyyyMMdd}.log"
No other .log or ${shortdate} occurrences were found; .txt hits are NativeMethods.txt and NeedDelete.txt (unrelated). Verify any external packaging/installer/CI or Clear Logs code references .log (not .txt) and adjust/add rotation/archival (e.g., MaxArchiveFiles / ArchiveEvery) as needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/Logger/Log.cs (2)
37-39: Prefer Path.Combine and verify consumers of new .log extensionUse Path.Combine instead of Replace + string concat. Also verify any tooling/docs expecting .txt now handle .log.
- FileName = CurrentLogDirectory.Replace(@"\", "/") + "/Flow.Launcher.${date:format=yyyyMMdd}.log", + FileName = Path.Combine(CurrentLogDirectory, "Flow.Launcher.${date:format=yyyyMMdd}.log"),
68-84: Make SetLogLevel null/compat‑safe and explicit about INFO; fix logger name
- Adds fallback if FindRuleByName is unavailable/returns null.
- Maps INFO explicitly.
- Uses nameof(Log) to avoid confusion with NLog.Logger.
- var rule = LogManager.Configuration.FindRuleByName("file"); - - var nlogLevel = level switch - { - LOGLEVEL.NONE => LogLevel.Off, - LOGLEVEL.ERROR => LogLevel.Error, - LOGLEVEL.DEBUG => LogLevel.Debug, - _ => LogLevel.Info - }; - - rule.SetLoggingLevels(nlogLevel, LogLevel.Fatal); - - LogManager.ReconfigExistingLoggers(); - - // We can't log Info when level is set to Error or None, so we use Debug - Debug(nameof(Logger), $"Using log level: {level}."); + var nlogLevel = level switch + { + LOGLEVEL.NONE => LogLevel.Off, + LOGLEVEL.ERROR => LogLevel.Error, + LOGLEVEL.INFO => LogLevel.Info, + LOGLEVEL.DEBUG => LogLevel.Debug, + _ => LogLevel.Info + }; + + var config = LogManager.Configuration; + if (config is null) + return; + + var rule = config.FindRuleByName("file"); + if (rule is null) + { + // Fallback for older NLog or missing RuleName + foreach (var r in config.LoggingRules) + { + if (r.RuleName == "file") { rule = r; break; } + } + } + if (rule is null) + return; + + rule.SetLoggingLevels(nlogLevel, LogLevel.Fatal); + LogManager.ReconfigExistingLoggers(); + + // We can't log Info when level is set to Error or None, so we use Debug + Debug(nameof(Log), $"Using log level: {level}.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/Logger/Log.cs(3 hunks)Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Flow.Launcher.Infrastructure/UserSettings/Settings.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Languages/en.xaml
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
Flow.Launcher.Infrastructure/Stopwatch.cs (2)
Debug(13-22)Info(38-47)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Infrastructure/Logger/Log.cs
[warning] 37-37:
MMdd is not a recognized word. (unrecognized-spelling)
[warning] 78-78:
nlog is not a recognized word. (unrecognized-spelling)
⏰ 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: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher.Infrastructure/Logger/Log.cs (3)
68-84: Confirm intent: user level only applies to file targetIn DEBUG builds the OutputDebugString target remains at Debug; user setting won’t affect it. Confirm this is desired UX.
70-76: Double‑check NONE→Off behavior with SetLoggingLevelsNLog should produce no output when min=Off and max=Fatal, but please verify at runtime that NONE truly silences the file logs.
168-171: Enum expansion impact checkEnsure settings persistence, defaults, and localization bindings handle NONE/ERROR correctly (migrations, UI selection, CLI/env if any).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/Logger/Log.cs (2)
37-37: Prefer Path.Combine; consider retention policy.
- Build the path without manual slash replacement.
- With per-day filenames, old logs will accumulate indefinitely. Consider NLog retention (ArchiveEvery/MaxArchiveFiles) or a pruning job.
- FileName = CurrentLogDirectory.Replace(@"\", "/") + "/Flow.Launcher.${date:format=yyyy-MM-dd}.log", + FileName = Path.Combine(CurrentLogDirectory, "Flow.Launcher.${date:format=yyyy-MM-dd}.log"),Alternative (keeps a constant file and lets NLog archive daily, auto-cleaning):
- FileName = Path.Combine(CurrentLogDirectory, "Flow.Launcher.${date:format=yyyy-MM-dd}.log"), + FileName = Path.Combine(CurrentLogDirectory, "Flow.Launcher.log"), + ArchiveEvery = FileArchivePeriod.Day, + ArchiveNumbering = ArchiveNumberingMode.Date, + ArchiveDateFormat = "yyyy-MM-dd", + MaxArchiveFiles = 30,
70-70: Nit: variable name trips spelling checks.Rename nlogLevel → nLogLevel or selectedNLogLevel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Logger/Log.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
Flow.Launcher.Infrastructure/Stopwatch.cs (2)
Debug(13-22)Info(38-47)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Infrastructure/Logger/Log.cs
[warning] 70-70:
nlog is not a recognized word. (unrecognized-spelling)
[warning] 78-78:
nlog is not a recognized word. (unrecognized-spelling)
⏰ 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). (6)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Infrastructure/Logger/Log.cs (1)
168-171: No migration risk — LogLevel persisted as stringSettings.LogLevel is annotated with [JsonConverter(typeof(JsonStringEnumConverter))], so stored values are enum names (strings) and reordering the enum will not remap persisted settings. (Flow.Launcher.Infrastructure/UserSettings/Settings.cs:343)
|
@dcog989 Thanks for your contribution! |
|
@Jack251970 - my pleasure. Thanks for your tidy / enhancements to the PR. :) |
Enhanced logging
jjw24
changed the title
Enhance logging
Flow.Launcher.${date:format=yyyy-MM-dd}.logas log file format.Fixes #3812