szaszm commented on code in PR #1670:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1670#discussion_r1342403137
##########
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp:
##########
@@ -245,8 +245,8 @@ bool
SourceInitiatedSubscriptionListener::Handler::handlePost(CivetServer* /*ser
int xml_buf_size = 0;
ws_xml_dump_memory_node_tree_enc(node, &xml_buf, &xml_buf_size, "UTF-8");
if (xml_buf != nullptr) {
- core::logging::LOG_TRACE(processor_.logger_) << "Received request: \""
<< std::string(xml_buf, xml_buf_size) << "\"";
- ws_xml_free_memory(xml_buf);
+ processor_.logger_->log_trace("Received request: \"{}\"",
std::string_view(xml_buf, xml_buf_size));
+ ws_xml_free_memory(xml_buf);
Review Comment:
Not strictly related to the change, but I'd move the cleanup up and out,
using `gsl::finally`, just to cover the hypothetical case where the logging
throws and the memory is leaked.
##########
extensions/windows-event-log/CollectorInitiatedSubscription.cpp:
##########
@@ -570,25 +574,12 @@ void CollectorInitiatedSubscription::notifyStop() {
}
void CollectorInitiatedSubscription::logError(int line, const std::string&
error) {
- logger_->log_error("Line %d: %s\n", error.c_str());
+ logger_->log_error("Line {}: {}\n", line, error);
}
void CollectorInitiatedSubscription::logWindowsError(int line, const
std::string& info) {
- auto error = GetLastError();
-
- LPVOID lpMsg{};
- FormatMessage(
- FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
- NULL,
- error,
- MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
- (LPTSTR)&lpMsg,
- 0,
- NULL);
-
- logger_->log_error("Line %d: '%s': error %d: %s\n", line, info.c_str(),
static_cast<int>(error), reinterpret_cast<char *>(lpMsg));
-
- LocalFree(lpMsg);
+ auto last_error = utils::OsUtils::windowsErrorToErrorCode(GetLastError());
+ logger_->log_error("Line {}: '{}': error {}: {}", line, info, last_error,
last_error.message());
Review Comment:
:+1:
##########
libminifi/include/utils/ResourceQueue.h:
##########
@@ -108,16 +108,16 @@ class ResourceQueue : public
std::enable_shared_from_this<ResourceQueue<Resource
private:
void returnResource(std::unique_ptr<ResourceType> resource) {
- logDebug("Returning [%p] resource", resource.get());
+ logDebug("Returning [{}] resource", static_cast<void*>(resource.get()));
if (reset_fn_)
reset_fn_.value()(*resource);
internal_queue_.enqueue(std::move(resource));
}
template<typename ...Args>
- void logDebug(const char * const format, Args&& ...args) {
+ void
logDebug(fmt::format_string<std::invoke_result_t<decltype(core::logging::map_args),
Args>...> fmt, Args&& ...args) {
Review Comment:
This particular instance was not changed to use the new type alias
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -234,33 +190,34 @@ class Logger : public BaseLogger {
std::mutex mutex_;
private:
- template<typename ...Args>
- inline void log(spdlog::level::level_enum level, const char* const format,
Args&& ...args) {
+ std::string trimToMaxSizeAndAddId(std::string&& my_string) {
+ auto max_log_size = max_log_size_.load();
+ if (max_log_size >= 0 && my_string.size() >
gsl::narrow<size_t>(max_log_size))
+ my_string = my_string.substr(0, max_log_size);
+ if (auto id = get_id()) {
+ my_string += *id;
+ }
+ return my_string;
+ }
+
+ template<typename ...T>
+ std::string stringify(fmt::format_string<T...> fmt, T&&... args) {
+ auto log_message = fmt::format(fmt, std::forward<T>(args)...);
Review Comment:
```suggestion
auto log_message = fmt::format(std::move(fmt), std::forward<T>(args)...);
```
##########
extensions/standard-processors/processors/TailFile.cpp:
##########
@@ -499,9 +499,9 @@ bool TailFile::getStateFromLegacyStateFile(const
std::shared_ptr<core::ProcessCo
}
void TailFile::logState() {
- logger_->log_info("State of the TailFile processor %s:", name_);
+ logger_->log_info("State of the TailFile processor {}:", name_);
for (const auto& [key, value] : tail_states_) {
- core::logging::LOG_INFO(logger_) << key << " => { " << value << " }";
+ logger_->log_info("key => {{}}", value);
Review Comment:
Did you test this line? I think a double pair of braces is just an escaped
form of a pair of braces, and to actually have it contain the logger value, it
would need a third pair. But I might be wrong.
##########
libminifi/include/core/logging/Logger.h:
##########
@@ -234,33 +190,34 @@ class Logger : public BaseLogger {
std::mutex mutex_;
private:
- template<typename ...Args>
- inline void log(spdlog::level::level_enum level, const char* const format,
Args&& ...args) {
+ std::string trimToMaxSizeAndAddId(std::string&& my_string) {
+ auto max_log_size = max_log_size_.load();
+ if (max_log_size >= 0 && my_string.size() >
gsl::narrow<size_t>(max_log_size))
+ my_string = my_string.substr(0, max_log_size);
+ if (auto id = get_id()) {
+ my_string += *id;
+ }
+ return my_string;
+ }
+
+ template<typename ...T>
+ std::string stringify(fmt::format_string<T...> fmt, T&&... args) {
+ auto log_message = fmt::format(fmt, std::forward<T>(args)...);
+ return trimToMaxSizeAndAddId(std::move(log_message));
+ }
+
+ template<typename ...T>
+ inline void log(spdlog::level::level_enum level,
fmt::format_string<std::invoke_result_t<decltype(map_args), T>...> fmt, T&&
...args) {
if (controller_ && !controller_->is_enabled())
- return;
+ return;
std::lock_guard<std::mutex> lock(mutex_);
if (!delegate_->should_log(level)) {
return;
}
- auto str = format_string(max_log_size_.load(), format,
conditional_stringify(std::forward<Args>(args))...);
- if (const auto id = get_id()) {
- str = str + *id;
- }
- delegate_->log(level, str);
+ delegate_->log(level, stringify(fmt, map_args(std::forward<T>(args))...));
Review Comment:
If we start moving the format string, I'd prefer to move it everywhere.
```suggestion
delegate_->log(level, stringify(std::move(fmt),
map_args(std::forward<T>(args))...));
```
--
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]
