feat: use spdlog for logging across project#1643
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded spdlog as a build dependency and integrated it into the project's logging subsystem; replaced many direct console outputs with g_logger calls and added file/console sink management and sanitization-aware HTTP logging across multiple source files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 7
🧹 Nitpick comments (2)
src/framework/core/logger.cpp (1)
159-165: Inner null-check onspdlog::default_logger()is always true.
getSpdLogger()returns null only whencreateSpdLogger()failed and returnednullptr. In that case,spdlog::set_default_logger()was never called with our custom logger, sospdlog::default_logger()returns spdlog's own built-in default logger — which is always non-null. Theif (const auto fallbackLogger = ...)guard is therefore unreachable as a null path and can be simplified:♻️ Simplify the fallback branch
- } else { - if (const auto fallbackLogger = spdlog::default_logger(); fallbackLogger) { - fallbackLogger->log(toSpdLogLevel(level), "{}", message); - if (level >= Fw::LogError) { - fallbackLogger->flush(); - } - } - } + } else { + spdlog::log(toSpdLogLevel(level), "{}", message); + if (level >= Fw::LogError) + spdlog::default_logger()->flush(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/logger.cpp` around lines 159 - 165, The inner null-check around spdlog::default_logger() is redundant because spdlog always provides a built-in default logger; simplify the fallback branch in logger.cpp by removing the conditional guard (if (const auto fallbackLogger = spdlog::default_logger(); fallbackLogger)) and instead directly retrieve the fallback via spdlog::default_logger(), then log the message with toSpdLogLevel(level) and flush when level >= Fw::LogError; this keeps behavior when getSpdLogger()/createSpdLogger() failed but removes the unreachable null-path check.src/framework/net/httplogin.cpp (1)
87-87:json::acceptis redundant after a successfuljson::parse.
json::parseat line 84 already succeeded (or would have thrown), sojson::acceptalways returnstruehere. The extra parse pass is wasted work.♻️ Proposed fix
- g_logger.debug("json::accept={}", json::accept(res->body));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/net/httplogin.cpp` at line 87, Remove the redundant json::accept call after a successful json::parse: replace the g_logger.debug("json::accept={}", json::accept(res->body)) line so it does not re-parse res->body; instead log the already-parsed value (the variable produced by json::parse) or log a static confirmation. Reference symbols: json::parse, json::accept, g_logger.debug, and res->body—use the parsed JSON variable from json::parse when logging, or drop the accept check entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CMakeLists.txt`:
- Line 562: The build links both spdlog::spdlog_header_only and
fmt::fmt-header-only causing duplicate fmt symbols; after each
find_package(spdlog) add a preprocessor definition SPDLOG_FMT_EXTERNAL_HO so
spdlog uses the external fmt implementation instead of its bundled fmt
(preventing ODR violations). Locate the CMakeLists occurrences around the
find_package(spdlog) lines that add spdlog::spdlog_header_only (references:
spdlog::spdlog_header_only and fmt::fmt-header-only) and set the define (e.g.,
via add_compile_definitions or target_compile_definitions for the target that
links spdlog) in each platform branch mentioned (lines referenced in the
comment).
In `@src/framework/core/logger.cpp`:
- Around line 47-50: The timestamp patterns use non-standard Year-Day-Month
ordering; update the four constexpr pattern strings s_spdConsolePattern,
s_spdConsolePatternDebug, s_spdFilePattern, and s_spdFilePatternDebug to use ISO
ordering by replacing "%Y-%d-%m" with "%Y-%m-%d" in each string so timestamps
render as Year-Month-Day (e.g., 2026-02-18).
In `@src/framework/net/httplogin.cpp`:
- Line 91: The log in startHttpLogin currently uses g_logger.error for a
connection failure while loginHttpsJson and loginHttpJson use g_logger.warning;
change the call in startHttpLogin from g_logger.error("HTTP error: {}",
to_string(err)) to g_logger.warning(...) so all three connection-failure paths
use the same warning level, preserving the same message and formatting; locate
this in the startHttpLogin function and update the logging call accordingly.
- Around line 48-68: The current logging prints plaintext credentials and tokens
(req.body, res.body, req.headers, res.headers) — change the logic that prepares
data for g_logger.debug so sensitive fields are redacted: parse req.body and
remove or replace the "password" field (and any "password*" variants) before
logging req.body; parse res.body and truncate or hash any session/token fields
(e.g., "token", "session", "access_token") before logging res.body; and when
iterating req.headers/res.headers, mask values for sensitive header keys like
"Authorization" and "Set-Cookie" instead of printing them raw. Implement these
redaction steps in the same scope where g_logger.debug is called so all debug
output uses the sanitized strings.
In `@src/main.cpp`:
- Around line 60-70: The help text should be written directly to stdout instead
of using the spdlog logger; modify printHelp so it uses std::cout (or
std::fprintf(stdout,...)) to emit the multi-line usage string with the
executableName substituted, rather than calling g_logger.info("..."), so the
output is unprefixed and always printed even when the spdlog sink is inactive;
locate the printHelp function and replace the g_logger.info call with a direct
stdout write of the same text (preserving newlines and options layout) while
keeping the executableName interpolation.
In `@src/tools/datdump.cpp`:
- Around line 247-249: The stdout branch currently uses g_logger.info("{}",
payload) which adds spdlog prefixes and breaks JSON consumers; change that
branch so it emits the raw payload exactly like the file path does (use
std::cout << payload << '\n' and flush as needed) instead of g_logger.info,
keeping the same variables (request.outputPath, payload) and behavior as the out
<< payload << '\n' file-write path.
---
Nitpick comments:
In `@src/framework/core/logger.cpp`:
- Around line 159-165: The inner null-check around spdlog::default_logger() is
redundant because spdlog always provides a built-in default logger; simplify the
fallback branch in logger.cpp by removing the conditional guard (if (const auto
fallbackLogger = spdlog::default_logger(); fallbackLogger)) and instead directly
retrieve the fallback via spdlog::default_logger(), then log the message with
toSpdLogLevel(level) and flush when level >= Fw::LogError; this keeps behavior
when getSpdLogger()/createSpdLogger() failed but removes the unreachable
null-path check.
In `@src/framework/net/httplogin.cpp`:
- Line 87: Remove the redundant json::accept call after a successful
json::parse: replace the g_logger.debug("json::accept={}",
json::accept(res->body)) line so it does not re-parse res->body; instead log the
already-parsed value (the variable produced by json::parse) or log a static
confirmation. Reference symbols: json::parse, json::accept, g_logger.debug, and
res->body—use the parsed JSON variable from json::parse when logging, or drop
the accept check entirely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/framework/core/logger.cpp (3)
95-111: Swallowing all exceptions silently during logger creation.The
catch (...)block discards any diagnostic information about why spdlog initialization failed. At a minimum, consider catchingspdlog::spdlog_exand writing tostderrso the failure isn't invisible — this is the one place where you can't useg_loggeritself.♻️ Proposed improvement
return logger; - } catch (...) { + } catch (const spdlog::spdlog_ex& ex) { + std::cerr << "Failed to create spdlog logger: " << ex.what() << std::endl; + return nullptr; + } catch (const std::exception& ex) { + std::cerr << "Failed to create spdlog logger: " << ex.what() << std::endl; return nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/logger.cpp` around lines 95 - 111, The createSpdLogger function currently swallows all exceptions; update its catch to at minimum catch spdlog::spdlog_ex (and optionally std::exception) and write a clear diagnostic to stderr (e.g., include ex.what()) before returning nullptr so failures to initialize spdlog are visible even when g_logger isn't available; locate this in createSpdLogger and replace the catch(...) with targeted catches that print to stderr.
153-165: Fallback logic is sound but double-logs the prefix.When spdlog is active (line 155),
messageis logged without the prefix (e.g.,"WARNING: "), because spdlog's own[warn]tag conveys the level. However,outmsg(line 146–147) still prepends the legacy prefix and is written tom_outFile(line 168) and pushed intom_logMessages(line 173). This means the file-fallback path and the in-memory history carry a redundant"WARNING: "prefix while the spdlog output does not, producing inconsistent log messages between sinks.Not a blocker since the legacy file path is only reached when spdlog file-sink setup fails, but worth being aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/logger.cpp` around lines 153 - 165, The file- and in-memory sinks are receiving messages with the legacy level prefix while spdlog output does not, causing inconsistent logs; update the logging path so when getSpdLogger() returns non-null you write the raw message (no legacy prefix) to m_outFile and push the raw message into m_logMessages, and only apply the legacy "LEVEL: " prefix when spdLogger is null (i.e., in the fallback branch). Locate getSpdLogger(), the variable message, the constructed outmsg, and where m_outFile and m_logMessages are written and change the prefixing logic to conditionally prepend the legacy prefix only in the fallback branch so both sinks produce consistent content.
237-244: Sink vector mutation is not guarded — relies on caller being on the event thread.
spdLogger->sinks()returns a reference to the internalstd::vector<sink_ptr>. Erasing/pushing while another thread could be iterating the same vector inlogger::log()is a data race. This is safe only if every call toLogger::logandLogger::setLogFileis guaranteed to run on the event thread (enforced by the dispatcher at line 139). IfsetLogFilecan ever be called from a different thread (e.g., during early init before the dispatcher is running), this would become a real race.Consider documenting this thread-safety invariant, or adding a brief assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/logger.cpp` around lines 237 - 244, The sink vector mutation in Logger::setLogFile (manipulating spdLogger->sinks() and getSpdLogFileSink()) can race with iterations in Logger::log; add a guard by asserting the caller is on the event thread at the start of Logger::setLogFile (using your dispatcher/isEventThread check) or, if you prefer runtime safety, protect access to the sink vector by a mutex used by both Logger::log and Logger::setLogFile before calling spdLogger->sinks(), std::erase, and push_back so mutations cannot occur concurrently.src/framework/net/httplogin.cpp (1)
443-446: Parse failure logged atinfo— considerwarning.A JSON parse failure from the login server is an unexpected condition (malformed or non-JSON response).
infosuggests normal operation;warningwould be more appropriate and consistent with the other HTTP error paths in this file.♻️ Proposed change
- g_logger.info("Failed to parse json response"); + g_logger.warning("Failed to parse json response");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/net/httplogin.cpp` around lines 443 - 446, In the catch(...) block that handles JSON parse failures in httplogin.cpp (where g_logger.info("Failed to parse json response") and this->errorMessage are set), change the log level from info to warning to reflect an unexpected/malformed server response; update the call to use the logger's warning method (matching other HTTP error paths) while keeping the same message and leaving this->errorMessage assignment unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/net/httplogin.cpp`:
- Around line 183-186: The debug log is printing raw parsed response JSON
(bodyResponse.dump()) which may contain sensitive login data; replace this by
sanitizing the response before logging: call the existing sanitizeHttpBody
helper with res->body (or the parsed body) and log the sanitized result instead
of bodyResponse.dump(); modify the block checking res->status == 200 in
httplogin.cpp to use sanitizeHttpBody(...) and pass that string to
g_logger.debug(...) so sensitive fields are redacted while preserving the debug
output.
---
Nitpick comments:
In `@src/framework/core/logger.cpp`:
- Around line 95-111: The createSpdLogger function currently swallows all
exceptions; update its catch to at minimum catch spdlog::spdlog_ex (and
optionally std::exception) and write a clear diagnostic to stderr (e.g., include
ex.what()) before returning nullptr so failures to initialize spdlog are visible
even when g_logger isn't available; locate this in createSpdLogger and replace
the catch(...) with targeted catches that print to stderr.
- Around line 153-165: The file- and in-memory sinks are receiving messages with
the legacy level prefix while spdlog output does not, causing inconsistent logs;
update the logging path so when getSpdLogger() returns non-null you write the
raw message (no legacy prefix) to m_outFile and push the raw message into
m_logMessages, and only apply the legacy "LEVEL: " prefix when spdLogger is null
(i.e., in the fallback branch). Locate getSpdLogger(), the variable message, the
constructed outmsg, and where m_outFile and m_logMessages are written and change
the prefixing logic to conditionally prepend the legacy prefix only in the
fallback branch so both sinks produce consistent content.
- Around line 237-244: The sink vector mutation in Logger::setLogFile
(manipulating spdLogger->sinks() and getSpdLogFileSink()) can race with
iterations in Logger::log; add a guard by asserting the caller is on the event
thread at the start of Logger::setLogFile (using your dispatcher/isEventThread
check) or, if you prefer runtime safety, protect access to the sink vector by a
mutex used by both Logger::log and Logger::setLogFile before calling
spdLogger->sinks(), std::erase, and push_back so mutations cannot occur
concurrently.
In `@src/framework/net/httplogin.cpp`:
- Around line 443-446: In the catch(...) block that handles JSON parse failures
in httplogin.cpp (where g_logger.info("Failed to parse json response") and
this->errorMessage are set), change the log level from info to warning to
reflect an unexpected/malformed server response; update the call to use the
logger's warning method (matching other HTTP error paths) while keeping the same
message and leaving this->errorMessage assignment unchanged.
Add spdlog dependency and migrate console/file logging to spdlog. - Add find_package(spdlog) and link spdlog::spdlog_header_only in CMakeLists and add spdlog to vcpkg.json. - Integrate spdlog in logger.cpp: include spdlog headers, create a default colored stdout logger, define console/file patterns (debug vs release), map Fw::LogLevel to spdlog levels, route Logger::log through spdlog (with flush on errors) and implement setLogFile to manage a basic_file_sink. - Replace direct std::cout/std::clog printing with g_logger (or spdlog calls) in httplogin, proxy_client, soundeffect, main, datdump and stdext/cast; add/remove includes accordingly and remove unused iostream includes from pch and other files. - Preserve legacy file-stream fallback when spdlog is not available or file sink setup fails.
Add HTTP log sanitization and redaction for sensitive headers, JSON request/response fields, and token-like values; include helper functions (toLowerCopy, sanitizeJson, sanitizeHttpBody, etc.). Fix spdlog time patterns (use %Y-%m-%d) and simplify fallback logger handling. Change an HTTP error log to a warning and remove a json::accept debug line. Remove direct spdlog usage in stdext::cast (stop logging cast exceptions) and replace some g_logger uses with std::cout for help and datdump output; add required includes (cctype, iostream) and adjust datdump to write to stdout when no output path is provided.
libergod
force-pushed
the
dudantas/feat-spdlog-for-coloured-and-detailed-logs
branch
from
12607b1 to
9cb4a0e
Compare
Collaborator
|
Currently, testing steps:
Machine: Windows 11 Pro, 16GB, i7 13th gen |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new logging backend using the
spdloglibrary and refactors logging throughout the codebase to use the centralizedg_loggerinterface. It also updates the build configuration to require and link againstspdlog. The main themes are logging modernization, build system changes, and code cleanup.Logging Modernization and Refactoring:
Integrated
spdlogas the new logging backend, replacing direct usage ofstd::cout,std::clog, andstd::endlwith calls tog_loggerthroughout the codebase. This includes adding log level support, log formatting, and file logging viaspdlogsinks. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19]Updated the logger implementation in
logger.cppto usespdlogfor both console and file output, including custom log patterns and dynamic sink management. [1] [2] [3]Refactored HTTP and proxy logging to use
g_loggerwith appropriate log levels (debug, warning, error), improving consistency and log filtering. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]Build System Updates:
find_package(spdlog CONFIG REQUIRED)tosrc/CMakeLists.txtand linkedspdlog::spdlog_header_onlytarget for all supported platforms, ensuring the dependency is available and used during builds. [1] [2] [3] [4] [5]Code Cleanup:
#include <iostream>from several files and the precompiled header, as logging now usesspdlogexclusively. [1] [2]These changes modernize the logging infrastructure, improve log consistency, and prepare the codebase for better log management and filtering.
Summary by CodeRabbit