Make logging thread-safe and add runtime-configurable log level#2834
Open
joka921 wants to merge 5 commits intoad-freiburg:masterfrom
Open
Make logging thread-safe and add runtime-configurable log level#2834joka921 wants to merge 5 commits intoad-freiburg:masterfrom
joka921 wants to merge 5 commits intoad-freiburg:masterfrom
Conversation
- Add a global `std::mutex` (via a non-nodiscard `LogLock` wrapper) so that each `AD_LOG_*` statement is serialized: the lock is acquired before the first `<<` and released at the closing `;`, preventing interleaved output from concurrent threads. - Add `std::atomic<LogLevel> runtimeLogLevel` (default `INFO`) and check it in the `AD_LOG` macro alongside the existing compile-time `LOGLEVEL` guard. Messages are suppressed when *either* gate rejects them. - Add free functions `logLevelFromString` / `logLevelToString` in `Log.h`. - Add a `LogLevelParameter` type (with string serializers) to `RuntimeParameters`, wired via `setOnUpdateAction` so any change to the parameter (HTTP, CLI) is immediately reflected in the atomic. - Change the CMake default compile-time `LOGLEVEL` from build-type-dependent (`DEBUG` / `INFO`) to always `DEBUG`, so all messages are compiled in; the runtime level (default `INFO`) handles the actual filtering. - Expose `--log-level` as a CLI flag in `ServerMain.cpp`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
LogLevel now inherits from EnumWithStrings<LogLevel, detail::LogLevelEnum>, which provides fromString(), toString(), JSON serialization, and a generic boost::program_options validate() via ADL. This removes the hand-written logLevelFromString()/logLevelToString() free functions, the LogLevel-specific validate() in ProgramOptionsHelpers.h, and simplifies the wrapper structs in RuntimeParameters.h. EnumWithStrings.h is updated to include <nlohmann/json.hpp> directly instead of util/json.h to avoid a circular include chain through util/File.h. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove the redundant `static const LogLevel FATAL/ERROR/...` members and their inline definitions; the EnumWithStrings base provides all necessary conversions without them. - `setRuntimeLogLevel` now throws `std::runtime_error` when the requested level is more verbose than the compile-time LOGLEVEL, since such messages are compiled out and can never appear at runtime. - Add `ScopedRuntimeLogLevel` RAII guard to temporarily set the runtime log level for a scope and restore it on exit. - Update `SKIP_IF_LOGLEVEL_IS_LOWER` in GTestHelpers.h to install a `ScopedRuntimeLogLevel` guard, so tests that capture DEBUG log output work correctly even when the default runtime level is INFO. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- AD_LOG macro: use fully-qualified ::ad_utility:: names so it works correctly from any namespace, including deeply nested ones. - setRuntimeLogLevel: use absl::StrCat instead of manual std::string concatenation for the error message. - Remove ScopedRuntimeLogLevel struct; GTestHelpers.h now uses an absl::Cleanup directly in SKIP_IF_LOGLEVEL_IS_LOWER. - Default runtime log level (both in Log.h and RuntimeParameters.h) is now min(INFO, LOGLEVEL) so the runtime level is never initialized to a value the binary cannot actually log. - CMakeLists.txt: extend LOGLEVEL description to mention that less verbose levels can be chosen at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joka921
commented
Apr 29, 2026
Member
Author
joka921
left a comment
joka921
left a comment
There was a problem hiding this comment.
Some comments from a self review, first wait for other reviews before working this in.
Comment on lines
+188
to
+189
| LogLevel{LOGLEVEL < LogLevel::Enum::INFO ? LOGLEVEL | ||
| : LogLevel::Enum::INFO}, |
| // Defaults to the less verbose of INFO and the compile-time LOGLEVEL so that | ||
| // the runtime level is never set to something the binary cannot log. | ||
| inline std::atomic<LogLevel::Enum> runtimeLogLevel{ | ||
| LOGLEVEL < LogLevel::Enum::INFO ? LOGLEVEL : LogLevel::Enum::INFO}; |
Comment on lines
+114
to
+117
| "Cannot set runtime log level to \"", level.toString(), | ||
| "\" because the compile-time log level is \"", | ||
| LogLevel{LOGLEVEL}.toString(), "\". Recompile with -DLOGLEVEL=", | ||
| level.toString(), " or higher to enable this log level.")}; |
Member
Author
There was a problem hiding this comment.
Maybe use backticks instead of quotes.
joka921
requested review from
RobinTF and
ullingerc
and removed request for
RobinTF
Overview
Conformance check passed ✅No test result changes. |
RobinTF
requested changes
Apr 29, 2026
Collaborator
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
I agree with your comments, I have a small question.
| P(FATAL, "FATAL"), | ||
| }}; | ||
| return map.at(LEVEL); | ||
| return LogLevel::descriptions_[static_cast<size_t>(LEVEL)].second; |
Collaborator
There was a problem hiding this comment.
Is there a reason why this can't be LEVEL.toString()?
|
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.
Summary
AD_LOG_*statement now acquires a globalstd::mutex(via aLogLockRAII wrapper used in the comma-operator expression) before writing to the stream. The lock covers the entire<<chain and is released at the statement's closing;, so messages from concurrent threads can no longer interleave.std::atomic<LogLevel> runtimeLogLevel(defaultINFO) is checked inAD_LOGalongside the existing compile-timeLOGLEVELguard. A message is suppressed if either gate rejects it.RuntimeParametersintegration: A newlogLevel_parameter (typeLogLevelParameter) is added toRuntimeParameters. ItssetOnUpdateActionwires changes directly to the atomic, so the level can be changed at runtime via HTTP (?cmd=set-parameter&log-level=DEBUG) or at startup via CLI.--log-levelis exposed inServerMain.cppwith defaultINFO.LOGLEVELnow always defaults toDEBUG(previously it was build-type-dependent). Since all messages are compiled in, the runtime level governs actual output.Test plan
cmake --build build --target qlever-serverbuilds without errors or warnings../qlever-server --helpshows--log-level arg (=INFO)with correct description.--log-level DEBUGproduces debug output;--log-level WARNsuppresses info messages.--log-level fooproduces a clear error message.?cmd=set-parameter&log-level=DEBUG) takes effect immediately.🤖 Generated with Claude Code