Add the logging system#36
Conversation
|
Warning Review limit reached
More reviews will be available in 57 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new C++20 Changescore.logging Module
Sequence Diagram(s)sequenceDiagram
participant Caller as Call Site (LOG_INFO macro)
participant logfmt as log_fmt()
participant logfn as log()
participant RingBuffer as Lock-free Ring Buffer
participant WorkerThread as Worker Thread
participant Sink as ILogSink (write/flush)
Caller->>logfmt: LOG_INFO(Category, fmt, args...)
logfmt->>logfmt: std::format_to_n into stack buffer
logfmt->>logfn: log(verbosity, category, file, func, line, text)
logfn->>logfn: check accepting_logs + verbosity threshold
logfn->>logfn: increment active_producers
logfn->>RingBuffer: enqueue(LogMessage) via sequence CAS
alt queue full
RingBuffer-->>logfn: drop, increment messages_dropped
else enqueued
RingBuffer-->>WorkerThread: notify condition_variable
end
logfn->>logfn: decrement active_producers
WorkerThread->>RingBuffer: dequeue next LogMessage
WorkerThread->>Sink: write(LogMessage)
WorkerThread->>WorkerThread: increment messages_written
sequenceDiagram
participant Caller as Call Site (LOG_FATAL macro)
participant fatalfmt as fatal_fmt()
participant fatalfn as fatal()
participant Stdout as stdout
participant Sink as All registered ILogSinks
participant Hook as internal::fatal_hook
Caller->>fatalfmt: LOG_FATAL(Category, fmt, args...)
fatalfmt->>fatalfn: fatal(category, file, func, line, text)
fatalfn->>Stdout: fprintf formatted fatal line
fatalfn->>Sink: write(LogMessage) for each sink
fatalfn->>Sink: flush() for each sink
fatalfn->>Hook: invoke fatal_hook()
fatalfn->>fatalfn: std::abort()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
Engine/cpp/runtime/core/logging/logging.test.cpp (1)
247-265: ⚡ Quick winMake
fatal_hookrestoration exception-safe.This test mutates global process state and restores it manually; if control exits unexpectedly before Line 264, later tests inherit the wrong hook.
Suggested fix
- draco::core::logging::internal::fatal_hook = [] { throw FatalTestException{}; }; + auto previous_fatal_hook = draco::core::logging::internal::fatal_hook; + struct FatalHookRestore final + { + void (**slot)(); + void (*old_value)(); + ~FatalHookRestore() { *slot = old_value; } + } fatal_hook_restore{&draco::core::logging::internal::fatal_hook, previous_fatal_hook}; + + draco::core::logging::internal::fatal_hook = [] { throw FatalTestException{}; }; @@ - draco::core::logging::internal::fatal_hook = [] { std::abort(); }; shutdown();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/runtime/core/logging/logging.test.cpp` around lines 247 - 265, The fatal_hook is manually restored after the test logic, but if an exception is thrown before reaching the restoration line at the end, the global state remains corrupted for subsequent tests. Wrap the fatal_hook assignment at the beginning of the test in a RAII pattern or scope guard that guarantees the hook is restored to its original state (std::abort) even if exceptions occur during the LOG_FATAL call or any other operation within the test scope. Ensure the restoration happens automatically when exiting the test scope, rather than relying on manual restoration at a specific line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Engine/cpp/runtime/core/logging/logging.cpp`:
- Around line 444-451: The timestamp format at line 444 uses second-level
precision which causes filename collisions when multiple log rolls occur within
the same second, causing the std::ios::trunc flag at line 451 to overwrite
previously rotated logs. Add higher precision to the timestamp (such as
milliseconds or microseconds) or implement a counter-based collision avoidance
mechanism in the new_path construction at line 450 to ensure each rolled log
file gets a unique filename even when rolls happen within the same second.
- Around line 195-219: The init() function creates a new worker thread and
assigns it to g_worker_thread without checking if a thread is already running.
To prevent std::terminate errors from repeated init() calls, add a guard before
the thread creation line that checks if g_worker_thread is joinable; if it is,
return early from the init() function to prevent reassigning an active thread.
The check should happen before g_worker_thread is assigned the new
std::thread(worker_loop).
- Around line 162-169: The worker thread loop (where sink->write(msg) is called
around line 166-169), the flush() function (lines 258-260), and the fatal()
function (lines 368-369) all invoke sink I/O operations without synchronization,
creating a data race when using std::ofstream-backed sinks. Add a new mutex
(separate from the existing g_sink_update_mutex which only protects sink list
modifications) to serialize all sink I/O operations. Lock this mutex around the
sink->write() call in the worker thread dequeue loop, around the flush
operations in the flush() function, and around the sink operations in the
fatal() function to ensure thread-safe access to the sink objects across all
three call sites.
In `@Engine/cpp/runtime/core/logging/logging.test.cpp`:
- Around line 288-306: The test has two issues: the ifstream object infile
opened at the file validation section is never closed before attempting to
remove the file at the end of the test, which causes cross-platform failures on
Windows where open files cannot be deleted; additionally, the hardcoded
test_output.log path can cause collisions when tests run in parallel. Fix this
by ensuring the infile stream is closed before file removal (either by wrapping
the file reading operations in a separate scope or explicitly calling
infile.close()) and by replacing the hardcoded test_path string with a unique
temporary file path using std::filesystem mechanisms to generate a unique name
per test run.
In `@Engine/cpp/runtime/core/logging/macros.h`:
- Around line 3-12: The SET_CATEGORY_VERBOSITY macro uses
std::memory_order_relaxed which is declared in the <atomic> header, but the
header file does not include it. Add `#include` <atomic> at the top of the file
near the existing `#include` <format> to ensure the header is self-sufficient and
all dependencies are properly declared, preventing potential compilation issues
for callers that may not have included <atomic> before using these macros.
---
Nitpick comments:
In `@Engine/cpp/runtime/core/logging/logging.test.cpp`:
- Around line 247-265: The fatal_hook is manually restored after the test logic,
but if an exception is thrown before reaching the restoration line at the end,
the global state remains corrupted for subsequent tests. Wrap the fatal_hook
assignment at the beginning of the test in a RAII pattern or scope guard that
guarantees the hook is restored to its original state (std::abort) even if
exceptions occur during the LOG_FATAL call or any other operation within the
test scope. Ensure the restoration happens automatically when exiting the test
scope, rather than relying on manual restoration at a specific line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2530ce9c-7b86-4ae8-8d00-4f2021e58a00
📒 Files selected for processing (8)
Engine/cpp/runtime/CMakeLists.txtEngine/cpp/runtime/core/CMakeLists.txtEngine/cpp/runtime/core/core.cppmEngine/cpp/runtime/core/io/image_loader.cppEngine/cpp/runtime/core/logging/logging.cppEngine/cpp/runtime/core/logging/logging.cppmEngine/cpp/runtime/core/logging/logging.test.cppEngine/cpp/runtime/core/logging/macros.h
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| #define LOG_FATAL(Category, fmt, ...) draco::core::logging::fatal_fmt(Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) | ||
| #define LOG_ERROR(Category, fmt, ...) draco::core::logging::log_fmt(draco::core::logging::LogVerbosity::Error, Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) | ||
| #define LOG_WARNING(Category, fmt, ...) draco::core::logging::log_fmt(draco::core::logging::LogVerbosity::Warning, Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) | ||
| #define LOG_DISPLAY(Category, fmt, ...) draco::core::logging::log_fmt(draco::core::logging::LogVerbosity::Display, Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) | ||
| #define LOG_INFO(Category, fmt, ...) draco::core::logging::log_fmt(draco::core::logging::LogVerbosity::Info, Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) | ||
| #define LOG_VERBOSE(Category, fmt, ...) draco::core::logging::log_fmt(draco::core::logging::LogVerbosity::Verbose, Log##Category, __FILE__, __func__, __LINE__, fmt, ##__VA_ARGS__) |
There was a problem hiding this comment.
We should probably opt to use std::source_location instead of the macros if possible, which might bring some trouble to the VA_ARGS.
Also it is probably better to use template Variadic argument list instead which makes the logging types safer. @OldDev78 do you have any comments on this?
There was a problem hiding this comment.
Please use modern methods indeed. std::source_location is cross-platform, and I'd also very much prefer variadic templates over macros.
|
|
||
| struct ILogSink | ||
| { | ||
| virtual ~ILogSink() = default; | ||
| virtual void write(const LogMessage& msg) = 0; | ||
| virtual void flush() = 0; | ||
| }; | ||
|
|
||
| class ConsoleSink final : public ILogSink | ||
| { | ||
| public: | ||
| void write(const LogMessage& msg) override; | ||
| void flush() override; | ||
| }; | ||
|
|
||
| class FileSink : public ILogSink | ||
| { | ||
| protected: | ||
| std::ofstream m_stream; | ||
| public: | ||
| FileSink(const std::filesystem::path& path); | ||
| void write(const LogMessage& msg) override; | ||
| void flush() override; | ||
| }; | ||
|
|
||
| class RollingFileSink final : public ILogSink | ||
| { | ||
| private: | ||
| std::filesystem::path m_base_path; | ||
| usize m_max_file_size; | ||
| usize m_current_size{0}; | ||
| std::ofstream m_stream; | ||
| void roll_file(); | ||
| public: | ||
| RollingFileSink(const std::filesystem::path& base_path, usize max_file_size); | ||
| void write(const LogMessage& msg) override; | ||
| void flush() override; | ||
| }; |
There was a problem hiding this comment.
Can we get some documentation explaining each of these Sinks and their usages?
There was a problem hiding this comment.
Can we get some documentation explaining each of these Sinks and their usages?
Sure, but should it be in Docs or here?
| const char* function; | ||
| u32 line; | ||
|
|
||
| char text[MaxLogMessageLength]; |
There was a problem hiding this comment.
Will there ever be a case where we want to increase the log message length, or need it to be dynamically allocated?
There was a problem hiding this comment.
Will there ever be a case where we want to increase the log message length, or need it to be dynamically allocated?
I don't think so but I'll let @OldDev78 comment on it
| void add_sink(std::shared_ptr<ILogSink> sink); | ||
| void remove_sink(std::shared_ptr<ILogSink> sink); |
There was a problem hiding this comment.
I'm not sure if we should be using shared_ptr, @OldDev78 thoughts on how we're managing shared memory like this?
There was a problem hiding this comment.
This will be using the default allocator (calling new) to begin with. If our allocators cannot be used with STL smart pointers, wrappers for such use need to be made.
| void log(LogVerbosity verbosity, const LogCategory& category, const char* file, const char* function, u32 line, const char* text); | ||
|
|
||
| [[noreturn]] void fatal(const LogCategory& category, const char* file, const char* function, u32 line, const char* text); |
There was a problem hiding this comment.
Like mentioned with macros.h we can probably be using std::source_location
There was a problem hiding this comment.
We should not be relying on macros at all.
| try | ||
| { | ||
| auto result = std::format_to_n(buffer, MaxLogMessageLength - 1, fmt, std::forward<Args>(args)...); | ||
| *result.out = '\0'; | ||
| success = true; | ||
| } | ||
| catch (const std::format_error&) | ||
| { | ||
| // Only catch string formatting failures | ||
| } |
There was a problem hiding this comment.
Spooky exceptions, I believe we're trying to not use exceptions so leaking exceptions might cause some issues down the line.
There was a problem hiding this comment.
In this level for sure, I'd prefer everything noexcept. If allocation fails at all, let terminate be called.
3 participants
Summary by CodeRabbit