You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/03/18 10:45:41 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
martinzink opened a new pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600387294
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -197,10 +200,117 @@ uint64_t OsUtils::getMemoryUsage() {
throw std::runtime_error("Could not get memory info for current process");
return pmc.WorkingSetSize;
#endif
+ throw std::runtime_error("getCurrentProcessPhysicalMemoryUsage() is not implemented for this platform");
+}
+
+uint64_t OsUtils::getSystemPhysicalMemoryUsage() {
+#ifdef __linux__
+ const std::string available_memory_prefix = "MemAvailable:";
+ const std::string total_memory_prefix = "MemTotal:";
+ std::ifstream meminfo_file("/proc/meminfo");
+ std::string line;
- throw std::runtime_error("getMemoryUsage() is not implemented for this platform");
+ minifi::utils::optional<uint64_t> total_memory_kByte;
+ minifi::utils::optional<uint64_t> available_memory_kByte;
+ while ((!total_memory_kByte.has_value() || !available_memory_kByte.has_value()) && std::getline(meminfo_file, line)) {
+ if (line.rfind(total_memory_prefix, 0) == 0) {
+ std::istringstream total_memory_line(line.substr(total_memory_prefix.length()));
+ total_memory_kByte.emplace(0);
+ total_memory_line >> total_memory_kByte.value();
+ } else if (line.rfind(available_memory_prefix, 0) == 0) {
+ std::istringstream available_memory_line(line.substr(available_memory_prefix.length()));
+ available_memory_kByte.emplace(0);
+ available_memory_line >> available_memory_kByte.value();
+ }
+ }
+ if (total_memory_kByte.has_value() && available_memory_kByte.has_value())
+ return (total_memory_kByte.value() - available_memory_kByte.value()) * 1024;
+
+ throw std::runtime_error("Could not get memory info");
+#endif
+
+#ifdef __APPLE__
+ vm_size_t page_size;
+ mach_port_t mach_port = mach_host_self();
+ vm_statistics64_data_t vm_stats;
+ mach_msg_type_number_t count = sizeof(vm_stats) / sizeof(natural_t);
+ if (KERN_SUCCESS == host_page_size(mach_port, &page_size) &&
+ KERN_SUCCESS == host_statistics64(mach_port, HOST_VM_INFO,
+ (host_info64_t)&vm_stats, &count)) {
+ uint64_t physical_memory_used = ((int64_t)vm_stats.active_count +
+ (int64_t)vm_stats.wire_count) * (int64_t)page_size;
+ return physical_memory_used;
+ }
+ throw std::runtime_error("Could not get memory info");
Review comment:
should we throw from these functions? as I understand we are calling these while serializing the heartbeat, in which case an unhandled exception results in no heartbeat (which might not be what we want)
should we put a `static_assert(false, "Unsupported platform")` in the "else"-branch, we would get compile-time notification if we compile for unsupported platform and whenever we still could get runtime invalid results (like here) we should log the error and not send that (nonexistent) information in the heartbeat (or send some sensible "error-value", -1 or something)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600354264
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -380,9 +411,59 @@ class Int64Value : public Value {
return false;
}
+ virtual bool getValue(double& /*ref*/) {
+ return false;
+ }
+
int64_t value;
};
+class DoubleValue : public Value {
+ public:
+ explicit DoubleValue(double value)
+ : Value(std::to_string(value)),
+ value(value) {
+ setTypeId<double>();
+ }
+ explicit DoubleValue(const std::string &strvalue)
+ : Value(strvalue) {
+ utils::internal::ValueParser(strvalue).parse(value).parseEnd();
+ setTypeId<double>();
+ }
+
+ double getValue() {
+ return value;
+ }
+
+ protected:
+ virtual bool getValue(int&) {
+ return false;
+ }
+
+ virtual bool getValue(uint32_t&) {
+ return false;
+ }
+
+ virtual bool getValue(int64_t&) {
+ return false;
Review comment:
why disable all castings? others like `IntValue` check the range (quite badly) and cast if possible, should we provide a helper like this, and thread all these `Value::getValue(*)` methods body through it?
```
template<T, U>
bool utils::cast_if_in_range(T in, U& out) {
U result = static_cast<U>(in);
if (static_cast<T>(result) != in) {
return false;
}
out = result;
return true;
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603114722
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -580,6 +573,10 @@ struct SerializedResponseNode {
SerializedResponseNode(const SerializedResponseNode &other) = default;
SerializedResponseNode &operator=(const SerializedResponseNode &other) = default;
+
+ bool empty() {
Review comment:
changed it in [7b80c81](7b80c812be57906faff88b091302fa36227f940f)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600410229
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,186 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
Review comment:
as the functionality here is inherently platform specific we could create separate `SystemCPUUsageTracker{Linux,Win,Apple}.cpp` files and include them here (they would need to be placed in a `platform` directory as the `cpp` files are collected by a glob)
alternatively we could also make the distinction at the cmake level
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r596767716
##########
File path: libminifi/include/utils/OsUtils.h
##########
@@ -29,8 +29,17 @@ namespace OsUtils {
/// Resolves a user ID to a username
extern std::string userIdToUsername(const std::string &uid);
-/// Returns memory usage in bytes, including shared memory
-uint64_t getMemoryUsage();
+/// Returns physical memory usage by the current process in bytes
+uint64_t getCurrentProcessPhysicalMemoryUsage();
Review comment:
I also looked into getting information about virtual memory but its meaning is wildly inconsistent between platforms and usually doesn't really hold useful information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601397912
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -197,10 +200,117 @@ uint64_t OsUtils::getMemoryUsage() {
throw std::runtime_error("Could not get memory info for current process");
return pmc.WorkingSetSize;
#endif
+ throw std::runtime_error("getCurrentProcessPhysicalMemoryUsage() is not implemented for this platform");
+}
+
+uint64_t OsUtils::getSystemPhysicalMemoryUsage() {
+#ifdef __linux__
+ const std::string available_memory_prefix = "MemAvailable:";
+ const std::string total_memory_prefix = "MemTotal:";
+ std::ifstream meminfo_file("/proc/meminfo");
+ std::string line;
- throw std::runtime_error("getMemoryUsage() is not implemented for this platform");
+ minifi::utils::optional<uint64_t> total_memory_kByte;
+ minifi::utils::optional<uint64_t> available_memory_kByte;
+ while ((!total_memory_kByte.has_value() || !available_memory_kByte.has_value()) && std::getline(meminfo_file, line)) {
+ if (line.rfind(total_memory_prefix, 0) == 0) {
+ std::istringstream total_memory_line(line.substr(total_memory_prefix.length()));
+ total_memory_kByte.emplace(0);
+ total_memory_line >> total_memory_kByte.value();
+ } else if (line.rfind(available_memory_prefix, 0) == 0) {
+ std::istringstream available_memory_line(line.substr(available_memory_prefix.length()));
+ available_memory_kByte.emplace(0);
+ available_memory_line >> available_memory_kByte.value();
+ }
+ }
+ if (total_memory_kByte.has_value() && available_memory_kByte.has_value())
+ return (total_memory_kByte.value() - available_memory_kByte.value()) * 1024;
+
+ throw std::runtime_error("Could not get memory info");
+#endif
+
+#ifdef __APPLE__
+ vm_size_t page_size;
+ mach_port_t mach_port = mach_host_self();
+ vm_statistics64_data_t vm_stats;
+ mach_msg_type_number_t count = sizeof(vm_stats) / sizeof(natural_t);
+ if (KERN_SUCCESS == host_page_size(mach_port, &page_size) &&
+ KERN_SUCCESS == host_statistics64(mach_port, HOST_VM_INFO,
+ (host_info64_t)&vm_stats, &count)) {
+ uint64_t physical_memory_used = ((int64_t)vm_stats.active_count +
+ (int64_t)vm_stats.wire_count) * (int64_t)page_size;
+ return physical_memory_used;
+ }
+ throw std::runtime_error("Could not get memory info");
Review comment:
yeah, I agree throwing seems excessive here.
I changed these functions to return int64 instead of uint64 and changed the throws to return -1 instead if something goes wrong. [d28d399](https://github.com/martinzink/nifi-minifi-cpp/commit/d28d3994fe633513158fb739edc49f69023b8a39)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600354264
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -380,9 +411,59 @@ class Int64Value : public Value {
return false;
}
+ virtual bool getValue(double& /*ref*/) {
+ return false;
+ }
+
int64_t value;
};
+class DoubleValue : public Value {
+ public:
+ explicit DoubleValue(double value)
+ : Value(std::to_string(value)),
+ value(value) {
+ setTypeId<double>();
+ }
+ explicit DoubleValue(const std::string &strvalue)
+ : Value(strvalue) {
+ utils::internal::ValueParser(strvalue).parse(value).parseEnd();
+ setTypeId<double>();
+ }
+
+ double getValue() {
+ return value;
+ }
+
+ protected:
+ virtual bool getValue(int&) {
+ return false;
+ }
+
+ virtual bool getValue(uint32_t&) {
+ return false;
+ }
+
+ virtual bool getValue(int64_t&) {
+ return false;
Review comment:
why disable all casts? others like `IntValue` check the range (quite badly) and cast if possible, should we provide a helper like this, and thread all these `Value::getValue(*)` methods body through it?
```
template<T, U>
bool utils::cast_if_in_range(T in, U& out) {
U result = static_cast<U>(in);
if (static_cast<T>(result) != in) {
return false;
}
out = result;
return true;
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600363533
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,121 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ if (!repositories_.empty()) {
+ serialized.push_back(serializeRepositories());
Review comment:
after the refactor we should only depend on the return value of `serializeRepositories()` to determine if we should append it to the `serialized` vector, I propose either
1. an `SerializedResponseNode::empty` member function
2. or `serializeRepositories()` returning an `optional<...>`
my preference lies with 1) az I wouldn't consider serializing an empty list of repositories "invalid" per-se
(this applies to `serializeComponents` as well)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601398341
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,113 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
+
+ int positive_int_value = 6;
+ value_node = positive_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(canConvertToType<uint32_t>(value_node));
+ REQUIRE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE(!canConvertToType<bool>(value_node));
+ REQUIRE(!canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "6");
+
+ int negative_int_value = -7;
+ value_node = negative_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(!canConvertToType<uint32_t>(value_node));
Review comment:
good idea, changed it in [d28d399](https://github.com/martinzink/nifi-minifi-cpp/commit/d28d3994fe633513158fb739edc49f69023b8a39)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600418932
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,113 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
+
+ int positive_int_value = 6;
+ value_node = positive_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(canConvertToType<uint32_t>(value_node));
+ REQUIRE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE(!canConvertToType<bool>(value_node));
+ REQUIRE(!canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "6");
+
+ int negative_int_value = -7;
+ value_node = negative_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(!canConvertToType<uint32_t>(value_node));
Review comment:
`REQUIRE_FALSE` might be used here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
~`static_cast` is preferred (or better yet, `gsl::narrow` if need be)~
there is no need for the cast here, if we really want to ensure that it is being called with `uint64_t` we could go for
```suggestion
used_physical_memory.value = uint64_t{utils::OsUtils::getSystemPhysicalMemoryUsage()};
```
which will compile time prevent narrowing conversion from whatever the functions return type is to `uint64_t`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r596762674
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -380,9 +411,59 @@ class Int64Value : public Value {
return false;
}
+ virtual bool getValue(double& /*ref*/) {
+ return false;
+ }
+
int64_t value;
};
+class DoubleValue : public Value {
Review comment:
This class is required to be able to parse and deparse doubles in the heartbeat
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r596759457
##########
File path: libminifi/CMakeLists.txt
##########
@@ -125,6 +125,8 @@ endif()
list(APPEND LIBMINIFI_LIBRARIES yaml-cpp ZLIB::ZLIB concurrentqueue RapidJSON spdlog cron Threads::Threads gsl-lite optional-lite libsodium)
if(NOT WIN32)
list(APPEND LIBMINIFI_LIBRARIES OSSP::libuuid++)
+else()
+ list(APPEND LIBMINIFI_LIBRARIES pdh)
Review comment:
pdh (Performance Data Helper) is required to collect CPU usage information on windows
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601394393
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,121 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ if (!repositories_.empty()) {
+ serialized.push_back(serializeRepositories());
Review comment:
good idea, went with your first suggestion in [d28d399](https://github.com/martinzink/nifi-minifi-cpp/commit/d28d3994fe633513158fb739edc49f69023b8a39)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r620090718
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
Review comment:
fixed it in [007a486](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/007a486268761ce8fc086dbcc5148ff1e03b0689)
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
Review comment:
fixed it in [007a486](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/007a486268761ce8fc086dbcc5148ff1e03b0689)
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTrackerBase() = default;
+ virtual ~ProcessCPUUsageTrackerBase() = default;
+ virtual double getCPUUsageAndRestartCollection() = 0;
+};
+
+#if defined(__linux__) || defined(__APPLE__)
+class ProcessCPUUsageTracker : ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTracker();
+ ~ProcessCPUUsageTracker() = default;
+ double getCPUUsageAndRestartCollection() override;
+
+ protected:
+ void queryCPUTimes();
+ bool isCurrentQueryOlderThanPrevious();
+ bool isCurrentQuerySameAsPrevious();
Review comment:
fixed it in [007a486](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/007a486268761ce8fc086dbcc5148ff1e03b0689)
##########
File path: libminifi/include/utils/ValueParser.h
##########
@@ -119,6 +119,17 @@ class ValueParser {
return *this;
}
+ ValueParser& parse(double& out) {
+ double result; // NOLINT
Review comment:
sure, fixed it in [007a486](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/007a486268761ce8fc086dbcc5148ff1e03b0689)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r621970958
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_user_low_ = total_user_low_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILE* file = fopen("/proc/stat", "r");
+ if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_, &total_sys_, &total_idle_) != 4) {
+ total_user_ = previous_total_user_;
+ total_user_low_ = previous_total_user_low_;
+ total_idle_ = previous_total_idle_;
+ total_sys_ = previous_total_sys_;
+ }
+ fclose(file);
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_user_low_ < previous_total_user_low_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_user_low_ == previous_total_user_low_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_user_low_diff = total_user_low_ - previous_total_user_low_;
+ uint64_t total_system_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_user_low_diff + total_system_diff;
+ double percent = static_cast<double>(total_diff)/static_cast<double>(total_diff+total_idle_diff);
+
+ return percent;
+}
+#endif // linux
+
+#ifdef WIN32
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_idle_(0), total_sys_(0), total_user_(0),
+ previous_total_idle_(0), previous_total_sys_(0), previous_total_user_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILETIME fidle, fsys, fuser;
+ GetSystemTimes(&fidle, &fsys, &fuser);
+ total_user_ = ULARGE_INTEGER{ fuser.dwLowDateTime, fuser.dwHighDateTime }.QuadPart;
+ total_sys_ = ULARGE_INTEGER{ fsys.dwLowDateTime, fsys.dwHighDateTime }.QuadPart;
+ total_idle_ = ULARGE_INTEGER{ fidle.dwLowDateTime, fidle.dwHighDateTime }.QuadPart;
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_sys_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_sys_diff;
+ double percent = static_cast<double>(total_diff - total_idle_diff) / static_cast<double>(total_diff);
+
+ return percent;
+}
+#endif // windows
+
+#ifdef __APPLE__
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_ticks_(0), previous_total_ticks_(0),
+ idle_ticks_(0), previous_idle_ticks_(0) {
+ queryCPUTicks();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTicks();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTicks() {
+ host_cpu_load_info_data_t cpuinfo;
+ mach_msg_type_number_t count = HOST_CPU_LOAD_INFO_COUNT;
+ auto query_result = host_statistics(mach_host_self(), HOST_CPU_LOAD_INFO, (host_info_t)&cpuinfo, &count);
+ if (query_result == KERN_SUCCESS) {
+ previous_total_ticks_ = total_ticks_;
+ previous_idle_ticks_ = idle_ticks_;
+ total_ticks_ = 0;
+ for (int i = 0; i < CPU_STATE_MAX; i++) {
+ total_ticks_ += cpuinfo.cpu_ticks[i];
+ }
+ idle_ticks_ = cpuinfo.cpu_ticks[CPU_STATE_IDLE];
+ }
Review comment:
Yeah, apples cpp documentation is not the best. The basic idea is the same as in the other platforms, I've got the idea to use host_statistics from forums, but managed to verify it: Apple uses this in very similar fashion:
https://github.com/apple/foundationdb/blob/6571e9e78cd0f22af5e283f0d5b7c3dde661afc4/flow/Platform.actor.cpp#L1177
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r609731373
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQuerySameAsPrevious()) {
Review comment:
Oops, you are right. Fixed it in [d2832cf](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/d2832cfcb3afa837e30e7bc95f496f0754d7603f)
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
- SerializedResponseNode componentNode(false);
- componentNode.name = component->getComponentName();
+ SerializedResponseNode component_node(false);
+ component_node.name = component->getComponentName();
- SerializedResponseNode uuidNode;
- uuidNode.name = "uuid";
- uuidNode.value = std::string{component->getComponentUUID().to_string()};
+ SerializedResponseNode uuid_node;
+ uuid_node.name = "uuid";
+ uuid_node.value = std::string{component->getComponentUUID().to_string()};
- SerializedResponseNode componentStatusNode;
- componentStatusNode.name = "running";
- componentStatusNode.value = component->isRunning();
+ SerializedResponseNode component_status_node;
+ component_status_node.name = "running";
+ component_status_node.value = component->isRunning();
- componentNode.children.push_back(componentStatusNode);
- componentNode.children.push_back(uuidNode);
- componentsNode.children.push_back(componentNode);
+ component_node.children.push_back(component_status_node);
+ component_node.children.push_back(uuid_node);
+ components_node.children.push_back(component_node);
}
- serialized.push_back(componentsNode);
}
+ return components_node;
+ }
+
+ SerializedResponseNode serializeAgentMemoryUsage() {
Review comment:
done [d2832cf](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/d2832cfcb3afa837e30e7bc95f496f0754d7603f)
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
Review comment:
done [d2832cf](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/d2832cfcb3afa837e30e7bc95f496f0754d7603f)
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
Review comment:
done [d2832cf](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/d2832cfcb3afa837e30e7bc95f496f0754d7603f)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603214289
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -228,36 +225,36 @@ class IntValue : public Value {
}
protected:
- virtual bool getValue(int &ref) {
+ bool getValue(int &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint32_t &ref) {
+ bool getValue(uint32_t &ref) override {
if (value < 0)
return false;
ref = value;
return true;
}
- virtual bool getValue(int64_t &ref) {
+ bool getValue(int64_t &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint64_t &ref) {
+ bool getValue(uint64_t &ref) override {
if (value < 0)
Review comment:
refactored it in [4b341dc](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/4b341dccf0c75efb76968acb467e6af5996f51d6)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601394864
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,121 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ if (!repositories_.empty()) {
+ serialized.push_back(serializeRepositories());
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ serialized.push_back(serializeComponents());
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ repositories.name = "repositories";
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
+
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
- SerializedResponseNode componentNode(false);
- componentNode.name = component->getComponentName();
+ SerializedResponseNode component_node(false);
+ component_node.name = component->getComponentName();
SerializedResponseNode uuidNode;
Review comment:
fixed in [d28d399](https://github.com/martinzink/nifi-minifi-cpp/commit/d28d3994fe633513158fb739edc49f69023b8a39)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
~`static_cast` is preferred (or better yet, `gsl::narrow` if need be)~
there is no need for the cast here, if we really want to ensure that it is being called by `uint64_t` we could go for
```suggestion
used_physical_memory.value = uint64_t{utils::OsUtils::getSystemPhysicalMemoryUsage()};
```
which will compile time prevent narrowing conversion from whatever the functions return type is to `uint64_t`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r619122307
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTrackerBase() = default;
+ virtual ~ProcessCPUUsageTrackerBase() = default;
+ virtual double getCPUUsageAndRestartCollection() = 0;
+};
+
+#if defined(__linux__) || defined(__APPLE__)
Review comment:
I see that you've already discussed this, it's okay to have it in a followup.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r621970763
##########
File path: libminifi/test/unit/MemoryUsageTest.cpp
##########
@@ -21,8 +21,29 @@
#include "../TestBase.h"
TEST_CASE("Test memory usage", "[testmemoryusage]") {
+ constexpr bool cout_enabled = true;
std::vector<char> v(30000000);
- const auto memoryUsage = utils::OsUtils::getMemoryUsage();
- REQUIRE(memoryUsage > v.size());
- REQUIRE(memoryUsage < 2 * v.size());
+ const auto RAMUsagebyProcess = utils::OsUtils::getCurrentProcessPhysicalMemoryUsage();
+ const auto RAMUsagebySystem = utils::OsUtils::getSystemPhysicalMemoryUsage();
+ const auto RAMTotal = utils::OsUtils::getSystemTotalPhysicalMemory();
+
+ if (cout_enabled) {
+ std::cout << "Physical Memory used by this process: " << RAMUsagebyProcess << " bytes" << std::endl;
+ std::cout << "Physical Memory used by the system: " << RAMUsagebySystem << " bytes" << std::endl;
+ std::cout << "Total Physical Memory in the system: " << RAMTotal << " bytes" << std::endl;
+ }
+ REQUIRE(RAMUsagebyProcess >= v.size());
+ REQUIRE(v.size()*2 >= RAMUsagebyProcess);
+ REQUIRE(RAMUsagebySystem >= RAMUsagebyProcess);
+ REQUIRE(RAMTotal >= RAMUsagebySystem);
+}
+
+#ifndef WIN32
+size_t GetTotalMemoryLegacy() {
+ return (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
Review comment:
Yeah, I just copy pasted the pre-PR method to verify the new one gives the same result.
Strange that the linter havent caught it before, and it didnt catch it now either.
Nonetheless changed it to use static_cast in https://github.com/apache/nifi-minifi-cpp/pull/1065
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r596759457
##########
File path: libminifi/CMakeLists.txt
##########
@@ -125,6 +125,8 @@ endif()
list(APPEND LIBMINIFI_LIBRARIES yaml-cpp ZLIB::ZLIB concurrentqueue RapidJSON spdlog cron Threads::Threads gsl-lite optional-lite libsodium)
if(NOT WIN32)
list(APPEND LIBMINIFI_LIBRARIES OSSP::libuuid++)
+else()
+ list(APPEND LIBMINIFI_LIBRARIES pdh)
Review comment:
pdh (Performance Data Helper) is required to collect CPU usage information on windows
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r620100424
##########
File path: libminifi/include/utils/ValueCaster.h
##########
@@ -0,0 +1,48 @@
+/**
+ *
+ * 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/licenseas/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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+template<class T, class U>
+bool cast_if_in_range(T in, U& out) {
Review comment:
yeah, now I am conflicted too. I was under the impression that freefunctions should be snake_case (while member functions should be lowerCamelCase) but there is no documentation about this besides the mentioning the google style guide (which uses UpperCamelCase and sometimes snake_case)
I don't have a strong preference and I am comfortable with either snake_case or lowerCamelCase here. What is your preference?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r620103666
##########
File path: libminifi/include/utils/ValueCaster.h
##########
@@ -0,0 +1,48 @@
+/**
+ *
+ * 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/licenseas/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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+template<class T, class U>
+bool cast_if_in_range(T in, U& out) {
Review comment:
I don't have any strong preference here either, so you can keep it as it is.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r621969862
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_user_low_ = total_user_low_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILE* file = fopen("/proc/stat", "r");
+ if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_, &total_sys_, &total_idle_) != 4) {
+ total_user_ = previous_total_user_;
+ total_user_low_ = previous_total_user_low_;
+ total_idle_ = previous_total_idle_;
+ total_sys_ = previous_total_sys_;
+ }
+ fclose(file);
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_user_low_ < previous_total_user_low_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_user_low_ == previous_total_user_low_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_user_low_diff = total_user_low_ - previous_total_user_low_;
+ uint64_t total_system_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_user_low_diff + total_system_diff;
+ double percent = static_cast<double>(total_diff)/static_cast<double>(total_diff+total_idle_diff);
Review comment:
That wont happened because this function (protected) is only called after the various checks (isCurrentQueryOlderThanPrevious and isCurrentQuerySameAsPrevious) which make sure that there are valid differences between the last and current query.
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
Review comment:
Yeah, I see your point, changed it in https://github.com/apache/nifi-minifi-cpp/pull/1065
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,187 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
+
+ int positive_int_value = 6;
+ value_node = positive_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(canConvertToType<uint32_t>(value_node));
+ REQUIRE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE_FALSE(canConvertToType<bool>(value_node));
+ REQUIRE(canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "6");
+
+ int negative_int_value = -7;
+ value_node = negative_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE_FALSE(canConvertToType<uint32_t>(value_node));
+ REQUIRE_FALSE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE_FALSE(canConvertToType<bool>(value_node));
+ REQUIRE(canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "-7");
+
+ uint32_t max_uint32_value = (uint64_t{1} << 32) - 1;
Review comment:
changed it in https://github.com/apache/nifi-minifi-cpp/pull/1065
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,187 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
Review comment:
changed it in https://github.com/apache/nifi-minifi-cpp/pull/1065
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r596771906
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -490,6 +477,8 @@ class DeviceInfoNode : public DeviceInformation {
std::string hostname_;
std::string ip_;
std::string device_id_;
+ static utils::SystemCPUUsageTracker cpu_load_tracker_;
+ static std::mutex cpu_load_tracker_mutex_;
Review comment:
this is static because CPU Usage tracking needs some persistent data, because it compares CPU ticks between heartbeats, but the response nodes are instantiated and destroyed with each heartbeat.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603124037
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -228,36 +225,36 @@ class IntValue : public Value {
}
protected:
- virtual bool getValue(int &ref) {
+ bool getValue(int &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint32_t &ref) {
+ bool getValue(uint32_t &ref) override {
if (value < 0)
return false;
ref = value;
return true;
}
- virtual bool getValue(int64_t &ref) {
+ bool getValue(int64_t &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint64_t &ref) {
+ bool getValue(uint64_t &ref) override {
if (value < 0)
Review comment:
We could, I left these signed into longer unsigned as is because they could be confidently casted with one if.
Of course making everything the same could make this more readable. Should I change it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600369706
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,121 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ if (!repositories_.empty()) {
+ serialized.push_back(serializeRepositories());
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ serialized.push_back(serializeComponents());
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ repositories.name = "repositories";
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
+
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
- SerializedResponseNode componentNode(false);
- componentNode.name = component->getComponentName();
+ SerializedResponseNode component_node(false);
+ component_node.name = component->getComponentName();
SerializedResponseNode uuidNode;
Review comment:
poor `uuidNode` got left behind with its old name 😏
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r609510112
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
- SerializedResponseNode componentNode(false);
- componentNode.name = component->getComponentName();
+ SerializedResponseNode component_node(false);
+ component_node.name = component->getComponentName();
- SerializedResponseNode uuidNode;
- uuidNode.name = "uuid";
- uuidNode.value = std::string{component->getComponentUUID().to_string()};
+ SerializedResponseNode uuid_node;
+ uuid_node.name = "uuid";
+ uuid_node.value = std::string{component->getComponentUUID().to_string()};
- SerializedResponseNode componentStatusNode;
- componentStatusNode.name = "running";
- componentStatusNode.value = component->isRunning();
+ SerializedResponseNode component_status_node;
+ component_status_node.name = "running";
+ component_status_node.value = component->isRunning();
- componentNode.children.push_back(componentStatusNode);
- componentNode.children.push_back(uuidNode);
- componentsNode.children.push_back(componentNode);
+ component_node.children.push_back(component_status_node);
+ component_node.children.push_back(uuid_node);
+ components_node.children.push_back(component_node);
}
- serialized.push_back(componentsNode);
}
+ return components_node;
+ }
+
+ SerializedResponseNode serializeAgentMemoryUsage() {
Review comment:
This func could be const
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
+ SerializedResponseNode repoNode;
+ repoNode.collapsible = false;
+ repoNode.name = repo.first;
- SerializedResponseNode isRunning;
- isRunning.name = "running";
- isRunning.value = repo.second->isRunning();
+ SerializedResponseNode queuesize;
+ queuesize.name = "size";
+ queuesize.value = repo.second->getRepoSize();
- SerializedResponseNode isFull;
- isFull.name = "full";
- isFull.value = repo.second->isFull();
+ SerializedResponseNode isRunning;
+ isRunning.name = "running";
+ isRunning.value = repo.second->isRunning();
- repoNode.children.push_back(queuesize);
- repoNode.children.push_back(isRunning);
- repoNode.children.push_back(isFull);
- repositories.children.push_back(repoNode);
- }
- serialized.push_back(repositories);
+ SerializedResponseNode isFull;
+ isFull.name = "full";
+ isFull.value = repo.second->isFull();
+
+ repoNode.children.push_back(queuesize);
+ repoNode.children.push_back(isRunning);
+ repoNode.children.push_back(isFull);
+ repositories.children.push_back(repoNode);
}
+ return repositories;
+ }
- serialized.push_back(uptime);
+ SerializedResponseNode serializeUptime() {
+ SerializedResponseNode uptime;
+ uptime.name = "uptime";
if (nullptr != monitor_) {
+ uptime.value = monitor_->getUptime();
+ } else {
+ uptime.value = "0";
+ }
+
+ return uptime;
+ }
+
+ SerializedResponseNode serializeComponents() {
+ SerializedResponseNode components_node(false);
+ components_node.name = "components";
+ if (monitor_ != nullptr) {
auto components = monitor_->getAllComponents();
- SerializedResponseNode componentsNode(false);
- componentsNode.name = "components";
for (auto component : components) {
Review comment:
I know it's old code, but as we touch it - copy seems to be needless here.
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQuerySameAsPrevious()) {
Review comment:
Do I miss something, or is it A || A?
I think in one of the conditions you wanted if check if query is **older** than previous
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -425,77 +427,123 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
+ }
+ serialized.push_back(serializeUptime());
- SerializedResponseNode uptime;
-
- uptime.name = "uptime";
- if (nullptr != monitor_) {
- uptime.value = monitor_->getUptime();
- } else {
- uptime.value = "0";
+ auto serializedComponents = serializeComponents();
+ if (!serializedComponents.empty()) {
+ serialized.push_back(serializedComponents);
}
- if (!repositories_.empty()) {
- SerializedResponseNode repositories;
+ serialized.push_back(serializeResourceConsumption());
- repositories.name = "repositories";
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeRepositories() {
+ SerializedResponseNode repositories;
- for (auto &repo : repositories_) {
- SerializedResponseNode repoNode;
- repoNode.collapsible = false;
- repoNode.name = repo.first;
+ repositories.name = "repositories";
- SerializedResponseNode queuesize;
- queuesize.name = "size";
- queuesize.value = repo.second->getRepoSize();
+ for (auto &repo : repositories_) {
Review comment:
Does it modify anything on the original container?
I don't see such line, so I would go with const & here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600511300
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
Review comment:
[utilization](https://dictionary.cambridge.org/dictionary/english/utilization):
> the amount of something available, produced, etc. compared with the total amount that exists or that could be produced
Either we should return a percentage value here or rename this to something like memoryUsage.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r621969441
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -164,43 +168,140 @@ std::string OsUtils::userIdToUsername(const std::string &uid) {
return name;
}
-uint64_t OsUtils::getMemoryUsage() {
-#ifdef __linux__
- static const std::string linePrefix = "VmRSS:";
- std::ifstream statusFile("/proc/self/status");
+int64_t OsUtils::getCurrentProcessPhysicalMemoryUsage() {
+#if defined(__linux__)
+ static const std::string resident_set_size_prefix = "VmRSS:";
+ std::ifstream status_file("/proc/self/status");
std::string line;
- while (std::getline(statusFile, line)) {
- // if line begins with "VmRSS:"
- if (line.rfind(linePrefix, 0) == 0) {
- std::istringstream valuableLine(line.substr(linePrefix.length()));
- uint64_t kByteValue;
- valuableLine >> kByteValue;
- return kByteValue * 1024;
+ while (std::getline(status_file, line)) {
+ if (line.rfind(resident_set_size_prefix, 0) == 0) {
+ std::istringstream resident_set_size_value(line.substr(resident_set_size_prefix.length()));
+ uint64_t memory_usage_in_kBytes;
+ resident_set_size_value >> memory_usage_in_kBytes;
+ return memory_usage_in_kBytes * 1024;
}
}
- throw std::runtime_error("Could not get memory info for current process");
-#endif
-
-#ifdef __APPLE__
+ return -1;
+#elif defined(__APPLE__)
task_basic_info tInfo;
mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&tInfo, &tInfoCount))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return tInfo.resident_size;
-#endif
-
-#ifdef _WIN32
+#elif defined(WIN32)
PROCESS_MEMORY_COUNTERS pmc;
if (!GetProcessMemoryInfo(GetCurrentProcess(), &pmc, sizeof(pmc)))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return pmc.WorkingSetSize;
+#else
+ static_assert(false, "Unsupported platform");
Review comment:
Thanks thats a valid point, changed it to only warn and give back -1 as result in https://github.com/apache/nifi-minifi-cpp/pull/1065
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -164,43 +168,140 @@ std::string OsUtils::userIdToUsername(const std::string &uid) {
return name;
}
-uint64_t OsUtils::getMemoryUsage() {
-#ifdef __linux__
- static const std::string linePrefix = "VmRSS:";
- std::ifstream statusFile("/proc/self/status");
+int64_t OsUtils::getCurrentProcessPhysicalMemoryUsage() {
+#if defined(__linux__)
+ static const std::string resident_set_size_prefix = "VmRSS:";
+ std::ifstream status_file("/proc/self/status");
std::string line;
- while (std::getline(statusFile, line)) {
- // if line begins with "VmRSS:"
- if (line.rfind(linePrefix, 0) == 0) {
- std::istringstream valuableLine(line.substr(linePrefix.length()));
- uint64_t kByteValue;
- valuableLine >> kByteValue;
- return kByteValue * 1024;
+ while (std::getline(status_file, line)) {
+ if (line.rfind(resident_set_size_prefix, 0) == 0) {
+ std::istringstream resident_set_size_value(line.substr(resident_set_size_prefix.length()));
+ uint64_t memory_usage_in_kBytes;
+ resident_set_size_value >> memory_usage_in_kBytes;
+ return memory_usage_in_kBytes * 1024;
}
}
- throw std::runtime_error("Could not get memory info for current process");
-#endif
-
-#ifdef __APPLE__
+ return -1;
+#elif defined(__APPLE__)
task_basic_info tInfo;
mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&tInfo, &tInfoCount))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return tInfo.resident_size;
-#endif
-
-#ifdef _WIN32
+#elif defined(WIN32)
PROCESS_MEMORY_COUNTERS pmc;
if (!GetProcessMemoryInfo(GetCurrentProcess(), &pmc, sizeof(pmc)))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return pmc.WorkingSetSize;
+#else
+ static_assert(false, "Unsupported platform");
#endif
+}
+
+int64_t OsUtils::getSystemPhysicalMemoryUsage() {
+#if defined(__linux__)
+ const std::string available_memory_prefix = "MemAvailable:";
+ const std::string total_memory_prefix = "MemTotal:";
+ std::ifstream meminfo_file("/proc/meminfo");
+ std::string line;
- throw std::runtime_error("getMemoryUsage() is not implemented for this platform");
+ minifi::utils::optional<uint64_t> total_memory_kByte;
+ minifi::utils::optional<uint64_t> available_memory_kByte;
+ while ((!total_memory_kByte.has_value() || !available_memory_kByte.has_value()) && std::getline(meminfo_file, line)) {
+ if (line.rfind(total_memory_prefix, 0) == 0) {
+ std::istringstream total_memory_line(line.substr(total_memory_prefix.length()));
+ total_memory_kByte.emplace(0);
+ total_memory_line >> total_memory_kByte.value();
+ } else if (line.rfind(available_memory_prefix, 0) == 0) {
+ std::istringstream available_memory_line(line.substr(available_memory_prefix.length()));
+ available_memory_kByte.emplace(0);
+ available_memory_line >> available_memory_kByte.value();
+ }
+ }
+ if (total_memory_kByte.has_value() && available_memory_kByte.has_value())
+ return (total_memory_kByte.value() - available_memory_kByte.value()) * 1024;
+
+ return -1;
+#elif defined(__APPLE__)
+ vm_size_t page_size;
+ mach_port_t mach_port = mach_host_self();
+ vm_statistics64_data_t vm_stats;
+ mach_msg_type_number_t count = sizeof(vm_stats) / sizeof(natural_t);
+ if (KERN_SUCCESS == host_page_size(mach_port, &page_size) &&
+ KERN_SUCCESS == host_statistics64(mach_port, HOST_VM_INFO,
+ (host_info64_t)&vm_stats, &count)) {
+ uint64_t physical_memory_used = ((int64_t)vm_stats.active_count +
+ (int64_t)vm_stats.wire_count) * (int64_t)page_size;
+ return physical_memory_used;
+ }
+ return -1;
+#elif defined(WIN32)
+ MEMORYSTATUSEX memory_info;
+ memory_info.dwLength = sizeof(MEMORYSTATUSEX);
+ GlobalMemoryStatusEx(&memory_info);
+ DWORDLONG physical_memory_used = memory_info.ullTotalPhys - memory_info.ullAvailPhys;
+ return physical_memory_used;
+#else
+ static_assert(false, "Unsupported platform");
+#endif
+}
+
+int64_t OsUtils::getSystemTotalPhysicalMemory() {
+#if defined(__linux__)
+ struct sysinfo memory_info;
+ sysinfo(&memory_info);
+ uint64_t total_physical_memory = memory_info.totalram;
+ total_physical_memory *= memory_info.mem_unit;
+ return total_physical_memory;
+#elif defined(__APPLE__)
+ int mib[2];
+ int64_t total_physical_memory = 0;
+ mib[0] = CTL_HW;
+ mib[1] = HW_MEMSIZE;
+ size_t length = sizeof(int64_t);
+ sysctl(mib, 2, &total_physical_memory, &length, NULL, 0);
+ return total_physical_memory;
+#elif defined(WIN32)
+ MEMORYSTATUSEX memory_info;
+ memory_info.dwLength = sizeof(MEMORYSTATUSEX);
+ GlobalMemoryStatusEx(&memory_info);
+ DWORDLONG total_physical_memory = memory_info.ullTotalPhys;
+ return total_physical_memory;
+#else
+ static_assert(false, "Unsupported platform");
+#endif
}
+std::string OsUtils::getMachineArchitecture() {
+#if defined(__linux__) || defined(__APPLE__)
Review comment:
changed it in https://github.com/apache/nifi-minifi-cpp/pull/1065
##########
File path: libminifi/src/utils/ProcessCPUUsageTracker.cpp
##########
@@ -0,0 +1,140 @@
+/**
+ * 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 "utils/ProcessCPUUsageTracker.h"
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <sys/times.h>
+#endif
Review comment:
changed it in https://github.com/apache/nifi-minifi-cpp/pull/1065
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603295791
##########
File path: extensions/http-curl/tests/C2VerifyResourceConsumptionInHeartbeat.cpp
##########
@@ -0,0 +1,133 @@
+/**
Review comment:
we will have to decide what to do with `ProcessMetrics.h` which creates a `metrics.RuntimeMetrics.ProcessMetrics` node, populating it with some questionable data (like involuntary context switch count)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
`static_cast` is preferred
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r620348236
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -164,43 +168,140 @@ std::string OsUtils::userIdToUsername(const std::string &uid) {
return name;
}
-uint64_t OsUtils::getMemoryUsage() {
-#ifdef __linux__
- static const std::string linePrefix = "VmRSS:";
- std::ifstream statusFile("/proc/self/status");
+int64_t OsUtils::getCurrentProcessPhysicalMemoryUsage() {
+#if defined(__linux__)
+ static const std::string resident_set_size_prefix = "VmRSS:";
+ std::ifstream status_file("/proc/self/status");
std::string line;
- while (std::getline(statusFile, line)) {
- // if line begins with "VmRSS:"
- if (line.rfind(linePrefix, 0) == 0) {
- std::istringstream valuableLine(line.substr(linePrefix.length()));
- uint64_t kByteValue;
- valuableLine >> kByteValue;
- return kByteValue * 1024;
+ while (std::getline(status_file, line)) {
+ if (line.rfind(resident_set_size_prefix, 0) == 0) {
+ std::istringstream resident_set_size_value(line.substr(resident_set_size_prefix.length()));
+ uint64_t memory_usage_in_kBytes;
+ resident_set_size_value >> memory_usage_in_kBytes;
+ return memory_usage_in_kBytes * 1024;
}
}
- throw std::runtime_error("Could not get memory info for current process");
-#endif
-
-#ifdef __APPLE__
+ return -1;
+#elif defined(__APPLE__)
task_basic_info tInfo;
mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&tInfo, &tInfoCount))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return tInfo.resident_size;
-#endif
-
-#ifdef _WIN32
+#elif defined(WIN32)
PROCESS_MEMORY_COUNTERS pmc;
if (!GetProcessMemoryInfo(GetCurrentProcess(), &pmc, sizeof(pmc)))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return pmc.WorkingSetSize;
+#else
+ static_assert(false, "Unsupported platform");
Review comment:
I want to go back to using dummy values or non-fatal warnings on unsupported platforms, instead of intentionally making the compilation fail even if the agent would work otherwise.
Aside from having to deal with manual cmake calls, there's not much stopping users to compile minifi c++ on non-linux POSIX platforms today, even though it's not tested.
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_user_low_ = total_user_low_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILE* file = fopen("/proc/stat", "r");
+ if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_, &total_sys_, &total_idle_) != 4) {
+ total_user_ = previous_total_user_;
+ total_user_low_ = previous_total_user_low_;
+ total_idle_ = previous_total_idle_;
+ total_sys_ = previous_total_sys_;
+ }
+ fclose(file);
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_user_low_ < previous_total_user_low_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_user_low_ == previous_total_user_low_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_user_low_diff = total_user_low_ - previous_total_user_low_;
+ uint64_t total_system_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_user_low_diff + total_system_diff;
+ double percent = static_cast<double>(total_diff)/static_cast<double>(total_diff+total_idle_diff);
Review comment:
If `queryCPUTimes` fails, then `total_* == previous_total_*` => `total_diff == 0 && total_idle_diff == 0` => division by zero.
##########
File path: libminifi/test/unit/MemoryUsageTest.cpp
##########
@@ -21,8 +21,29 @@
#include "../TestBase.h"
TEST_CASE("Test memory usage", "[testmemoryusage]") {
+ constexpr bool cout_enabled = true;
std::vector<char> v(30000000);
- const auto memoryUsage = utils::OsUtils::getMemoryUsage();
- REQUIRE(memoryUsage > v.size());
- REQUIRE(memoryUsage < 2 * v.size());
+ const auto RAMUsagebyProcess = utils::OsUtils::getCurrentProcessPhysicalMemoryUsage();
+ const auto RAMUsagebySystem = utils::OsUtils::getSystemPhysicalMemoryUsage();
+ const auto RAMTotal = utils::OsUtils::getSystemTotalPhysicalMemory();
+
+ if (cout_enabled) {
+ std::cout << "Physical Memory used by this process: " << RAMUsagebyProcess << " bytes" << std::endl;
+ std::cout << "Physical Memory used by the system: " << RAMUsagebySystem << " bytes" << std::endl;
+ std::cout << "Total Physical Memory in the system: " << RAMTotal << " bytes" << std::endl;
+ }
+ REQUIRE(RAMUsagebyProcess >= v.size());
+ REQUIRE(v.size()*2 >= RAMUsagebyProcess);
+ REQUIRE(RAMUsagebySystem >= RAMUsagebyProcess);
+ REQUIRE(RAMTotal >= RAMUsagebySystem);
+}
+
+#ifndef WIN32
+size_t GetTotalMemoryLegacy() {
+ return (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
Review comment:
C-style cast, the linter should reject this. In any case, use `static_cast`. https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast
##########
File path: libminifi/src/utils/OsUtils.cpp
##########
@@ -164,43 +168,140 @@ std::string OsUtils::userIdToUsername(const std::string &uid) {
return name;
}
-uint64_t OsUtils::getMemoryUsage() {
-#ifdef __linux__
- static const std::string linePrefix = "VmRSS:";
- std::ifstream statusFile("/proc/self/status");
+int64_t OsUtils::getCurrentProcessPhysicalMemoryUsage() {
+#if defined(__linux__)
+ static const std::string resident_set_size_prefix = "VmRSS:";
+ std::ifstream status_file("/proc/self/status");
std::string line;
- while (std::getline(statusFile, line)) {
- // if line begins with "VmRSS:"
- if (line.rfind(linePrefix, 0) == 0) {
- std::istringstream valuableLine(line.substr(linePrefix.length()));
- uint64_t kByteValue;
- valuableLine >> kByteValue;
- return kByteValue * 1024;
+ while (std::getline(status_file, line)) {
+ if (line.rfind(resident_set_size_prefix, 0) == 0) {
+ std::istringstream resident_set_size_value(line.substr(resident_set_size_prefix.length()));
+ uint64_t memory_usage_in_kBytes;
+ resident_set_size_value >> memory_usage_in_kBytes;
+ return memory_usage_in_kBytes * 1024;
}
}
- throw std::runtime_error("Could not get memory info for current process");
-#endif
-
-#ifdef __APPLE__
+ return -1;
+#elif defined(__APPLE__)
task_basic_info tInfo;
mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, (task_info_t)&tInfo, &tInfoCount))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return tInfo.resident_size;
-#endif
-
-#ifdef _WIN32
+#elif defined(WIN32)
PROCESS_MEMORY_COUNTERS pmc;
if (!GetProcessMemoryInfo(GetCurrentProcess(), &pmc, sizeof(pmc)))
- throw std::runtime_error("Could not get memory info for current process");
+ return -1;
return pmc.WorkingSetSize;
+#else
+ static_assert(false, "Unsupported platform");
#endif
+}
+
+int64_t OsUtils::getSystemPhysicalMemoryUsage() {
+#if defined(__linux__)
+ const std::string available_memory_prefix = "MemAvailable:";
+ const std::string total_memory_prefix = "MemTotal:";
+ std::ifstream meminfo_file("/proc/meminfo");
+ std::string line;
- throw std::runtime_error("getMemoryUsage() is not implemented for this platform");
+ minifi::utils::optional<uint64_t> total_memory_kByte;
+ minifi::utils::optional<uint64_t> available_memory_kByte;
+ while ((!total_memory_kByte.has_value() || !available_memory_kByte.has_value()) && std::getline(meminfo_file, line)) {
+ if (line.rfind(total_memory_prefix, 0) == 0) {
+ std::istringstream total_memory_line(line.substr(total_memory_prefix.length()));
+ total_memory_kByte.emplace(0);
+ total_memory_line >> total_memory_kByte.value();
+ } else if (line.rfind(available_memory_prefix, 0) == 0) {
+ std::istringstream available_memory_line(line.substr(available_memory_prefix.length()));
+ available_memory_kByte.emplace(0);
+ available_memory_line >> available_memory_kByte.value();
+ }
+ }
+ if (total_memory_kByte.has_value() && available_memory_kByte.has_value())
+ return (total_memory_kByte.value() - available_memory_kByte.value()) * 1024;
+
+ return -1;
+#elif defined(__APPLE__)
+ vm_size_t page_size;
+ mach_port_t mach_port = mach_host_self();
+ vm_statistics64_data_t vm_stats;
+ mach_msg_type_number_t count = sizeof(vm_stats) / sizeof(natural_t);
+ if (KERN_SUCCESS == host_page_size(mach_port, &page_size) &&
+ KERN_SUCCESS == host_statistics64(mach_port, HOST_VM_INFO,
+ (host_info64_t)&vm_stats, &count)) {
+ uint64_t physical_memory_used = ((int64_t)vm_stats.active_count +
+ (int64_t)vm_stats.wire_count) * (int64_t)page_size;
+ return physical_memory_used;
+ }
+ return -1;
+#elif defined(WIN32)
+ MEMORYSTATUSEX memory_info;
+ memory_info.dwLength = sizeof(MEMORYSTATUSEX);
+ GlobalMemoryStatusEx(&memory_info);
+ DWORDLONG physical_memory_used = memory_info.ullTotalPhys - memory_info.ullAvailPhys;
+ return physical_memory_used;
+#else
+ static_assert(false, "Unsupported platform");
+#endif
+}
+
+int64_t OsUtils::getSystemTotalPhysicalMemory() {
+#if defined(__linux__)
+ struct sysinfo memory_info;
+ sysinfo(&memory_info);
+ uint64_t total_physical_memory = memory_info.totalram;
+ total_physical_memory *= memory_info.mem_unit;
+ return total_physical_memory;
+#elif defined(__APPLE__)
+ int mib[2];
+ int64_t total_physical_memory = 0;
+ mib[0] = CTL_HW;
+ mib[1] = HW_MEMSIZE;
+ size_t length = sizeof(int64_t);
+ sysctl(mib, 2, &total_physical_memory, &length, NULL, 0);
+ return total_physical_memory;
+#elif defined(WIN32)
+ MEMORYSTATUSEX memory_info;
+ memory_info.dwLength = sizeof(MEMORYSTATUSEX);
+ GlobalMemoryStatusEx(&memory_info);
+ DWORDLONG total_physical_memory = memory_info.ullTotalPhys;
+ return total_physical_memory;
+#else
+ static_assert(false, "Unsupported platform");
+#endif
}
+std::string OsUtils::getMachineArchitecture() {
+#if defined(__linux__) || defined(__APPLE__)
Review comment:
This could be in the else branch of `#if defined(WIN32)`. BSDs have uname, too.
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,187 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
+
+ int positive_int_value = 6;
+ value_node = positive_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE(canConvertToType<uint32_t>(value_node));
+ REQUIRE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE_FALSE(canConvertToType<bool>(value_node));
+ REQUIRE(canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "6");
+
+ int negative_int_value = -7;
+ value_node = negative_int_value;
+ REQUIRE(value_node.getValue()->getTypeIndex() == org::apache::nifi::minifi::state::response::Value::INT_TYPE);
+ REQUIRE_FALSE(canConvertToType<uint32_t>(value_node));
+ REQUIRE_FALSE(canConvertToType<uint64_t>(value_node));
+ REQUIRE(canConvertToType<int32_t>(value_node));
+ REQUIRE(canConvertToType<int64_t>(value_node));
+ REQUIRE(canConvertToType<int> (value_node));
+ REQUIRE_FALSE(canConvertToType<bool>(value_node));
+ REQUIRE(canConvertToType<double>(value_node));
+ REQUIRE(value_node.to_string() == "-7");
+
+ uint32_t max_uint32_value = (uint64_t{1} << 32) - 1;
Review comment:
Implicit narrowing here. I would convert the initializer to either `std::numeric_limits<uint32_t>::max()` (preferred), `static_cast<uint32_t>(-1)` or `~uint32_t{0}` to avoid the conversion altogether.
##########
File path: libminifi/src/utils/ProcessCPUUsageTracker.cpp
##########
@@ -0,0 +1,140 @@
+/**
+ * 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 "utils/ProcessCPUUsageTracker.h"
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <sys/times.h>
+#endif
Review comment:
`times` is POSIX. Since we have no clear macro check for posix, either `!WIN32` or `__unix__` is fine for me.
##########
File path: libminifi/test/unit/ResponseNodeValueTests.cpp
##########
@@ -0,0 +1,187 @@
+/**
+ *
+ * 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 <memory>
+
+#include "../../include/core/state/Value.h"
+#include "../TestBase.h"
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node) {
+ T conversion_target;
+ return value_node.getValue()->convertValue(conversion_target);
+}
+
+template <class T>
+bool canConvertToType(org::apache::nifi::minifi::state::response::ValueNode value_node, const T& expected_result) {
+ T conversion_target;
+ bool canConvert = value_node.getValue()->convertValue(conversion_target);
+ return canConvert && (expected_result == conversion_target);
+}
+
+TEST_CASE("IntValueNodeTests", "[responsenodevaluetests]") {
+ org::apache::nifi::minifi::state::response::ValueNode value_node;
Review comment:
This file could use a type alias and maybe a handful of namespace aliases.
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
Review comment:
Minor, but I prefer to capitalize abbreviations as single words, simply because this allows one to programmatically convert between casing styles, e.g. `XmlParser` could easily be converted to `xml_parser`, but `XMLParser` would likely end up being `x_m_l_parser`, and even with smarter heuristics, programmatic approaches would fail at `SOAPXMLRequest` or `XMLRPCRequest`. The google style guide happens to agree with me [1], but I leave it up to you whether you want to change it or not.
[1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> For the purposes of the naming rules below, a "word" is anything that you would write in English without internal spaces. This includes abbreviations, such as acronyms and initialisms. For names written in mixed case (also sometimes referred to as "camel case" or "Pascal case"), in which the first letter of each word is capitalized, prefer to capitalize abbreviations as single words, e.g., `StartRpc()` rather than `StartRPC()`.
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,190 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_user_(0), previous_total_user_(0),
+ total_user_low_(0), previous_total_user_low_(0),
+ total_sys_(0), previous_total_sys_(0),
+ total_idle_(0), previous_total_idle_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_user_low_ = total_user_low_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILE* file = fopen("/proc/stat", "r");
+ if (fscanf(file, "cpu %lu %lu %lu %lu", &total_user_, &total_user_low_, &total_sys_, &total_idle_) != 4) {
+ total_user_ = previous_total_user_;
+ total_user_low_ = previous_total_user_low_;
+ total_idle_ = previous_total_idle_;
+ total_sys_ = previous_total_sys_;
+ }
+ fclose(file);
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_user_low_ < previous_total_user_low_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_user_low_ == previous_total_user_low_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_user_low_diff = total_user_low_ - previous_total_user_low_;
+ uint64_t total_system_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_user_low_diff + total_system_diff;
+ double percent = static_cast<double>(total_diff)/static_cast<double>(total_diff+total_idle_diff);
+
+ return percent;
+}
+#endif // linux
+
+#ifdef WIN32
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_idle_(0), total_sys_(0), total_user_(0),
+ previous_total_idle_(0), previous_total_sys_(0), previous_total_user_(0) {
+ queryCPUTimes();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTimes();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTimes() {
+ previous_total_user_ = total_user_;
+ previous_total_sys_ = total_sys_;
+ previous_total_idle_ = total_idle_;
+ FILETIME fidle, fsys, fuser;
+ GetSystemTimes(&fidle, &fsys, &fuser);
+ total_user_ = ULARGE_INTEGER{ fuser.dwLowDateTime, fuser.dwHighDateTime }.QuadPart;
+ total_sys_ = ULARGE_INTEGER{ fsys.dwLowDateTime, fsys.dwHighDateTime }.QuadPart;
+ total_idle_ = ULARGE_INTEGER{ fidle.dwLowDateTime, fidle.dwHighDateTime }.QuadPart;
+}
+
+bool SystemCPUUsageTracker::isCurrentQueryOlderThanPrevious() const {
+ return (total_user_ < previous_total_user_ ||
+ total_sys_ < previous_total_sys_ ||
+ total_idle_ < previous_total_idle_);
+}
+
+bool SystemCPUUsageTracker::isCurrentQuerySameAsPrevious() const {
+ return (total_user_ == previous_total_user_ &&
+ total_sys_ == previous_total_sys_ &&
+ total_idle_ == previous_total_idle_);
+}
+
+double SystemCPUUsageTracker::getCPUUsageBetweenLastTwoQueries() const {
+ uint64_t total_user_diff = total_user_ - previous_total_user_;
+ uint64_t total_sys_diff = total_sys_ - previous_total_sys_;
+ uint64_t total_idle_diff = total_idle_ - previous_total_idle_;
+ uint64_t total_diff = total_user_diff + total_sys_diff;
+ double percent = static_cast<double>(total_diff - total_idle_diff) / static_cast<double>(total_diff);
+
+ return percent;
+}
+#endif // windows
+
+#ifdef __APPLE__
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
+ total_ticks_(0), previous_total_ticks_(0),
+ idle_ticks_(0), previous_idle_ticks_(0) {
+ queryCPUTicks();
+}
+
+double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() {
+ queryCPUTicks();
+ if (isCurrentQuerySameAsPrevious() || isCurrentQueryOlderThanPrevious()) {
+ return -1.0;
+ } else {
+ return getCPUUsageBetweenLastTwoQueries();
+ }
+}
+
+void SystemCPUUsageTracker::queryCPUTicks() {
+ host_cpu_load_info_data_t cpuinfo;
+ mach_msg_type_number_t count = HOST_CPU_LOAD_INFO_COUNT;
+ auto query_result = host_statistics(mach_host_self(), HOST_CPU_LOAD_INFO, (host_info_t)&cpuinfo, &count);
+ if (query_result == KERN_SUCCESS) {
+ previous_total_ticks_ = total_ticks_;
+ previous_idle_ticks_ = idle_ticks_;
+ total_ticks_ = 0;
+ for (int i = 0; i < CPU_STATE_MAX; i++) {
+ total_ticks_ += cpuinfo.cpu_ticks[i];
+ }
+ idle_ticks_ = cpuinfo.cpu_ticks[CPU_STATE_IDLE];
+ }
Review comment:
I can't verify this logic because it's incredibly hard to find proper documentation for [`host_statistics`](https://developer.apple.com/documentation/kernel/1502546-host_statistics). Could you explain how you came up with the solution?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
~`static_cast` is preferred (or better yet, `gsl::narrow` if need be)~
there is no need for the cast here, if we really want to ensure that it is being called with `uint64_t` we could go for
```suggestion
used_physical_memory.value = uint64_t{utils::OsUtils::getSystemPhysicalMemoryUsage()};
```
which will compile time prevent narrowing conversion from, whatever the function returns, to `uint64_t`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
~`static_cast` is preferred (or better yet, `gsl::narrow` if need be)~
there is no need for the cast here, if we really want to ensure that it is being called with `uint64_t` we could go for
```suggestion
used_physical_memory.value = uint64_t{utils::OsUtils::getSystemPhysicalMemoryUsage()};
```
which will compile time prevent narrowing conversion from whatever the function returns, to `uint64_t`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r605660476
##########
File path: extensions/http-curl/tests/C2VerifyResourceConsumptionInHeartbeat.cpp
##########
@@ -0,0 +1,133 @@
+/**
Review comment:
yeah, I am not sure anyone uses those nodes and they hardly contain any useful information (almost all info has been ported to DeviceInfoNode and AgentInformation)
I created https://issues.apache.org/jira/browse/MINIFICPP-1536 to handle 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603068827
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -580,6 +573,10 @@ struct SerializedResponseNode {
SerializedResponseNode(const SerializedResponseNode &other) = default;
SerializedResponseNode &operator=(const SerializedResponseNode &other) = default;
+
+ bool empty() {
Review comment:
we could make it const
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r619063072
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTrackerBase() = default;
+ virtual ~ProcessCPUUsageTrackerBase() = default;
+ virtual double getCPUUsageAndRestartCollection() = 0;
+};
+
+#if defined(__linux__) || defined(__APPLE__)
Review comment:
Just a personal thought, but if all the implementation details are different on windows and unix platforms, maybe we could have separate files for windows and unix implementations and include the appropriate header file in compilation time. What do you think? Same for the SystemCPUUsageTracker
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
Review comment:
Could be a bit shorter
``` c++
#ifdef WIN32
#include <cstdint>
#include "windows.h"
#else
#include <time.h>
#endif
```
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
Review comment:
We should use `#pragma once` for all new headers.
##########
File path: libminifi/include/utils/ValueParser.h
##########
@@ -119,6 +119,17 @@ class ValueParser {
return *this;
}
+ ValueParser& parse(double& out) {
+ double result; // NOLINT
Review comment:
Could we just initialize this to 0 to satisfy the linter?
##########
File path: libminifi/include/utils/ValueCaster.h
##########
@@ -0,0 +1,48 @@
+/**
+ *
+ * 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/licenseas/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.
+ */
+
+#ifndef LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+#define LIBMINIFI_INCLUDE_UTILS_VALUECASTER_H_
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+namespace internal {
+
+template<class T, class U>
+bool cast_if_in_range(T in, U& out) {
Review comment:
I'm a bit conflicted about this as usually casts follow the `snake_case` style, but our code should follow the `camelCase` for function names.
##########
File path: libminifi/include/utils/ProcessCPUUsageTracker.h
##########
@@ -0,0 +1,96 @@
+/**
+ * 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.
+ */
+#ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+#define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_
+
+#if defined(__linux__) || defined(__APPLE__)
+#include <time.h>
+#endif
+
+#ifdef WIN32
+#include <cstdint>
+#include "windows.h"
+#endif
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+class ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTrackerBase() = default;
+ virtual ~ProcessCPUUsageTrackerBase() = default;
+ virtual double getCPUUsageAndRestartCollection() = 0;
+};
+
+#if defined(__linux__) || defined(__APPLE__)
+class ProcessCPUUsageTracker : ProcessCPUUsageTrackerBase {
+ public:
+ ProcessCPUUsageTracker();
+ ~ProcessCPUUsageTracker() = default;
+ double getCPUUsageAndRestartCollection() override;
+
+ protected:
+ void queryCPUTimes();
+ bool isCurrentQueryOlderThanPrevious();
+ bool isCurrentQuerySameAsPrevious();
Review comment:
The `isCurrent*` functions could be const. Same comment for `SystemCPUUsageTracker` class.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601411415
##########
File path: libminifi/src/utils/SystemCPUUsageTracker.cpp
##########
@@ -0,0 +1,186 @@
+/**
+ * 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 "utils/SystemCPUUsageTracker.h"
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+#ifdef __linux__
+
+SystemCPUUsageTracker::SystemCPUUsageTracker() :
Review comment:
Yeah that would probably make it easier to maintain.
There are however no current patterns on these and I am not sure which version would be better, currently there are also couple of functions which are similarly handled (#ifdef separated by platform in the function body) (e.g. FileUtils.h get_executable_path)
Maybe we could file a followup Jira where we can discuss how to handle these platformspecific codes? (CPUUsageTrackers, OsUtils, FileUtils etc)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603132719
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -228,36 +225,36 @@ class IntValue : public Value {
}
protected:
- virtual bool getValue(int &ref) {
+ bool getValue(int &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint32_t &ref) {
+ bool getValue(uint32_t &ref) override {
if (value < 0)
return false;
ref = value;
return true;
}
- virtual bool getValue(int64_t &ref) {
+ bool getValue(int64_t &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint64_t &ref) {
+ bool getValue(uint64_t &ref) override {
if (value < 0)
Review comment:
I say we go for it if the refactor is not substantial
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601400402
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
removed the unneccesary casts (we are also returning int64_t instead of uint64_t to be able to signal errors, see comments above)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600354264
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -380,9 +411,59 @@ class Int64Value : public Value {
return false;
}
+ virtual bool getValue(double& /*ref*/) {
+ return false;
+ }
+
int64_t value;
};
+class DoubleValue : public Value {
+ public:
+ explicit DoubleValue(double value)
+ : Value(std::to_string(value)),
+ value(value) {
+ setTypeId<double>();
+ }
+ explicit DoubleValue(const std::string &strvalue)
+ : Value(strvalue) {
+ utils::internal::ValueParser(strvalue).parse(value).parseEnd();
+ setTypeId<double>();
+ }
+
+ double getValue() {
+ return value;
+ }
+
+ protected:
+ virtual bool getValue(int&) {
+ return false;
+ }
+
+ virtual bool getValue(uint32_t&) {
+ return false;
+ }
+
+ virtual bool getValue(int64_t&) {
+ return false;
Review comment:
why disable all casts? others like `IntValue` check the range (quite badly) and cast if possible, should we provide a helper like this, and thread all these `Value::getValue(*)` methods body through it?
```
template<T, U>
bool utils::cast_if_in_range(T in, U& out) {
U result = static_cast<U>(in);
if (static_cast<T>(result) != in) {
return false;
}
out = result;
return true;
}
```
edit: this is an insufficient implementation, see `gsl::narrow` for a proper implementation (it would be great if we could use that instead of writing our own, but that throws (or calls terminate))
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603069419
##########
File path: libminifi/include/core/state/nodes/AgentInformation.h
##########
@@ -427,13 +427,15 @@ class AgentStatus : public StateMonitorNode {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
- if (!repositories_.empty()) {
- serialized.push_back(serializeRepositories());
+ auto serializedRepositories = serializeRepositories();
+ if (!serializedRepositories.empty()) {
+ serialized.push_back(serializedRepositories);
}
serialized.push_back(serializeUptime());
- if (nullptr != monitor_) {
- serialized.push_back(serializeComponents());
+ auto serializedComponents = serializeComponents();
+ if (serializedComponents.empty()) {
Review comment:
is a negation missing here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601393638
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -380,9 +411,59 @@ class Int64Value : public Value {
return false;
}
+ virtual bool getValue(double& /*ref*/) {
+ return false;
+ }
+
int64_t value;
};
+class DoubleValue : public Value {
+ public:
+ explicit DoubleValue(double value)
+ : Value(std::to_string(value)),
+ value(value) {
+ setTypeId<double>();
+ }
+ explicit DoubleValue(const std::string &strvalue)
+ : Value(strvalue) {
+ utils::internal::ValueParser(strvalue).parse(value).parseEnd();
+ setTypeId<double>();
+ }
+
+ double getValue() {
+ return value;
+ }
+
+ protected:
+ virtual bool getValue(int&) {
+ return false;
+ }
+
+ virtual bool getValue(uint32_t&) {
+ return false;
+ }
+
+ virtual bool getValue(int64_t&) {
+ return false;
Review comment:
Thanks, I've added this (extended to work with signed-unsigned casts) function and changed to use it where it makes sense in [d28d399](https://github.com/apache/nifi-minifi-cpp/pull/1032/commits/d28d3994fe633513158fb739edc49f69023b8a39)
Also added a bunch more unit tests to verify the behaviour.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r600388400
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
+ used_physical_memory.value = (uint64_t)utils::OsUtils::getSystemPhysicalMemoryUsage();
Review comment:
`static_cast` is preferred (or better yet, `gsl::narrow` if need be)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r603098402
##########
File path: libminifi/include/core/state/Value.h
##########
@@ -228,36 +225,36 @@ class IntValue : public Value {
}
protected:
- virtual bool getValue(int &ref) {
+ bool getValue(int &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint32_t &ref) {
+ bool getValue(uint32_t &ref) override {
if (value < 0)
return false;
ref = value;
return true;
}
- virtual bool getValue(int64_t &ref) {
+ bool getValue(int64_t &ref) override {
ref = value;
return true;
}
- virtual bool getValue(uint64_t &ref) {
+ bool getValue(uint64_t &ref) override {
if (value < 0)
Review comment:
should we use the new `utils::internal::cast_if_in_range` here as well?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a change in pull request #1032: MINIFICPP-1504: Add Resource consumption data to heartbeats
Posted by GitBox <gi...@apache.org>.
martinzink commented on a change in pull request #1032:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1032#discussion_r601399069
##########
File path: libminifi/include/core/state/nodes/DeviceInformation.h
##########
@@ -353,119 +355,104 @@ class DeviceInfoNode : public DeviceInformation {
std::vector<SerializedResponseNode> serialize() {
std::vector<SerializedResponseNode> serialized;
+ serialized.push_back(serializeIdentifier());
+ serialized.push_back(serializeSystemInfo());
+ serialized.push_back(serializeNetworkInfo());
+
+ return serialized;
+ }
+
+ protected:
+ SerializedResponseNode serializeIdentifier() {
SerializedResponseNode identifier;
identifier.name = "identifier";
identifier.value = device_id_;
+ return identifier;
+ }
- SerializedResponseNode systemInfo;
- systemInfo.name = "systemInfo";
-
- SerializedResponseNode vcores;
- vcores.name = "vCores";
- size_t ncpus = std::thread::hardware_concurrency();
-
- vcores.value = ncpus;
-
- systemInfo.children.push_back(vcores);
-
- SerializedResponseNode ostype;
- ostype.name = "operatingSystem";
- ostype.value = getOperatingSystem();
-
- systemInfo.children.push_back(ostype);
-#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
- SerializedResponseNode mem;
- mem.name = "physicalMem";
-
- uint64_t mema = (size_t) sysconf(_SC_PHYS_PAGES) * (size_t) sysconf(_SC_PAGESIZE);
+ SerializedResponseNode serializeVCoreInfo() {
+ SerializedResponseNode v_cores;
+ v_cores.name = "vCores";
+ v_cores.value = std::thread::hardware_concurrency();
+ return v_cores;
+ }
- mem.value = mema;
+ SerializedResponseNode serializeOperatingSystemType() {
+ SerializedResponseNode os_type;
+ os_type.name = "operatingSystem";
+ os_type.value = getOperatingSystem();
+ return os_type;
+ }
- systemInfo.children.push_back(mem);
-#endif
-#ifndef WIN32
- SerializedResponseNode arch;
- arch.name = "machinearch";
+ SerializedResponseNode serializeTotalPhysicalMemoryInformation() {
+ SerializedResponseNode total_physical_memory;
+ total_physical_memory.name = "physicalMem";
+ total_physical_memory.value = (uint64_t)utils::OsUtils::getSystemTotalPhysicalMemory();
+ return total_physical_memory;
+ }
- utsname buf;
+ SerializedResponseNode serializePhysicalMemoryUsageInformation() {
+ SerializedResponseNode used_physical_memory;
+ used_physical_memory.name = "memoryUtilization";
Review comment:
good catch thanks, I've changed it to memoryUsage in [d28d399](https://github.com/martinzink/nifi-minifi-cpp/commit/d28d3994fe633513158fb739edc49f69023b8a39)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org