Check debug level before logging debug & Expose LOGLEVEL API for performance optimization #4131
Conversation
Enhanced logging with conditional debug checks based on log level, ensuring logs are only generated in DEBUG mode. Integrated `CommunityToolkit.Mvvm.DependencyInjection` for dependency injection, improving modularity and testability. - Replaced direct settings access with `FlowSettings` via DI. - Updated logging in `PluginManager.cs`, `PluginsLoader.cs`, `MainViewModel.cs`, and `ResultsViewModel.cs`. - Improved plugin initialization error handling with conditional logging. - Refactored query and result logging for better performance. - Added necessary namespace imports for DI and logging. - Cleaned up redundant logging and improved code readability.
prlabeler
bot
added
bug
Jack251970
changed the title
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces many unconditional debug logs with checks that only emit when the configured log level is DEBUG, introduces a LOGLEVEL enum in plugin namespace, exposes current log level via IPublicAPI/PublicAPIInstance, and adds a public Log.LogLevel property with internal mapping to NLog levels. No control-flow or functional behavior changes beyond conditionalized logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to enhance logging performance by adding conditional checks before debug log calls and introducing dependency injection for the Settings class. However, the implementation has a fundamental flaw: the conditional checks occur after string interpolation has already been evaluated, meaning the performance optimization goal is not achieved. Additionally, NLog already provides built-in log level filtering through Log.SetLogLevel(), making these manual checks redundant.
Key changes:
- Added conditional
if (Settings.LogLevel == LOGLEVEL.DEBUG)checks beforeLogDebugcalls in multiple files - Introduced dependency injection using
CommunityToolkit.Mvvm.DependencyInjectionto injectSettings/FlowSettings - Added
Flow.Launcher.Infrastructure.Loggernamespace imports
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/ResultsViewModel.cs | Added conditional check before one LogDebug call, but string interpolation happens before the check |
| Flow.Launcher/ViewModel/MainViewModel.cs | Added conditional checks before 6 LogDebug calls (inconsistent - other LogDebug calls in same file remain unconditional), string interpolation occurs before checks |
| Flow.Launcher.Core/Plugin/PluginsLoader.cs | Added DI for FlowSettings and conditional check before LogDebug call, but interpolation happens before check |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Added DI for FlowSettings and conditional checks before 2 LogDebug calls, but interpolation happens before checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Updated logging system to conditionally execute debug-level logs only when the log level is set to `DEBUG`, reducing performance overhead in production. Introduced a `LogLevel` property in the `Log` class to dynamically track and configure logging levels. Refactored logging rules to use `NLog.LogLevel` for better compatibility and maintainability. Key changes: - Added conditional checks for `Log.LogLevel` in debug logs across multiple files (`DialogJump.cs`, `WindowsDialog.cs`, `Http.cs`, etc.). - Updated `SetLogLevel` to dynamically adjust NLog configurations. - Streamlined logging method calls to use `NLog.LogLevel` consistently. - Improved exception handling in `Http.cs` and optimized `Stopwatch` utility for debug-level logging. - Minor formatting adjustments and bug fixes in logging rule initialization. These changes enhance the logging system's efficiency, maintainability, and clarity.
Replaced `FlowSettings.LogLevel` with `PublicApi.Instance.GetLogLevel()` for dynamic log level retrieval. Introduced a centralized `LOGLEVEL` enum in the `Flow.Launcher.Plugin` namespace to standardize log level definitions. Added `GetLogLevel()` to the `IPublicAPI` interface for plugin access. Removed the dependency on `CommunityToolkit.Mvvm.DependencyInjection` and unused namespaces across multiple files. Updated `PublicAPIInstance` to use a `Lock` type for `_saveSettingsLock`. Introduced a new `LogLevel.cs` file for the `LOGLEVEL` enum and removed the old enum from `Flow.Launcher.Infrastructure.Logger`. Improved modularity, maintainability, and extensibility of the logging system while streamlining the codebase.
|
🥷 Code experts: taooceros Jack251970, taooceros have 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:
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:
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:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Jack251970
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
Flow.Launcher.Core/Plugin/PluginsLoader.cs (1)
111-112: String interpolation defeats the conditional check optimization.The string interpolation
$"Constructor cost for <{metadata.Name}> is <{metadata.InitTime}ms>"occurs before the conditional check, so the performance cost of formatting is already paid regardless of the log level.Apply this diff to move the interpolation inside the conditional:
- if (PublicApi.Instance.GetLogLevel() == LOGLEVEL.DEBUG) - PublicApi.Instance.LogDebug(ClassName, $"Constructor cost for <{metadata.Name}> is <{metadata.InitTime}ms>"); + if (PublicApi.Instance.GetLogLevel() == LOGLEVEL.DEBUG) + { + PublicApi.Instance.LogDebug(ClassName, $"Constructor cost for <{metadata.Name}> is <{metadata.InitTime}ms>"); + }Note: This is the same issue flagged in the past review comment from Copilot.
Flow.Launcher.Infrastructure/Http/Http.cs (4)
163-164: String interpolation defeats the conditional check optimization.Same issue as line 150-152: string interpolation happens before the conditional check.
195-196: String interpolation defeats the conditional check optimization.Same issue: string interpolation happens before the conditional check.
207-208: String interpolation defeats the conditional check optimization.Same issue: string interpolation happens before the conditional check.
231-232: String interpolation defeats the conditional check optimization.Same issue: string interpolation happens before the conditional check.
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/Http/Http.cs (1)
235-238: Consider capturing the exception for better diagnostics.While the bare
catchis acceptable for returning an empty string on any failure, capturing the exception variable would enable better diagnostics if needed in the future.- catch + catch (Exception e) { + // Optionally log: Log.Debug(ClassName, $"GetStringAsync failed: {e.Message}"); return string.Empty; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Flow.Launcher.Core/Plugin/PluginManager.cs(1 hunks)Flow.Launcher.Core/Plugin/PluginsLoader.cs(1 hunks)Flow.Launcher.Infrastructure/Http/Http.cs(6 hunks)Flow.Launcher.Infrastructure/Logger/Log.cs(6 hunks)Flow.Launcher.Infrastructure/Stopwatch.cs(3 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(1 hunks)Flow.Launcher.Plugin/LogLevel.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher.Core/Plugin/PluginManager.cs
- Flow.Launcher.Infrastructure/Stopwatch.cs
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-07-20T07:28:28.092Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3850
File: Flow.Launcher.Core/Resource/Internationalization.cs:0-0
Timestamp: 2025-07-20T07:28:28.092Z
Learning: In Flow Launcher's Internationalization class, when the Flow Launcher language directory or default language file is missing, the only viable approach is to log an error and return early - there are no fallback mechanisms or alternative recovery strategies available due to architectural constraints.
Applied to files:
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Infrastructure/Http/Http.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-01-18T10:10:18.414Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Applied to files:
Flow.Launcher.Core/Plugin/PluginsLoader.csFlow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Flow.Launcher.Infrastructure/Logger/Log.cs
📚 Learning: 2025-09-06T05:32:51.575Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.
Applied to files:
Flow.Launcher.Infrastructure/Logger/Log.cs
🧬 Code graph analysis (5)
Flow.Launcher.Plugin/LogLevel.cs (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LOGLEVEL(645-645)Flow.Launcher/PublicAPIInstance.cs (1)
LOGLEVEL(626-626)
Flow.Launcher.Infrastructure/Http/Http.cs (3)
Flow.Launcher.Infrastructure/Logger/Log.cs (3)
Log(14-170)Log(22-67)Debug(156-159)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LOGLEVEL(645-645)Flow.Launcher.Infrastructure/Stopwatch.cs (1)
Debug(14-24)
Flow.Launcher.Core/Plugin/PluginsLoader.cs (1)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
LOGLEVEL(645-645)LogDebug(302-302)
Flow.Launcher.Infrastructure/Logger/Log.cs (3)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LOGLEVEL(645-645)Flow.Launcher/PublicAPIInstance.cs (1)
LOGLEVEL(626-626)Flow.Launcher.Infrastructure/Stopwatch.cs (2)
Debug(14-24)Info(41-50)
Flow.Launcher/PublicAPIInstance.cs (2)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
LOGLEVEL(645-645)Flow.Launcher.Infrastructure/Logger/Log.cs (2)
Log(14-170)Log(22-67)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (8)
Flow.Launcher/PublicAPIInstance.cs (2)
626-627: LGTM!The
GetLogLevel()implementation correctly exposes the current log level via the public API surface, aligning with the interface contract.
55-55: TheLocktype is available—project targets .NET 9.0.The project's target framework is
net9.0-windows10.0.19041.0, which meansSystem.Threading.Lock(introduced in .NET 9) is fully supported. The change fromobjecttoLockis compatible with the codebase.Flow.Launcher.Plugin/LogLevel.cs (1)
1-27: LGTM!The
LOGLEVELenum is well-designed with clear XML documentation and follows a logical hierarchy. The placement in theFlow.Launcher.Pluginnamespace makes it accessible to both the core application and plugin developers.Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
640-645: LGTM!The
GetLogLevel()API addition is well-documented and provides a clean interface for plugins to query the current log level, enabling them to implement conditional logging.Flow.Launcher.Infrastructure/Logger/Log.cs (3)
19-20: LGTM!The public
LogLevelproperty with private setter properly exposes the current log level while maintaining control over modifications through theSetLogLevelmethod.
69-90: LGTM!The
SetLogLevelimplementation correctly:
- Updates the public
LogLevelproperty- Maps
LOGLEVELvalues to NLog levels appropriately- Reconfigures existing loggers
- Guards its own debug log output with a level check
142-169: LGTM!The logging methods correctly route to the appropriate NLog levels through the
LogInternalhelper, maintaining clean separation between the public API and NLog implementation details.Flow.Launcher.Infrastructure/Http/Http.cs (1)
150-152: String interpolation defeats the conditional check optimization.The string interpolation
$"Url <{url}>"occurs during the method call argument evaluation, before the conditional check executes. The performance cost is already paid regardless of log level.Apply this diff to move interpolation inside the conditional:
- if (Log.LogLevel == LOGLEVEL.DEBUG) - Log.Debug(ClassName, $"Url <{url}>"); + if (Log.LogLevel == LOGLEVEL.DEBUG) + { + Log.Debug(ClassName, $"Url <{url}>"); + }Likely an incorrect or invalid review comment.
Replaced direct usage of `Settings.LogLevel` with `App.API.GetLogLevel()` to centralize log level retrieval and align with the updated API. Updated logging calls to use `App.API.LogDebug` for consistency. Removed the `using Flow.Launcher.Infrastructure.Logger;` directive from `MainViewModel.cs` and `ResultsViewModel.cs`, as logging is now accessed through `App.API`. Cleaned up unused namespaces in `ResultsViewModel.cs`. Added `using CommunityToolkit.Mvvm.Input;` in `MainViewModel.cs` to support new MVVM-related functionality. Improved code readability and maintainability by reducing redundancy in log level checks and logging calls.
Enhanced logging with conditional debug checks based on log level, ensuring logs are only generated in DEBUG mode. Integrated
CommunityToolkit.Mvvm.DependencyInjectionfor dependency injection, improving modularity and testability.FlowSettingsvia DI.PluginManager.cs,PluginsLoader.cs,MainViewModel.cs, andResultsViewModel.cs.