From 66f29f2d2a4c679031bde0b83be660eafe12cd7c Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 15 Jun 2026 10:56:03 +0000 Subject: [PATCH 1/3] feat(logging): add Loggers registry (6/6) Final 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 --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/logging/loggers.cc | 147 +++++++++++++++++ src/iceberg/logging/loggers.h | 68 ++++++++ src/iceberg/logging/meson.build | 1 + src/iceberg/meson.build | 1 + src/iceberg/test/CMakeLists.txt | 2 + src/iceberg/test/loggers_test.cc | 155 ++++++++++++++++++ src/iceberg/test/logging_end_to_end_test.cc | 168 ++++++++++++++++++++ src/iceberg/test/meson.build | 2 + 9 files changed, 545 insertions(+) create mode 100644 src/iceberg/logging/loggers.cc create mode 100644 src/iceberg/logging/loggers.h create mode 100644 src/iceberg/test/loggers_test.cc create mode 100644 src/iceberg/test/logging_end_to_end_test.cc diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index dec79ada4..c366c2e6a 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -56,6 +56,7 @@ set(ICEBERG_SOURCES location_provider.cc logging/cerr_logger.cc logging/logger.cc + logging/loggers.cc logging/spdlog_logger.cc manifest/manifest_adapter.cc manifest/manifest_entry.cc diff --git a/src/iceberg/logging/loggers.cc b/src/iceberg/logging/loggers.cc new file mode 100644 index 000000000..3a12e5047 --- /dev/null +++ b/src/iceberg/logging/loggers.cc @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/logging/loggers.h" + +#include +#include +#include +#include +#include +#include +#include + +// Build-generated, .cc-only. Defines ICEBERG_HAS_SPDLOG; tested with #ifdef. +#include "iceberg/logging/cerr_logger.h" +#include "iceberg/logging/config.h" +#include "iceberg/util/macros.h" +#ifdef ICEBERG_HAS_SPDLOG +# include "iceberg/logging/spdlog_logger_internal.h" +#endif + +namespace iceberg { + +namespace { + +/// \brief Registry-constructible no-op logger (Load returns unique_ptr). +class NoopLogger final : public Logger { + public: + bool ShouldLog(LogLevel /*level*/) const noexcept override { return false; } + void Log(LogMessage&& /*message*/) noexcept override {} + void SetLevel(LogLevel /*level*/) noexcept override {} + LogLevel level() const noexcept override { return LogLevel::kOff; } + bool IsNoop() const override { return true; } +}; + +/// \brief Extract the logger type, defaulting to the compiled-in backend. +std::string InferLoggerType( + const std::unordered_map& properties) { + auto it = properties.find(std::string(kLoggerImpl)); + if (it != properties.end() && !it->second.empty()) { + return it->second; + } +#ifdef ICEBERG_HAS_SPDLOG + return std::string(kLoggerTypeSpdlog); +#else + return std::string(kLoggerTypeCerr); +#endif +} + +struct LoggerRegistryState { + std::shared_mutex mtx; + std::unordered_map map; +}; + +LoggerRegistryState& GetRegistry() { + static auto* state = + new LoggerRegistryState{.map = { + {std::string(kLoggerTypeNoop), + [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }}, + {std::string(kLoggerTypeCerr), + [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }}, +#ifdef ICEBERG_HAS_SPDLOG + {std::string(kLoggerTypeSpdlog), + [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }}, +#endif + }}; + return *state; +} + +} // namespace + +Status Loggers::Register(std::string_view logger_type, LoggerFactory factory) { + if (!factory) { + return InvalidArgument("Logger factory for '{}' must not be empty", logger_type); + } + auto& registry = GetRegistry(); + std::unique_lock lock(registry.mtx); + registry.map[std::string(logger_type)] = std::move(factory); + return {}; +} + +Result> Loggers::Load( + const std::unordered_map& properties) { + std::string logger_type = InferLoggerType(properties); + + LoggerFactory factory; + { + auto& registry = GetRegistry(); + std::shared_lock lock(registry.mtx); + auto it = registry.map.find(logger_type); + if (it == registry.map.end()) { + return InvalidArgument( + "Unknown logger type '{}'. Register a factory with Loggers::Register() " + "before using this type.", + logger_type); + } + factory = it->second; + } + + try { + ICEBERG_ASSIGN_OR_RAISE(auto logger, factory(properties)); + if (!logger) { + return InvalidArgument("Logger factory for '{}' returned null", logger_type); + } + ICEBERG_RETURN_UNEXPECTED(logger->Initialize(properties)); + return logger; + } catch (const std::exception& ex) { + return InvalidArgument("Logger factory for '{}' failed: {}", logger_type, ex.what()); + } catch (...) { + return InvalidArgument("Logger factory for '{}' failed with unknown exception", + logger_type); + } +} + +Status Loggers::LoadAndSetDefault( + const std::unordered_map& properties) { + ICEBERG_ASSIGN_OR_RAISE(auto logger, Load(properties)); + SetDefaultLogger(std::shared_ptr(std::move(logger))); + return {}; +} + +} // namespace iceberg diff --git a/src/iceberg/logging/loggers.h b/src/iceberg/logging/loggers.h new file mode 100644 index 000000000..36ccab1d5 --- /dev/null +++ b/src/iceberg/logging/loggers.h @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/logging/loggers.h +/// \brief Property-driven registry/factory for Logger backends. + +#include +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/logging/logger.h" +#include "iceberg/result.h" + +namespace iceberg { + +/// \brief Property key selecting the logger implementation. +constexpr std::string_view kLoggerImpl = "logger-impl"; +/// \brief Built-in logger type identifiers. +constexpr std::string_view kLoggerTypeNoop = "noop"; +constexpr std::string_view kLoggerTypeCerr = "cerr"; +constexpr std::string_view kLoggerTypeSpdlog = "spdlog"; + +/// \brief Factory constructing a Logger from catalog-style properties. +using LoggerFactory = std::function>( + const std::unordered_map& properties)>; + +/// \brief Registry of logger factories, mirroring MetricsReporters. +/// +/// Built-in factories: "noop", "cerr", and (only when built with ICEBERG_SPDLOG) +/// "spdlog". When the "logger-impl" property is absent, the default is "spdlog" +/// if compiled in, otherwise "cerr" -- an intentional divergence from the metrics +/// registry's noop default (we want logs by default). +class ICEBERG_EXPORT Loggers { + public: + /// \brief Construct and initialize a logger from properties. + static Result> Load( + const std::unordered_map& properties); + + /// \brief Register a factory for \p logger_type (overwrites any existing). + static Status Register(std::string_view logger_type, LoggerFactory factory); + + /// \brief Load a logger from properties and install it as the default. + static Status LoadAndSetDefault( + const std::unordered_map& properties); +}; + +} // namespace iceberg diff --git a/src/iceberg/logging/meson.build b/src/iceberg/logging/meson.build index 04aec7d32..d4bf258ff 100644 --- a/src/iceberg/logging/meson.build +++ b/src/iceberg/logging/meson.build @@ -29,6 +29,7 @@ install_headers( 'log_level.h', 'log_macros.h', 'logger.h', + 'loggers.h', 'short_log_macros.h', ], subdir: 'iceberg/logging', diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 8f5680a99..a94b27198 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -108,6 +108,7 @@ iceberg_sources = files( 'location_provider.cc', 'logging/cerr_logger.cc', 'logging/logger.cc', + 'logging/loggers.cc', 'logging/spdlog_logger.cc', 'manifest/manifest_adapter.cc', 'manifest/manifest_entry.cc', diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 1181a722e..dde109bb5 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -96,6 +96,8 @@ add_iceberg_test(logging_test cerr_logger_test.cc log_level_test.cc logger_test.cc + loggers_test.cc + logging_end_to_end_test.cc macros_active_level_test.cc macros_test.cc spdlog_logger_test.cc) diff --git a/src/iceberg/test/loggers_test.cc b/src/iceberg/test/loggers_test.cc new file mode 100644 index 000000000..f75dd64e5 --- /dev/null +++ b/src/iceberg/test/loggers_test.cc @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/logging/loggers.h" + +#include +#include +#include +#include + +#include + +// Build-generated, test-only: gates the spdlog-by-property expectation. +#include "iceberg/logging/config.h" +#include "iceberg/logging/log_level.h" +#include "iceberg/logging/logger.h" +#include "iceberg/test/logging_test_helpers.h" + +namespace iceberg { + +TEST(LoggersTest, LoadDefaultReturnsNonNullNonNoop) { + auto result = Loggers::Load({}); + ASSERT_TRUE(result.has_value()); + ASSERT_NE(result.value(), nullptr); + // The default backend (spdlog or cerr) is a real sink, never the no-op. + EXPECT_FALSE(result.value()->IsNoop()); +} + +TEST(LoggersTest, LoadNoopByProperty) { + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeNoop)}}); + ASSERT_TRUE(result.has_value()); + EXPECT_TRUE(result.value()->IsNoop()); +} + +TEST(LoggersTest, LoadCerrByProperty) { + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}}); + ASSERT_TRUE(result.has_value()); + ASSERT_NE(result.value(), nullptr); + EXPECT_FALSE(result.value()->IsNoop()); +} + +TEST(LoggersTest, UnknownTypeIsAnError) { + auto result = + Loggers::Load({{std::string(kLoggerImpl), std::string("does-not-exist")}}); + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().kind, ErrorKind::kInvalidArgument); +} + +TEST(LoggersTest, RegisterCustomFactoryThenLoad) { + auto status = Loggers::Register("capturing", + [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }); + ASSERT_TRUE(status.has_value()); + + auto result = Loggers::Load({{std::string(kLoggerImpl), "capturing"}}); + ASSERT_TRUE(result.has_value()); + EXPECT_NE(dynamic_cast(result.value().get()), nullptr); +} + +TEST(LoggersTest, RegisterRejectsEmptyFactory) { + auto status = Loggers::Register("bad", LoggerFactory{}); + ASSERT_FALSE(status.has_value()); + EXPECT_EQ(status.error().kind, ErrorKind::kInvalidArgument); +} + +TEST(LoggersTest, LoadAndSetDefaultInstallsLogger) { + auto previous = GetDefaultLogger(); + auto status = Loggers::LoadAndSetDefault( + {{std::string(kLoggerImpl), std::string(kLoggerTypeNoop)}}); + ASSERT_TRUE(status.has_value()); + EXPECT_TRUE(GetDefaultLogger()->IsNoop()); + SetDefaultLogger(previous); // restore +} + +TEST(LoggersTest, LoadAppliesLevelProperty) { + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}, + {std::string(kLevelProperty), std::string("error")}}); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result.value()->level(), LogLevel::kError); +} + +TEST(LoggersTest, LoadRejectsInvalidLevelProperty) { + auto result = + Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}, + {std::string(kLevelProperty), std::string("not-a-level")}}); + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().kind, ErrorKind::kInvalidArgument); +} + +// Registering the same type twice replaces the factory rather than failing, so a +// later Register wins. This pins the documented last-one-wins behavior. +TEST(LoggersTest, RegisterSameTypeTwiceReplacesFactory) { + constexpr std::string_view kType = "replaceable-test-logger"; + ASSERT_TRUE( + Loggers::Register(kType, [](const auto&) -> Result> { + return std::make_unique(); + }).has_value()); + + // Second registration of the same key must succeed and take effect. + ASSERT_TRUE( + Loggers::Register(kType, [](const auto&) -> Result> { + auto logger = std::make_unique(); + logger->SetLevel(LogLevel::kError); // distinguishes the 2nd factory + return logger; + }).has_value()); + + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kType)}}); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ((*result)->level(), LogLevel::kError); +} + +// The spdlog backend is reachable through the registry by property when compiled +// in; when it is not, the type is simply unknown. Either way Load must not crash. +TEST(LoggersTest, LoadSpdlogByPropertyWhenCompiledIn) { + auto result = + Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeSpdlog)}}); +#ifdef ICEBERG_HAS_SPDLOG + ASSERT_TRUE(result.has_value()); + EXPECT_NE(*result, nullptr); + EXPECT_FALSE((*result)->IsNoop()); +#else + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().kind, ErrorKind::kInvalidArgument); +#endif +} + +// A "pattern" property routed through the registry reaches the sink's Initialize. +// CerrLogger has a fixed layout and must ignore it without erroring. +TEST(LoggersTest, LoadPassesPatternPropertyToSink) { + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}, + {std::string(kPatternProperty), std::string("%v")}, + {std::string(kLevelProperty), std::string("warn")}}); + ASSERT_TRUE(result.has_value()); + EXPECT_EQ((*result)->level(), LogLevel::kWarn); // level still applied +} + +} // namespace iceberg diff --git a/src/iceberg/test/logging_end_to_end_test.cc b/src/iceberg/test/logging_end_to_end_test.cc new file mode 100644 index 000000000..98ce7ef65 --- /dev/null +++ b/src/iceberg/test/logging_end_to_end_test.cc @@ -0,0 +1,168 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// End-to-end tests: exercise the public surface the way an application does -- +// configure/install a real backend via the registry, log through the LOG_* +// macros, and observe the actual output. The per-layer unit tests cover each +// piece in isolation against a fake; these cover the seams between them. + +// Internal/build-generated header is acceptable in a test TU (not installed). +#include +#include +#include +#include + +#include + +#include "iceberg/logging/cerr_logger.h" +#include "iceberg/logging/config.h" +#include "iceberg/logging/log_level.h" +#include "iceberg/logging/log_macros.h" +#include "iceberg/logging/logger.h" +#include "iceberg/logging/loggers.h" +#include "iceberg/test/logging_test_helpers.h" + +#ifdef ICEBERG_HAS_SPDLOG +# include +# include + +# include "iceberg/logging/spdlog_logger_internal.h" +#endif + +namespace iceberg { + +namespace { + +/// \brief RAII redirect of std::cerr to a stringstream for the test scope. +class CerrCapture { + public: + CerrCapture() : old_(std::cerr.rdbuf(buffer_.rdbuf())) {} + ~CerrCapture() { std::cerr.rdbuf(old_); } + std::string str() const { return buffer_.str(); } + + private: + std::ostringstream buffer_; + std::streambuf* old_; +}; + +} // namespace + +// Configure CerrLogger through the registry, install it as the process default, +// then log via a macro and observe the formatted line on std::cerr -- the full +// registry -> default-slot -> macro -> Emit -> backend -> output path. +TEST(LoggingEndToEndTest, ConfiguredCerrLoggerEmitsFormattedLineThroughMacro) { + ScopedDefaultLogger guard(GetDefaultLogger()); // save + restore the default + auto status = Loggers::LoadAndSetDefault( + {{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}}); + ASSERT_TRUE(status.has_value()); + + std::string out; + { + CerrCapture capture; + ICEBERG_LOG_WARN("u={}", 7); + out = capture.str(); + } + EXPECT_NE(out.find("warn"), std::string::npos); + EXPECT_NE(out.find("u=7"), std::string::npos); + EXPECT_NE(out.find("logging_end_to_end_test.cc"), std::string::npos); + EXPECT_EQ(out.back(), '\n'); +} + +// The level set on the installed default logger gates emission decided through +// the whole macro path (not just a direct ShouldLog() call). +TEST(LoggingEndToEndTest, InstalledLevelFiltersThroughFullMacroPath) { + ScopedDefaultLogger guard(GetDefaultLogger()); + auto status = Loggers::LoadAndSetDefault( + {{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}}); + ASSERT_TRUE(status.has_value()); + SetDefaultLevel(LogLevel::kError); + + { + CerrCapture capture; + ICEBERG_LOG_INFO("dropped {}", 1); + EXPECT_TRUE(capture.str().empty()); + } + { + CerrCapture capture; + ICEBERG_LOG_ERROR("kept {}", 2); + EXPECT_NE(capture.str().find("kept 2"), std::string::npos); + } +} + +// The "level" property set at configuration time gates emission through the full +// registry -> Initialize -> default-slot -> macro path. +TEST(LoggingEndToEndTest, ConfiguredLevelByPropertyFiltersThroughMacro) { + ScopedDefaultLogger guard(GetDefaultLogger()); + auto status = Loggers::LoadAndSetDefault( + {{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}, + {std::string(kLevelProperty), std::string("error")}}); + ASSERT_TRUE(status.has_value()); + + { + CerrCapture capture; + ICEBERG_LOG_INFO("dropped {}", 1); + EXPECT_TRUE(capture.str().empty()); + } + { + CerrCapture capture; + ICEBERG_LOG_ERROR("kept {}", 2); + EXPECT_NE(capture.str().find("kept 2"), std::string::npos); + } +} + +// The process default with no configuration is a real sink (never the no-op), +// and is the backend the build was compiled with: spdlog when ICEBERG_SPDLOG is +// ON, otherwise the std::cerr logger. +TEST(LoggingEndToEndTest, DefaultLoggerIsTheCompiledBackend) { + auto def = GetDefaultLogger(); + ASSERT_NE(def, nullptr); + EXPECT_FALSE(def->IsNoop()); +#ifdef ICEBERG_HAS_SPDLOG + EXPECT_NE(dynamic_cast(def.get()), nullptr); +#else + EXPECT_NE(dynamic_cast(def.get()), nullptr); +#endif +} + +#ifdef ICEBERG_HAS_SPDLOG +// The "spdlog" registry type resolves to the spdlog-backed sink by name. +TEST(LoggingEndToEndTest, SpdlogFactoryLoadsByName) { + auto result = + Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeSpdlog)}}); + ASSERT_TRUE(result.has_value()); + ASSERT_NE(result.value(), nullptr); + EXPECT_FALSE(result.value()->IsNoop()); + EXPECT_NE(dynamic_cast(result.value().get()), nullptr); +} + +// A macro statement reaches a real spdlog sink: install a SpdLogger backed by an +// ostream sink as the default, log through the macro, and observe the output. +TEST(LoggingEndToEndTest, MacroLogsThroughRealSpdLogger) { + std::ostringstream out; + auto sink = std::make_shared(out); + ScopedDefaultLogger guard(std::make_shared( + spdlog::logger("e2e", std::move(sink)), LogLevel::kTrace)); + + ICEBERG_LOG_INFO("v={}", 9); + GetDefaultLogger()->Flush(); + EXPECT_NE(out.str().find("v=9"), std::string::npos); +} +#endif // ICEBERG_HAS_SPDLOG + +} // namespace iceberg diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 6dde45ee9..3c2569ae4 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -68,6 +68,8 @@ iceberg_tests = { 'cerr_logger_test.cc', 'log_level_test.cc', 'logger_test.cc', + 'loggers_test.cc', + 'logging_end_to_end_test.cc', 'macros_active_level_test.cc', 'macros_test.cc', 'spdlog_logger_test.cc', From 1136d749ab791e72209f767da1a7e29bf5983854 Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 17 Aug 2026 09:15:52 +0000 Subject: [PATCH 2/3] docs(logging): explain running the logger factory outside the registry lock Co-authored-by: Isaac --- src/iceberg/logging/loggers.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/iceberg/logging/loggers.cc b/src/iceberg/logging/loggers.cc index 3a12e5047..bd919583a 100644 --- a/src/iceberg/logging/loggers.cc +++ b/src/iceberg/logging/loggers.cc @@ -123,6 +123,9 @@ Result> Loggers::Load( } try { + // Run the (user-supplied) factory outside the registry lock so it cannot + // deadlock or re-enter the registry; the try/catch turns a throwing factory + // into an error instead of propagating. ICEBERG_ASSIGN_OR_RAISE(auto logger, factory(properties)); if (!logger) { return InvalidArgument("Logger factory for '{}' returned null", logger_type); From ba0199fd2a636683bec183313ff28142fb4d7d85 Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 17 Aug 2026 09:21:00 +0000 Subject: [PATCH 3/3] refactor(logging): dedup NoopLogger via internal::MakeNoopLogger loggers.cc had its own copy of NoopLogger (identical to logger.cc's) because the registry factory returns unique_ptr while Logger::Noop() hands out a shared singleton. Expose internal::MakeNoopLogger() returning unique_ptr, used by both Logger::Noop() and the "noop" factory, and delete the duplicate class. Co-authored-by: Isaac --- src/iceberg/logging/logger.cc | 4 +++- src/iceberg/logging/logger.h | 4 ++++ src/iceberg/logging/loggers.cc | 12 +----------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/iceberg/logging/logger.cc b/src/iceberg/logging/logger.cc index f1cb8488e..ec8ff8c9c 100644 --- a/src/iceberg/logging/logger.cc +++ b/src/iceberg/logging/logger.cc @@ -83,7 +83,7 @@ struct ThreadCache { std::shared_ptr Logger::Noop() { // Intentionally leaked: reachable via the function-local static (LSan-clean) // and never destroyed, so logging during static teardown stays safe. - static auto* instance = new std::shared_ptr(std::make_shared()); + static auto* instance = new std::shared_ptr(internal::MakeNoopLogger()); return *instance; } @@ -141,6 +141,8 @@ FatalHandler GetFatalHandler() { namespace internal { +std::unique_ptr MakeNoopLogger() { return std::make_unique(); } + namespace { /// \brief The one place the per-thread cache's lifetime is managed; shared by diff --git a/src/iceberg/logging/logger.h b/src/iceberg/logging/logger.h index 0a9647f66..8575158ca 100644 --- a/src/iceberg/logging/logger.h +++ b/src/iceberg/logging/logger.h @@ -296,6 +296,10 @@ class ICEBERG_EXPORT ScopedLogger { namespace internal { +/// \brief Construct a fresh no-op logger. Shared by Logger::Noop() (which caches a +/// single instance) and the "noop" registry factory (which needs an owned one). +ICEBERG_EXPORT std::unique_ptr MakeNoopLogger(); + /// \brief Hot-path accessor for the default logger. /// /// Returns a reference to a thread-local cached shared_ptr that is refreshed diff --git a/src/iceberg/logging/loggers.cc b/src/iceberg/logging/loggers.cc index bd919583a..f28edfd60 100644 --- a/src/iceberg/logging/loggers.cc +++ b/src/iceberg/logging/loggers.cc @@ -39,16 +39,6 @@ namespace iceberg { namespace { -/// \brief Registry-constructible no-op logger (Load returns unique_ptr). -class NoopLogger final : public Logger { - public: - bool ShouldLog(LogLevel /*level*/) const noexcept override { return false; } - void Log(LogMessage&& /*message*/) noexcept override {} - void SetLevel(LogLevel /*level*/) noexcept override {} - LogLevel level() const noexcept override { return LogLevel::kOff; } - bool IsNoop() const override { return true; } -}; - /// \brief Extract the logger type, defaulting to the compiled-in backend. std::string InferLoggerType( const std::unordered_map& properties) { @@ -74,7 +64,7 @@ LoggerRegistryState& GetRegistry() { {std::string(kLoggerTypeNoop), [](const std::unordered_map&) -> Result> { - return std::make_unique(); + return internal::MakeNoopLogger(); }}, {std::string(kLoggerTypeCerr), [](const std::unordered_map&)