MINIFICPP-1504: Add Resource consumption data to heartbeats#1032
MINIFICPP-1504: Add Resource consumption data to heartbeats#1032martinzink wants to merge 10 commits intoapache:mainfrom
Conversation
| int64_t value; | ||
| }; | ||
|
|
||
| class DoubleValue : public Value { |
There was a problem hiding this comment.
This class is required to be able to parse and deparse doubles in the heartbeat
libminifi/include/utils/OsUtils.h
Outdated
| /// Returns memory usage in bytes, including shared memory | ||
| uint64_t getMemoryUsage(); | ||
| /// Returns physical memory usage by the current process in bytes | ||
| uint64_t getCurrentProcessPhysicalMemoryUsage(); |
There was a problem hiding this 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.
| std::string ip_; | ||
| std::string device_id_; | ||
| static utils::SystemCPUUsageTracker cpu_load_tracker_; | ||
| static std::mutex cpu_load_tracker_mutex_; |
There was a problem hiding this 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.
| utsname buf; | ||
| SerializedResponseNode serializePhysicalMemoryUsageInformation() { | ||
| SerializedResponseNode used_physical_memory; | ||
| used_physical_memory.name = "memoryUtilization"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
good catch thanks, I've changed it to memoryUsage in d28d399
arpadboda
left a comment
arpadboda
left a comment
There was a problem hiding this comment.
Mostly looks good, added some comments
| SerializedResponseNode queuesize; | ||
| queuesize.name = "size"; | ||
| queuesize.value = repo.second->getRepoSize(); | ||
| for (auto &repo : repositories_) { |
There was a problem hiding this comment.
Does it modify anything on the original container?
I don't see such line, so I would go with const & here
| SerializedResponseNode componentsNode(false); | ||
| componentsNode.name = "components"; | ||
|
|
||
| for (auto component : components) { |
There was a problem hiding this comment.
I know it's old code, but as we touch it - copy seems to be needless here.
| return components_node; | ||
| } | ||
|
|
||
| SerializedResponseNode serializeAgentMemoryUsage() { |
|
|
||
| double SystemCPUUsageTracker::getCPUUsageAndRestartCollection() { | ||
| queryCPUTimes(); | ||
| if (isCurrentQuerySameAsPrevious() || isCurrentQuerySameAsPrevious()) { |
There was a problem hiding this 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
| #ifndef LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_ | ||
| #define LIBMINIFI_INCLUDE_UTILS_PROCESSCPUUSAGETRACKER_H_ |
There was a problem hiding this comment.
We should use #pragma once for all new headers.
| #if defined(__linux__) || defined(__APPLE__) | ||
| #include <time.h> | ||
| #endif | ||
|
|
||
| #ifdef WIN32 | ||
| #include <cstdint> | ||
| #include "windows.h" | ||
| #endif |
There was a problem hiding this comment.
Could be a bit shorter
#ifdef WIN32
#include <cstdint>
#include "windows.h"
#else
#include <time.h>
#endif| virtual double getCPUUsageAndRestartCollection() = 0; | ||
| }; | ||
|
|
||
| #if defined(__linux__) || defined(__APPLE__) |
There was a problem hiding this 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
There was a problem hiding this comment.
I see that you've already discussed this, it's okay to have it in a followup.
| bool isCurrentQueryOlderThanPrevious(); | ||
| bool isCurrentQuerySameAsPrevious(); |
There was a problem hiding this comment.
The isCurrent* functions could be const. Same comment for SystemCPUUsageTracker class.
| namespace internal { | ||
|
|
||
| template<class T, class U> | ||
| bool cast_if_in_range(T in, U& out) { |
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this comment.
I don't have any strong preference here either, so you can keep it as it is.
| } | ||
|
|
||
| ValueParser& parse(double& out) { | ||
| double result; // NOLINT |
There was a problem hiding this comment.
Could we just initialize this to 0 to satisfy the linter?
| return -1; | ||
| return pmc.WorkingSetSize; | ||
| #else | ||
| static_assert(false, "Unsupported platform"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks thats a valid point, changed it to only warn and give back -1 as result in #1065
Signed-off-by: Arpad Boda <aboda@apache.org> This closes apache#1032
5 participants
Manual testing:
In MemoryUsageTest and CPUUsageTest with cout_enabled, one can manually verify the resource consumption with some thirdparty tool.
I've tested and got appropriate results on
These test results are available on MINIFICPP-1504
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:
For documentation related changes:
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.