Uh oh!
There was an error while loading. Please reload this page.
feat(logging): add Loggers registry (6/6) - #737
Open
kamcheungting-db wants to merge 2 commits into
Open
Conversation
kamcheungting-dbforce-pushed
the
logging-block6-registry
branch
15 times, most recently
from
June 22, 2026 10:24
19003d8 to
51e997bComparekamcheungting-dbforce-pushed
the
logging-block6-registry
branch
6 times, most recently
from
June 24, 2026 18:39
faed95d to
d0066d4Comparekamcheungting-dbforce-pushed
the
logging-block6-registry
branch
2 times, most recently
from
June 30, 2026 20:37
49a68c1 to
b68bd93Comparemanuzhang
commented
Jul 9, 2026
Member
@kamcheungting-db can you please rebase your PR? |
kamcheungting-dbforce-pushed
the
logging-block6-registry
branch
5 times, most recently
from
July 16, 2026 07:21
8d4515e to
aaa1ebfCompareThere was a problem hiding this comment.
Pull request overview
This PR completes the logging stack by adding a configuration-driven Loggers registry/factory (mirroring MetricsReporters), installing a process-default logger selected by properties, and introducing public ICEBERG_LOG_* macros (plus opt-in bare LOG_* aliases) with end-to-end and unit test coverage across spdlog-on/off builds.
Changes:
- Added
Loggers::{Register, Load, LoadAndSetDefault}with built-in backends (noop,cerr, andspdlogwhen compiled in) and a compiled-backend default. - Introduced
iceberg/logging/log_macros.h(andshort_log_macros.h) plus aFatalHandlerhook for fatal logging. - Added comprehensive tests (macros behavior, registry behavior, spdlog backend, and end-to-end logging output), and wired builds (CMake + Meson) including MSVC
/Zc:preprocessorfor__VA_OPT__.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/iceberg/test/spdlog_logger_test.cc | Adds unit tests for the spdlog-backed logger implementation. |
| src/iceberg/test/meson.build | Registers new logging-related tests in Meson. |
| src/iceberg/test/macros_test.cc | Adds runtime behavior + death tests for logging macros (formatting, gating, fatal semantics, handler). |
| src/iceberg/test/macros_active_level_test.cc | Tests compile-time active-level stripping behavior. |
| src/iceberg/test/logging_end_to_end_test.cc | End-to-end validation of registry/config/default/macro integration and real output. |
| src/iceberg/test/loggers_test.cc | Adds unit tests for the Loggers registry and property handling. |
| src/iceberg/test/CMakeLists.txt | Enables MSVC conforming preprocessor for tests; adds new test sources. |
| src/iceberg/meson.build | Adds logging sources (including registry + spdlog backend) to Meson build. |
| src/iceberg/logging/short_log_macros.h | Provides opt-in bare LOG_* aliases for ICEBERG_LOG_*. |
| src/iceberg/logging/meson.build | Installs new public logging headers; generates Meson-only config.h. |
| src/iceberg/logging/loggers.h | Declares the public Loggers registry/factory API and property keys. |
| src/iceberg/logging/loggers.cc | Implements the registry, built-in factories, and load/initialize semantics. |
| src/iceberg/logging/logger.h | Extends logging API docs; adds FatalHandler API surface. |
| src/iceberg/logging/logger.cc | Implements compiled-backend default selection and fatal handler storage/access. |
| src/iceberg/logging/log_macros.h | Adds the ICEBERG_LOG_* macro layer and supporting helpers. |
| src/iceberg/logging/internal/spdlog_logger.h | Introduces internal spdlog-backed SpdLogger sink. |
| src/iceberg/logging/internal/spdlog_logger.cc | Implements spdlog sink behavior including pattern support and level mapping. |
| src/iceberg/logging/config.h.in | Adds generated build-time logging backend configuration header template. |
| src/iceberg/CMakeLists.txt | Generates config.h; gates spdlog compilation/link; exports /Zc:preprocessor for consumers. |
| meson.build | Enables /Zc:preprocessor via Meson supported-args on MSVC. |
| CMakeLists.txt | Adds ICEBERG_SPDLOG option. |
| cmake_modules/IcebergThirdpartyToolchain.cmake | Gates spdlog dependency resolution behind ICEBERG_SPDLOG. |
Comment on lines
+82
to
+95
| /// \brief Runtime-level variant against the current logger: emit if enabled, then | ||
| /// flush + abort when level == kFatal (using the same acquired logger). | ||
| template <typename MakeMessage> | ||
| void LogToCurrentRuntime(LogLevel level, const std::source_location& location, | ||
| MakeMessage&& make_message) noexcept { | ||
| const std::shared_ptr<Logger>& logger = CurrentLogger(); | ||
| if (logger) { | ||
| EmitIfEnabled(*logger, level, location, std::forward<MakeMessage>(make_message)); | ||
| } | ||
| if (level == LogLevel::kFatal) { | ||
| if (logger) logger->Flush(); | ||
| std::abort(); | ||
| } | ||
| } |
Comment on lines
+97
to
+108
| /// \brief Runtime-level variant against an explicit logger: emit if enabled, then | ||
| /// flush + abort when level == kFatal. | ||
| template <typename MakeMessage> | ||
| void LogToExplicitRuntime(Logger& logger, LogLevel level, | ||
| const std::source_location& location, | ||
| MakeMessage&& make_message) noexcept { | ||
| EmitIfEnabled(logger, level, location, std::forward<MakeMessage>(make_message)); | ||
| if (level == LogLevel::kFatal) { | ||
| logger.Flush(); | ||
| std::abort(); | ||
| } | ||
| } |
Comment on lines
+74
to
+79
| SpdLogger::SpdLogger(std::shared_ptr<spdlog::logger> logger, LogLevel level) | ||
| : logger_(std::move(logger)), level_(level) { | ||
| if (logger_) { | ||
| logger_->set_level(spdlog::level::trace); // filtering is done by ShouldLog | ||
| } | ||
| } |
kamcheungting-dbforce-pushed
the
logging-block6-registry
branch
4 times, most recently
from
August 6, 2026 01:05
865f93f to
6ad0e68CompareFifth block: the default production backend and the build option that selects it. - SpdLogger wraps spdlog::logger (kCritical/kFatal -> spdlog critical, others 1:1), forwarding the pre-formatted message and source location. Synchronous only in v1 (spdlog's source_loc is a non-owning const char*, unsafe with async sinks). It lives in logging/internal/, is gated by #ifdef ICEBERG_HAS_SPDLOG, and is NOT installed -- consumers obtain it via the default logger or the registry, never by including spdlog headers. - New ICEBERG_SPDLOG CMake option (default ON). config.h is ALWAYS generated (only ICEBERG_HAS_SPDLOG's definedness varies) so logger.cc compiles in both configurations; MakeDefaultLogger() prefers SpdLogger when compiled in, else CerrLogger. - Critically, ICEBERG_SPDLOG=OFF now UNWIRES the previously-unconditional spdlog link (interface-lib lists + resolve_spdlog_dependency), not just the new source -- so an OFF build has no spdlog dependency at all. spdlog_logger_test (compiled only on the ON path) covers the level mapping including fatal->critical and source-location forwarding. Co-authored-by: Isaac
kamcheungting-dbforce-pushed
the
logging-block6-registry
branch
from
August 6, 2026 01:58
6ad0e68 to
7ba75ceCompareFinal block: configuration-driven backend selection, mirroring MetricsReporters. - Loggers::Register(type, factory) registers a named backend; Loggers::Load(props) builds one, selecting the type from the "logger-impl" property key. - Built-in factories: "noop", "cerr", and (only when built with ICEBERG_SPDLOG) "spdlog". With no logger-impl set, the default is spdlog when compiled in, else cerr -- logs by default, an intentional divergence from the metrics registry's noop default. - Loggers::LoadAndSetDefault(props) loads a logger and installs it as the process default. This completes the system end to end: levels -> Logger interface + default logger -> CerrLogger/SpdLogger backends -> macros -> configuration-driven selection. loggers_test covers load default/noop/cerr, unknown-type errors, empty-factory rejection, custom Register, and LoadAndSetDefault. Adds logging_end_to_end_test, which drives the public surface as an application does -- now that every layer is present: configure a backend via the registry, install it as the default, log through the LOG_* macros, and observe real output. Covers registry -> default-slot -> macro -> backend -> std::cerr output, level filtering through the full macro path, the compiled-backend identity of the default (spdlog when ON, cerr when OFF), the "spdlog" factory by name, and a macro statement reaching a real spdlog sink. Co-authored-by: Isaac
kamcheungting-dbforce-pushed
the
logging-block6-registry
branch
from
August 6, 2026 02:01
7ba75ce to
dc3648fCompare
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part 6 of the logging stack (builds on #726). Completes the system with configuration-driven backend selection, mirroring
MetricsReporters.What's here
Loggers::Register(type, factory)registers a named backend;Loggers::Load(properties)builds one, choosing the type from thelogger-implproperty.noop,cerr, and (when built with spdlog)spdlog. With nologger-implset, the default is spdlog when compiled in, else cerr — i.e. logs by default.Loggers::LoadAndSetDefault(properties)builds a logger and installs it as the process default.End-to-end tests —
logging_end_to_end_testdrives the public surface the way an app does: configure a backend + level via properties, install it, log through the macros, and check the real output. Also covers the compiled-in default backend and a macro reaching a real spdlog sink. clang/libc++, spdlog ON and OFF.This pull request and its description were written by Isaac.