Skip to content

Commit 9f1e7e8

Browse files
yingcai-cyyingcai-cy
committed
simplify logging and fix review comments.
1 parent cc320fa commit 9f1e7e8

File tree

4 files changed

+162
-183
lines changed

4 files changed

+162
-183
lines changed

‎src/iceberg/util/logger.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
#include <spdlog/sinks/stdout_color_sinks.h>
2323
#include <spdlog/spdlog.h>
2424

25+
#include "iceberg/util/spdlog_logger.h"
26+
2527
namespace iceberg {
2628

2729
namespace {
@@ -94,14 +96,8 @@ bool SpdlogLogger::ShouldLogImpl(LogLevel level) const noexcept {
9496
return level >= current_level_;
9597
}
9698

97-
void SpdlogLogger::LogRawImpl(LogLevel level, const std::string& message) const {
98-
auto typed_logger = std::static_pointer_cast<spdlog::logger>(logger_);
99-
auto spdlog_level = ToSpdlogLevel(level);
100-
typed_logger->log(spdlog_level, message);
101-
}
102-
103-
void SpdlogLogger::LogWithLocationRawImpl(LogLevel level, const std::string& message,
104-
const std::source_location& location) const {
99+
void SpdlogLogger::LogRawImpl(LogLevel level, const std::source_location& location,
100+
const std::string& message) const {
105101
auto typed_logger = std::static_pointer_cast<spdlog::logger>(logger_);
106102
auto spdlog_level = ToSpdlogLevel(level);
107103

‎src/iceberg/util/logger.h‎

Lines changed: 74 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,15 @@
2020
#pragma once
2121

2222
#include <format>
23+
#include <iostream>
2324
#include <memory>
2425
#include <source_location>
2526
#include <string>
2627
#include <string_view>
2728
#include <type_traits>
2829

30+
#include <iceberg/iceberg_export.h>
31+
2932
namespace iceberg {
3033

3134
/// \brief Log levels for the iceberg logger system
@@ -40,7 +43,7 @@ enum class LogLevel : int {
4043
};
4144

4245
/// \brief Convert log level to string representation
43-
constexpr std::string_view LogLevelToString(LogLevel level) {
46+
ICEBERG_EXPORT constexpr std::string_view LogLevelToString(LogLevel level) {
4447
switch (level) {
4548
case LogLevel::kTrace:
4649
return "TRACE";
@@ -64,148 +67,51 @@ constexpr std::string_view LogLevelToString(LogLevel level) {
6467
/// \brief Logger interface that uses CRTP to avoid virtual function overhead
6568
/// \tparam Derived The concrete logger implementation
6669
template <typename Derived>
67-
class LoggerInterface {
70+
class ICEBERG_EXPORT LoggerInterface {
6871
public:
6972
/// \brief Check if a log level is enabled
7073
bool ShouldLog(LogLevel level) const noexcept {
71-
return static_cast<const Derived*>(this)->ShouldLogImpl(level);
74+
return derived()->ShouldLogImpl(level);
7275
}
7376

7477
/// \brief Log a message with the specified level
7578
template <typename... Args>
76-
void Log(LogLevel level, std::string_view format_str, Args&&... args) const {
77-
if (ShouldLog(level)) {
78-
static_cast<const Derived*>(this)->LogImpl(level, format_str,
79-
std::forward<Args>(args)...);
80-
}
81-
}
82-
83-
/// \brief Log a message with source location information
84-
template <typename... Args>
85-
void LogWithLocation(LogLevel level, std::string_view format_str,
86-
const std::source_location& location, Args&&... args) const {
87-
if (ShouldLog(level)) {
88-
static_cast<const Derived*>(this)->LogWithLocationImpl(level, format_str, location,
89-
std::forward<Args>(args)...);
90-
}
79+
void Log(LogLevel level, const std::source_location& location,
80+
std::string_view format_str, Args&&... args) const {
81+
derived()->LogImpl(level, location, format_str, std::forward<Args>(args)...);
9182
}
9283

9384
/// \brief Set the minimum log level
94-
void SetLevel(LogLevel level) { static_cast<Derived*>(this)->SetLevelImpl(level); }
85+
void SetLevel(LogLevel level) { derived()->SetLevelImpl(level); }
9586

9687
/// \brief Get the current minimum log level
97-
LogLevel GetLevel() const noexcept {
98-
return static_cast<const Derived*>(this)->GetLevelImpl();
99-
}
100-
101-
// Convenience methods for different log levels
102-
template <typename... Args>
103-
void Trace(std::string_view format_str, Args&&... args) const {
104-
Log(LogLevel::kTrace, format_str, std::forward<Args>(args)...);
105-
}
106-
107-
template <typename... Args>
108-
void Debug(std::string_view format_str, Args&&... args) const {
109-
Log(LogLevel::kDebug, format_str, std::forward<Args>(args)...);
110-
}
111-
112-
template <typename... Args>
113-
void Info(std::string_view format_str, Args&&... args) const {
114-
Log(LogLevel::kInfo, format_str, std::forward<Args>(args)...);
115-
}
116-
117-
template <typename... Args>
118-
void Warn(std::string_view format_str, Args&&... args) const {
119-
Log(LogLevel::kWarn, format_str, std::forward<Args>(args)...);
120-
}
121-
122-
template <typename... Args>
123-
void Error(std::string_view format_str, Args&&... args) const {
124-
Log(LogLevel::kError, format_str, std::forward<Args>(args)...);
125-
}
126-
127-
template <typename... Args>
128-
void Critical(std::string_view format_str, Args&&... args) const {
129-
Log(LogLevel::kCritical, format_str, std::forward<Args>(args)...);
130-
}
88+
LogLevel GetLevel() const noexcept { return derived()->GetLevelImpl(); }
13189

13290
protected:
13391
LoggerInterface() = default;
13492
~LoggerInterface() = default;
135-
};
136-
137-
/// \brief Concept to constrain types that implement the Logger interface
138-
template <typename T>
139-
concept Logger = std::is_base_of_v<LoggerInterface<T>, T>;
140-
141-
/// \brief Default spdlog-based logger implementation
142-
class SpdlogLogger : public LoggerInterface<SpdlogLogger> {
143-
public:
144-
/// \brief Create a new spdlog logger with the given name
145-
explicit SpdlogLogger(std::string_view logger_name = "iceberg");
146-
147-
/// \brief Create a spdlog logger from an existing spdlog::logger
148-
explicit SpdlogLogger(std::shared_ptr<void> spdlog_logger);
149-
150-
// Implementation methods required by LoggerInterface
151-
bool ShouldLogImpl(LogLevel level) const noexcept;
152-
153-
template <typename... Args>
154-
void LogImpl(LogLevel level, std::string_view format_str, Args&&... args) const {
155-
if constexpr (sizeof...(args) > 0) {
156-
std::string formatted_message =
157-
std::vformat(format_str, std::make_format_args(args...));
158-
LogRawImpl(level, formatted_message);
159-
} else {
160-
LogRawImpl(level, std::string(format_str));
161-
}
162-
}
163-
164-
template <typename... Args>
165-
void LogWithLocationImpl(LogLevel level, std::string_view format_str,
166-
const std::source_location& location, Args&&... args) const {
167-
std::string message;
168-
if constexpr (sizeof...(args) > 0) {
169-
message = std::vformat(format_str, std::make_format_args(args...));
170-
} else {
171-
message = std::string(format_str);
172-
}
173-
LogWithLocationRawImpl(level, message, location);
174-
}
175-
176-
void SetLevelImpl(LogLevel level);
177-
LogLevel GetLevelImpl() const noexcept;
178-
179-
/// \brief Get the underlying spdlog logger (for advanced usage)
180-
std::shared_ptr<void> GetUnderlyingLogger() const { return logger_; }
18193

18294
private:
183-
void LogRawImpl(LogLevel level, const std::string& message) const;
184-
void LogWithLocationRawImpl(LogLevel level, const std::string& message,
185-
const std::source_location& location) const;
95+
/// \brief Get const pointer to the derived class instance
96+
const Derived* derived() const noexcept { return static_cast<const Derived*>(this); }
18697

187-
std::shared_ptr<void> logger_; // Type-erased spdlog::logger
188-
LogLevel current_level_ = LogLevel::kInfo;
98+
/// \brief Get non-const pointer to the derived class instance
99+
Derived* derived() noexcept { return static_cast<Derived*>(this); }
189100
};
190101

191-
/// \brief No-op logger implementation for performance-critical scenarios
192-
class NullLogger : public LoggerInterface<NullLogger> {
193-
public:
194-
bool ShouldLogImpl(LogLevel) const noexcept { return false; }
195-
196-
template <typename... Args>
197-
void LogImpl(LogLevel, std::string_view, Args&&...) const noexcept {}
198-
199-
template <typename... Args>
200-
void LogWithLocationImpl(LogLevel, std::string_view, const std::source_location&,
201-
Args&&...) const noexcept {}
202-
203-
void SetLevelImpl(LogLevel) noexcept {}
204-
LogLevel GetLevelImpl() const noexcept { return LogLevel::kOff; }
205-
};
102+
/// \brief Concept to constrain types that implement the Logger interface
103+
template <typename T>
104+
concept Logger = requires(const T& t, T& nt, LogLevel level,
105+
const std::source_location& location,
106+
std::string_view format_str) {
107+
{ t.ShouldLogImpl(level) } -> std::convertible_to<bool>;
108+
{ t.LogImpl(level, location, format_str) } -> std::same_as<void>;
109+
{ nt.SetLevelImpl(level) } -> std::same_as<void>;
110+
{ t.GetLevelImpl() } -> std::convertible_to<LogLevel>;
111+
} && std::is_base_of_v<LoggerInterface<T>, T>;
206112

207113
/// \brief Global logger registry for managing logger instances
208-
class LoggerRegistry {
114+
class ICEBERG_EXPORT LoggerRegistry {
209115
public:
210116
/// \brief Get the singleton instance of the logger registry
211117
static LoggerRegistry& Instance();
@@ -217,11 +123,16 @@ class LoggerRegistry {
217123
template <Logger LoggerImpl>
218124
void SetDefaultLogger(std::shared_ptr<LoggerImpl> logger) {
219125
default_logger_ = std::static_pointer_cast<void>(logger);
220-
log_func_ = [](const void* logger_ptr, LogLevel level, const std::string& message) {
126+
log_func_ = [](const void* logger_ptr, LogLevel level,
127+
const std::source_location& location, std::string_view format_str,
128+
std::format_args args) {
221129
auto* typed_logger = static_cast<const LoggerImpl*>(logger_ptr);
222-
if (typed_logger->ShouldLog(level)) {
223-
typed_logger->LogImpl(level, "{}", message);
224-
}
130+
std::string formatted_message = std::vformat(format_str, args);
131+
typed_logger->Log(level, location, "{}", formatted_message);
132+
};
133+
should_log_func_ = [](const void* logger_ptr, LogLevel level) -> bool {
134+
auto* typed_logger = static_cast<const LoggerImpl*>(logger_ptr);
135+
return typed_logger->ShouldLog(level);
225136
};
226137
}
227138

@@ -236,15 +147,22 @@ class LoggerRegistry {
236147

237148
/// \brief Log using the default logger
238149
template <typename... Args>
239-
void Log(LogLevel level, std::string_view format_str, Args&&... args) const {
240-
if (default_logger_ && log_func_) {
241-
std::string formatted_message;
242-
if constexpr (sizeof...(args) > 0) {
243-
formatted_message = std::vformat(format_str, std::make_format_args(args...));
244-
} else {
245-
formatted_message = std::string(format_str);
150+
void Log(LogLevel level, const std::source_location& location,
151+
std::string_view format_str, Args&&... args) const {
152+
if (default_logger_ && should_log_func_ && log_func_) {
153+
if (should_log_func_(default_logger_.get(), level)) {
154+
try {
155+
if constexpr (sizeof...(args) > 0) {
156+
auto args_store = std::make_format_args(args...);
157+
log_func_(default_logger_.get(), level, location, format_str, args_store);
158+
} else {
159+
log_func_(default_logger_.get(), level, location, format_str,
160+
std::format_args{});
161+
}
162+
} catch (const std::exception& e) {
163+
std::cerr << "Logging error: " << e.what() << std::endl;
164+
}
246165
}
247-
log_func_(default_logger_.get(), level, formatted_message);
248166
}
249167
}
250168

@@ -255,32 +173,39 @@ class LoggerRegistry {
255173
LoggerRegistry() = default;
256174

257175
std::shared_ptr<void> default_logger_;
258-
void (*log_func_)(const void*, LogLevel, const std::string&) = nullptr;
176+
void (*log_func_)(const void*, LogLevel, const std::source_location&, std::string_view,
177+
std::format_args) = nullptr;
178+
bool (*should_log_func_)(const void*, LogLevel) = nullptr;
259179
};
260180

261181
/// \brief Convenience macros for logging with automatic source location
262-
#define LOG_WITH_LOCATION(level, format_str, ...) \
182+
#define ICEBERG_LOG_WITH_LOCATION(level, format_str, ...) \
263183
do { \
264-
::iceberg::LoggerRegistry::Instance().Log(level, \
184+
::iceberg::LoggerRegistry::Instance().Log(level, std::source_location::current(), \
265185
format_str __VA_OPT__(, ) __VA_ARGS__); \
266-
} while (0)
267-
268-
#define LOG_TRACE(format_str, ...) \
269-
LOG_WITH_LOCATION(::iceberg::LogLevel::kTrace, format_str __VA_OPT__(, ) __VA_ARGS__)
186+
} while (false)
270187

271-
#define LOG_DEBUG(format_str, ...) \
272-
LOG_WITH_LOCATION(::iceberg::LogLevel::kDebug, format_str __VA_OPT__(, ) __VA_ARGS__)
188+
#define ICEBERG_LOG_TRACE(format_str, ...) \
189+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kTrace, \
190+
format_str __VA_OPT__(, ) __VA_ARGS__)
273191

274-
#define LOG_INFO(format_str, ...) \
275-
LOG_WITH_LOCATION(::iceberg::LogLevel::kInfo, format_str __VA_OPT__(, ) __VA_ARGS__)
192+
#define ICEBERG_LOG_DEBUG(format_str, ...) \
193+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kDebug, \
194+
format_str __VA_OPT__(, ) __VA_ARGS__)
276195

277-
#define LOG_WARN(format_str, ...) \
278-
LOG_WITH_LOCATION(::iceberg::LogLevel::kWarn, format_str __VA_OPT__(, ) __VA_ARGS__)
196+
#define ICEBERG_LOG_INFO(format_str, ...) \
197+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kInfo, \
198+
format_str __VA_OPT__(, ) __VA_ARGS__)
279199

280-
#define LOG_ERROR(format_str, ...) \
281-
LOG_WITH_LOCATION(::iceberg::LogLevel::kError, format_str __VA_OPT__(, ) __VA_ARGS__)
200+
#define ICEBERG_LOG_WARN(format_str, ...) \
201+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kWarn, \
202+
format_str __VA_OPT__(, ) __VA_ARGS__)
282203

283-
#define LOG_CRITICAL(format_str, ...) \
284-
LOG_WITH_LOCATION(::iceberg::LogLevel::kCritical, format_str __VA_OPT__(, ) __VA_ARGS__)
204+
#define ICEBERG_LOG_ERROR(format_str, ...) \
205+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kError, \
206+
format_str __VA_OPT__(, ) __VA_ARGS__)
285207

208+
#define ICEBERG_LOG_CRITICAL(format_str, ...) \
209+
ICEBERG_LOG_WITH_LOCATION(::iceberg::LogLevel::kCritical, \
210+
format_str __VA_OPT__(, ) __VA_ARGS__)
286211
} // namespace iceberg

0 commit comments

Comments
 (0)