You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "martinzink (via GitHub)" <gi...@apache.org> on 2024/01/18 13:42:40 UTC
[PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
martinzink opened a new pull request, #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457821386
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
Review Comment:
Makes sense, included this in https://github.com/apache/nifi-minifi-cpp/pull/1718/commits/d1d9008ca3f4edca4cc0cf847978f536581aa3ae
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
Good idea, unfortunetly spdlog doesnt offer that functionality...
However we are doing this from a singleton object (LoggerConfiguration is used as a singleton from prod code), so with this information in mind, I've expanded the already existing pattern of requiring the lock_guard for this type of code to run. (of course this doesnt protect us from malicious code that creates a lock_guard from a seperate mutex, we gotta refactor this whole mess in a more indepth PR in my opinion)
https://github.com/apache/nifi-minifi-cpp/pull/1718/commits/d1d9008ca3f4edca4cc0cf847978f536581aa3ae
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457463615
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -145,40 +143,33 @@ void LoggerConfiguration::initialize(const std::shared_ptr<LoggerProperties> &lo
}
formatter_ = std::make_shared<spdlog::pattern_formatter>(spdlog_pattern);
- std::map<std::string, std::shared_ptr<spdlog::logger>> spdloggers;
- for (auto const & logger_impl : loggers) {
- std::shared_ptr<spdlog::logger> spdlogger;
- auto it = spdloggers.find(logger_impl->name);
- if (it == spdloggers.end()) {
- spdlogger = get_logger(logger_, root_namespace_, logger_impl->name, formatter_, true);
- spdloggers[logger_impl->name] = spdlogger;
- } else {
- spdlogger = it->second;
- }
- logger_impl->set_delegate(spdlogger);
- }
+ spdlog::apply_all([&](auto spd_logger) {
+ setupSpdLogger(spd_logger, root_namespace_, spd_logger->name(), formatter_);
+ });
Review Comment:
This is where the magic happens, everything else is basically a refactor/test fix.
Previously we had a shared_ptr to every LoggerImpl ever created and if we wanted to modify the settings we iterated it and recreated the underlying spdlogger.
Now we rely on the registry in spdlog, so if we want to modify the logger properties we modify the underlying spdlogger without touching the LoggerImpl-s so we dont need a shared_ptr for these.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457944711
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
This caused the LoggerConfigurationTests to fail to compile. I've added a fix, now it builds but its really ugly... (it wasnt much prettier before), but this perfectly highlights the shortcomings of this security measure (not a new thing here) in LoggerConfigurations.
I'd rather not delay this fix, because besides this hack is only used in the test, and these lock protected functions are private/protected...
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1458513484
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
Leaving this unsynchronized and accepting occasional partially applied config changes is also an option. I don't think this is a thread safety issue, I'd expect spdlog to be internally synchronized.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457992373
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
Convinced myself and deleted the test in https://github.com/apache/nifi-minifi-cpp/pull/1718/commits/0f8988bd5e960ef85e9118b1f41d8632ce8876a5
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1718: MINIFICPP-2288 Remove the caching of loggers
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457526803
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
Review Comment:
I'd change this to take a `const std::string&` instead: now we're calling it with a `std::string`, creating a temporary `std::string_view`, only to use it to create a temporary `std::string` that we can pass further.
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
Synchronization may become an issue here. Can we do these atomically on the spdlog logger object?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2288 Remove the caching of loggers [nifi-minifi-cpp]
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1718:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1718#discussion_r1457989914
##########
libminifi/src/core/logging/LoggerConfiguration.cpp:
##########
@@ -267,52 +257,56 @@ std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::initialize_names
return root_namespace;
}
-std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<Logger>& logger, const std::shared_ptr<internal::LoggerNamespace> &root_namespace, std::string_view name_view,
- const std::shared_ptr<spdlog::formatter>& formatter, bool remove_if_present) {
- std::string name{name_view};
- std::shared_ptr<spdlog::logger> spdlogger = spdlog::get(name);
- if (spdlogger) {
- if (remove_if_present) {
- spdlog::drop(name);
- } else {
- return spdlogger;
- }
+std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(const std::shared_ptr<internal::LoggerNamespace> &root_namespace, const std::string_view name_view,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ const std::string name{name_view};
+ if (auto spdlogger = spdlog::get(name)) {
+ return spdlogger;
}
+ return create_logger(root_namespace, name, formatter);
+}
+
+void LoggerConfiguration::setupSpdLogger(const std::shared_ptr<spdlog::logger>& spd_logger,
+ const std::shared_ptr<internal::LoggerNamespace>& root_namespace,
+ const std::string& name,
+ const std::shared_ptr<spdlog::formatter>& formatter) {
+ if (!spd_logger)
+ return;
std::shared_ptr<internal::LoggerNamespace> current_namespace = root_namespace;
std::vector<std::shared_ptr<spdlog::sinks::sink>> sinks = root_namespace->sinks;
std::vector<std::shared_ptr<spdlog::sinks::sink>> inherited_sinks;
spdlog::level::level_enum level = root_namespace->level;
std::string current_namespace_str;
- std::string sink_namespace_str = "root";
- std::string level_namespace_str = "root";
for (auto const & name_segment : utils::string::split(name, "::")) {
current_namespace_str += name_segment;
auto child_pair = current_namespace->children.find(name_segment);
if (child_pair == current_namespace->children.end()) {
break;
}
- std::copy(current_namespace->exported_sinks.begin(), current_namespace->exported_sinks.end(), std::back_inserter(inherited_sinks));
+ ranges::copy(current_namespace->exported_sinks, std::back_inserter(inherited_sinks));
+
current_namespace = child_pair->second;
if (!current_namespace->sinks.empty()) {
sinks = current_namespace->sinks;
- sink_namespace_str = current_namespace_str;
}
if (current_namespace->has_level) {
level = current_namespace->level;
- level_namespace_str = current_namespace_str;
}
current_namespace_str += "::";
}
- if (logger != nullptr) {
- logger->log_debug("{} logger got sinks from namespace {} and level {} from namespace {}", name, sink_namespace_str, spdlog::level::to_string_view(level), level_namespace_str);
- }
- std::copy(inherited_sinks.begin(), inherited_sinks.end(), std::back_inserter(sinks));
- spdlogger = std::make_shared<spdlog::logger>(name, begin(sinks), end(sinks));
- spdlogger->set_level(level);
- spdlogger->set_formatter(formatter->clone());
- spdlogger->flush_on(std::max(spdlog::level::info, current_namespace->level));
+ ranges::copy(inherited_sinks, std::back_inserter(sinks));
+ spd_logger->sinks() = sinks;
+ spd_logger->set_level(level);
+ spd_logger->set_formatter(formatter->clone());
+ spd_logger->flush_on(std::max(spdlog::level::info, current_namespace->level));
Review Comment:
Im starting to think we should just simply delete this test...
We shouldnt test private implementation details in the first space...
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org