From 913feacb412856561fd75b4fd5c5c77d190339ce Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 15 Jun 2026 10:56:03 +0000 Subject: [PATCH 1/4] 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 2ea4162f9..0a2971f5e 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -57,6 +57,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 4c6f5a362..e99fadaf5 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -109,6 +109,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 8c1d43cfa..67f867a08 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 87b317904..894b3be69 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 fc7839cc0309dd369b70bc9d5d80b4d9186bafa7 Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 17 Aug 2026 09:15:52 +0000 Subject: [PATCH 2/4] 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 8528de031b4c53519565cc6929788ff2e4dadd33 Mon Sep 17 00:00:00 2001 From: Kam Cheung Ting Date: Mon, 17 Aug 2026 09:21:00 +0000 Subject: [PATCH 3/4] 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&) From f2e230acd2eb7c5ba82fbddaa98da7e603371a96 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Fri, 21 Aug 2026 17:57:21 +0800 Subject: [PATCH 4/4] simplify code and fix ci --- src/iceberg/logging/loggers.cc | 41 ++--- src/iceberg/logging/loggers.h | 4 - src/iceberg/test/CMakeLists.txt | 1 - src/iceberg/test/loggers_test.cc | 85 ++++++++-- src/iceberg/test/logging_end_to_end_test.cc | 168 -------------------- src/iceberg/test/meson.build | 1 - 6 files changed, 86 insertions(+), 214 deletions(-) delete mode 100644 src/iceberg/test/logging_end_to_end_test.cc diff --git a/src/iceberg/logging/loggers.cc b/src/iceberg/logging/loggers.cc index f28edfd60..40f15c313 100644 --- a/src/iceberg/logging/loggers.cc +++ b/src/iceberg/logging/loggers.cc @@ -59,26 +59,24 @@ struct LoggerRegistryState { }; LoggerRegistryState& GetRegistry() { - static auto* state = - new LoggerRegistryState{.map = { - {std::string(kLoggerTypeNoop), - [](const std::unordered_map&) - -> Result> { - return internal::MakeNoopLogger(); - }}, - {std::string(kLoggerTypeCerr), - [](const std::unordered_map&) - -> Result> { - return std::make_unique(); - }}, + static auto* state = new LoggerRegistryState{ + .map = { + {std::string(kLoggerTypeNoop), + [](const std::unordered_map&) + -> Result> { return internal::MakeNoopLogger(); }}, + {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(); - }}, + {std::string(kLoggerTypeSpdlog), + [](const std::unordered_map&) + -> Result> { + return std::make_unique(); + }}, #endif - }}; + }}; return *state; } @@ -130,11 +128,4 @@ Result> Loggers::Load( } } -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 index 36ccab1d5..07faca153 100644 --- a/src/iceberg/logging/loggers.h +++ b/src/iceberg/logging/loggers.h @@ -59,10 +59,6 @@ class ICEBERG_EXPORT Loggers { /// \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/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 67f867a08..a5d902c75 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -97,7 +97,6 @@ add_iceberg_test(logging_test 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 index f75dd64e5..3f12e0757 100644 --- a/src/iceberg/test/loggers_test.cc +++ b/src/iceberg/test/loggers_test.cc @@ -19,7 +19,9 @@ #include "iceberg/logging/loggers.h" +#include #include +#include #include #include #include @@ -29,11 +31,34 @@ // Build-generated, test-only: gates the spdlog-by-property expectation. #include "iceberg/logging/config.h" #include "iceberg/logging/log_level.h" +#include "iceberg/logging/log_macros.h" #include "iceberg/logging/logger.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 { + +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 + TEST(LoggersTest, LoadDefaultReturnsNonNullNonNoop) { auto result = Loggers::Load({}); ASSERT_TRUE(result.has_value()); @@ -81,15 +106,6 @@ TEST(LoggersTest, RegisterRejectsEmptyFactory) { 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")}}); @@ -142,14 +158,53 @@ TEST(LoggersTest, LoadSpdlogByPropertyWhenCompiledIn) { #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) { +TEST(LoggingEndToEndTest, ConfiguredCerrLoggerEmitsFormattedLineThroughMacro) { + auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}}); + ASSERT_TRUE(result.has_value()); + ScopedDefaultLogger guard(std::shared_ptr(std::move(result.value()))); + + std::string out; + { + CerrCapture capture; + ICEBERG_LOG_WARN("u={}", 7); + out = capture.str(); + } + ASSERT_FALSE(out.empty()); + EXPECT_NE(out.find("warn"), std::string::npos); + EXPECT_NE(out.find("u=7"), std::string::npos); + EXPECT_NE(out.find("loggers_test.cc"), std::string::npos); + EXPECT_EQ(out.back(), '\n'); +} + +TEST(LoggingEndToEndTest, ConfiguredLevelByPropertyFiltersThroughMacro) { auto result = Loggers::Load({{std::string(kLoggerImpl), std::string(kLoggerTypeCerr)}, - {std::string(kPatternProperty), std::string("%v")}, - {std::string(kLevelProperty), std::string("warn")}}); + {std::string(kLevelProperty), std::string("error")}}); ASSERT_TRUE(result.has_value()); - EXPECT_EQ((*result)->level(), LogLevel::kWarn); // level still applied + ScopedDefaultLogger guard(std::shared_ptr(std::move(result.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); + } +} + +#ifdef ICEBERG_HAS_SPDLOG +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/logging_end_to_end_test.cc b/src/iceberg/test/logging_end_to_end_test.cc deleted file mode 100644 index 98ce7ef65..000000000 --- a/src/iceberg/test/logging_end_to_end_test.cc +++ /dev/null @@ -1,168 +0,0 @@ -/* - * 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 894b3be69..95278e6f8 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -69,7 +69,6 @@ iceberg_tests = { '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',