wgtmac commented on code in PR #726: URL: https://github.com/apache/iceberg-cpp/pull/726#discussion_r3695118425
########## src/iceberg/logging/internal/spdlog_logger.cc: ########## @@ -0,0 +1,107 @@ +/* + * 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/internal/spdlog_logger.h" + +#ifdef ICEBERG_HAS_SPDLOG + +# include <memory> +# include <string> +# include <unordered_map> +# include <utility> + +# include <spdlog/common.h> +# include <spdlog/sinks/stdout_color_sinks.h> + +# include "iceberg/util/macros.h" + +namespace iceberg::internal { + +namespace { + +spdlog::level::level_enum ToSpdLevel(LogLevel level) noexcept { + switch (level) { + case LogLevel::kTrace: + return spdlog::level::trace; + case LogLevel::kDebug: + return spdlog::level::debug; + case LogLevel::kInfo: + return spdlog::level::info; + case LogLevel::kWarn: + return spdlog::level::warn; + case LogLevel::kError: + return spdlog::level::err; + case LogLevel::kCritical: + case LogLevel::kFatal: + // spdlog has no "fatal"; the process abort is owned by the macro layer. + return spdlog::level::critical; + case LogLevel::kOff: + return spdlog::level::off; + } + return spdlog::level::off; +} + +} // namespace + +SpdLogger::SpdLogger(LogLevel level) + : SpdLogger(std::make_shared<spdlog::logger>( + "iceberg", std::make_shared<spdlog::sinks::stderr_color_sink_mt>()), + level) {} + +Status SpdLogger::Initialize( + const std::unordered_map<std::string, std::string>& properties) { + if (auto it = properties.find(std::string(kPatternProperty)); it != properties.end()) { + logger_->set_pattern(it->second); + } + // Apply "level" via the base implementation. + return Logger::Initialize(properties); +} + +SpdLogger::SpdLogger(std::shared_ptr<spdlog::logger> logger, LogLevel level) + : logger_(std::move(logger)), level_(level) { + // logger_ is a hard precondition: SpdLogger is not consumer-constructible (it is + // obtained via the default logger or the "spdlog" registry factory, both of which + // pass a real logger), so Initialize/Log/Flush may dereference it unconditionally. + ICEBERG_DCHECK(logger_ != nullptr, "SpdLogger requires a non-null spdlog::logger"); + logger_->set_level(spdlog::level::trace); // filtering is done by ShouldLog +} + +void SpdLogger::Log(LogMessage&& message) noexcept { + try { + spdlog::source_loc loc{message.location.file_name(), + static_cast<int>(message.location.line()), + message.location.function_name()}; + // Pass the pre-formatted text as an argument ("{}") so any braces in the + // message are not re-interpreted as a format string. + logger_->log(loc, ToSpdLevel(message.level), "{}", message.message); Review Comment: This re-formats an already formatted message through fmt for every emitted record, adding another full copy and possibly an allocation for long messages. Please call the raw-message overload with `spdlog::string_view_t{message.message.data(), message.message.size()}` instead. ########## src/iceberg/logging/internal/spdlog_logger.cc: ########## @@ -0,0 +1,107 @@ +/* + * 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/internal/spdlog_logger.h" + +#ifdef ICEBERG_HAS_SPDLOG + +# include <memory> +# include <string> +# include <unordered_map> +# include <utility> + +# include <spdlog/common.h> +# include <spdlog/sinks/stdout_color_sinks.h> + +# include "iceberg/util/macros.h" + +namespace iceberg::internal { + +namespace { + +spdlog::level::level_enum ToSpdLevel(LogLevel level) noexcept { + switch (level) { + case LogLevel::kTrace: + return spdlog::level::trace; + case LogLevel::kDebug: + return spdlog::level::debug; + case LogLevel::kInfo: + return spdlog::level::info; + case LogLevel::kWarn: + return spdlog::level::warn; + case LogLevel::kError: + return spdlog::level::err; + case LogLevel::kCritical: + case LogLevel::kFatal: + // spdlog has no "fatal"; the process abort is owned by the macro layer. + return spdlog::level::critical; + case LogLevel::kOff: + return spdlog::level::off; + } + return spdlog::level::off; +} + +} // namespace + +SpdLogger::SpdLogger(LogLevel level) + : SpdLogger(std::make_shared<spdlog::logger>( + "iceberg", std::make_shared<spdlog::sinks::stderr_color_sink_mt>()), + level) {} + +Status SpdLogger::Initialize( + const std::unordered_map<std::string, std::string>& properties) { + if (auto it = properties.find(std::string(kPatternProperty)); it != properties.end()) { + logger_->set_pattern(it->second); + } + // Apply "level" via the base implementation. + return Logger::Initialize(properties); +} + +SpdLogger::SpdLogger(std::shared_ptr<spdlog::logger> logger, LogLevel level) + : logger_(std::move(logger)), level_(level) { + // logger_ is a hard precondition: SpdLogger is not consumer-constructible (it is + // obtained via the default logger or the "spdlog" registry factory, both of which + // pass a real logger), so Initialize/Log/Flush may dereference it unconditionally. + ICEBERG_DCHECK(logger_ != nullptr, "SpdLogger requires a non-null spdlog::logger"); Review Comment: `ICEBERG_DCHECK` disappears under `NDEBUG`, and the constructor dereferences `logger_` immediately afterwards. Since `shared_ptr` is nullable and the header only documents the synchronous requirement, an empty input becomes a release crash. Please reject null in a factory or encode the non-null requirement in the API. ########## src/iceberg/logging/meson.build: ########## @@ -15,7 +15,23 @@ # specific language governing permissions and limitations # under the License. +# Generate the .cc-only logging backend config header. The meson build always +# links spdlog, so ICEBERG_HAS_SPDLOG is always defined here. Generated into +# build/src/iceberg/logging/config.h (resolved via include_directories('..'), +# which exposes both the source and build trees); not installed. +logging_config_data = configuration_data() +logging_config_data.set('ICEBERG_HAS_SPDLOG', 1) Review Comment: CMake supports `ICEBERG_SPDLOG=OFF`, but Meson always enables this backend and unconditionally links spdlog. Please add a matching Meson feature option so the no-spdlog/Cerr configuration is available in both supported build systems. ########## src/iceberg/test/spdlog_logger_test.cc: ########## @@ -0,0 +1,106 @@ +/* + * 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. + */ + +// Internal/build-generated header is acceptable in a test TU (not installed). +#include "iceberg/logging/config.h" + +#ifdef ICEBERG_HAS_SPDLOG + +# include <memory> +# include <source_location> +# include <sstream> +# include <string> +# include <unordered_map> + +# include <gtest/gtest.h> +# include <spdlog/logger.h> +# include <spdlog/sinks/ostream_sink.h> + +# include "iceberg/logging/internal/spdlog_logger.h" +# include "iceberg/logging/log_level.h" +# include "iceberg/logging/logger.h" + +namespace iceberg { + +namespace { + +LogMessage MakeMessage(LogLevel level, std::string text) { + return LogMessage{.level = level, + .message = std::move(text), + .location = std::source_location::current(), + .attributes = {}}; +} + +internal::SpdLogger MakeCapturing(std::ostringstream& out, + LogLevel level = LogLevel::kTrace) { + auto sink = std::make_shared<spdlog::sinks::ostream_sink_mt>(out); + auto spd = std::make_shared<spdlog::logger>("test", sink); + return internal::SpdLogger(spd, level); +} + +} // namespace + +TEST(SpdLoggerTest, DefaultLevelIsInfo) { + internal::SpdLogger logger; + EXPECT_EQ(logger.level(), LogLevel::kInfo); + EXPECT_FALSE(logger.ShouldLog(LogLevel::kDebug)); + EXPECT_TRUE(logger.ShouldLog(LogLevel::kError)); +} + +TEST(SpdLoggerTest, ForwardsMessageToSink) { Review Comment: This only verifies the payload, so the advertised source-location forwarding is untested. Please use a `%s:%# %! %v` pattern and assert the file, line, and function fields. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
