szaszm commented on a change in pull request #1152: URL: https://github.com/apache/nifi-minifi-cpp/pull/1152#discussion_r732604183
########## File path: extensions/procfs/CPPLINT.cfg ########## @@ -0,0 +1 @@ +filter=-runtime/int Review comment: Is this still relevant? I didn't see any `long` in the code and this checks for `short`, `long`, and `long long` AFAIK. ########## File path: extensions/procfs/CpuStat.cpp ########## @@ -0,0 +1,116 @@ +/** + * 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 "CpuStat.h" + +namespace org::apache::nifi::minifi::procfs { + +std::optional<CpuStatData> CpuStatData::parseCpuStatLine(std::istringstream& iss) { Review comment: This only depends on the `std::istream` interface, consider relaxing the requirements by accepting `std::istream&`. ########## File path: extensions/procfs/CpuStat.h ########## @@ -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. + */ + +#pragma once + +#include <sstream> +#include <string> +#include <utility> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class CpuStatData { + CpuStatData() = default; + + public: + CpuStatData(const CpuStatData& src) = default; + CpuStatData(CpuStatData&& src) noexcept = default; + + static std::optional<CpuStatData> parseCpuStatLine(std::istringstream& iss); + + bool operator>=(const CpuStatData &rhs) const; + bool operator==(const CpuStatData &rhs) const; + CpuStatData operator-(const CpuStatData &rhs) const; + + uint64_t getUser() const { return user_; } + uint64_t getNice() const { return nice_; } + uint64_t getSystem() const { return system_; } + uint64_t getIdle() const { return idle_; } + uint64_t getIoWait() const { return io_wait_; } + uint64_t getIrq() const { return irq_; } + uint64_t getSoftIrq() const { return soft_irq_; } + uint64_t getSteal() const { return steal_; } + uint64_t getGuest() const { return guest_; } + uint64_t getGuestNice() const { return guest_nice_; } + + uint64_t getIdleAll() const { return idle_ + io_wait_; } + uint64_t getSystemAll() const { return system_ + irq_ + soft_irq_; } + uint64_t getVirtAll() const { return guest_ + guest_nice_; } + uint64_t getTotal() const { return user_ + nice_ + getSystemAll() + getIdleAll() + steal_ + getVirtAll(); } Review comment: Cosider using `[[nodiscard]]` and `noexcept` on getters ########## File path: extensions/procfs/CpuStat.cpp ########## @@ -0,0 +1,116 @@ +/** + * 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 "CpuStat.h" + +namespace org::apache::nifi::minifi::procfs { + +std::optional<CpuStatData> CpuStatData::parseCpuStatLine(std::istringstream& iss) { + CpuStatData data; + iss >> data.user_ >> data.nice_ >> data.system_ >> data.idle_ >> data.io_wait_ >> data.irq_ >> data.soft_irq_ >> data.steal_ >> data.guest_ >> data.guest_nice_; + if (iss.fail()) + return std::nullopt; + data.user_ -= data.guest_; // Guest time is already accounted in usertime + data.nice_ -= data.guest_nice_; + return data; +} + +bool CpuStatData::operator>=(const CpuStatData &rhs) const { + return user_ >= rhs.user_ + && nice_ >= rhs.nice_ + && system_ >= rhs.system_ + && idle_ >= rhs.idle_ + && io_wait_ >= rhs.io_wait_ + && irq_ >= rhs.irq_ + && soft_irq_ >= rhs.soft_irq_ + && steal_ >= rhs.steal_ + && guest_ >= rhs.guest_ + && guest_nice_ >= rhs.guest_nice_; +} Review comment: Does this implementation makes sense? It can happen that both `a < b && b < a` are true. If you want lexicographical comparison instead, you can use an explicitly defaulted `operator<=>` instead of both implemented operators. ########## File path: CMakeLists.txt ########## @@ -587,6 +587,14 @@ if (ENABLE_SYSTEMD) createExtension(SYSTEMD-EXTENSIONS "SYSTEMD EXTENSIONS" "Enabled log collection from journald" "extensions/systemd" "extensions/systemd/tests") endif() +## Add the procfs extension +if(CMAKE_SYSTEM_NAME STREQUAL "Linux") Review comment: This has no extra dependencies other than procfs, which is already present on all systems. I would consider adding it to standard-processors on linux. ########## File path: extensions/procfs/CpuStat.h ########## @@ -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. + */ + +#pragma once + +#include <sstream> +#include <string> +#include <utility> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class CpuStatData { + CpuStatData() = default; + + public: + CpuStatData(const CpuStatData& src) = default; + CpuStatData(CpuStatData&& src) noexcept = default; + + static std::optional<CpuStatData> parseCpuStatLine(std::istringstream& iss); + + bool operator>=(const CpuStatData &rhs) const; + bool operator==(const CpuStatData &rhs) const; + CpuStatData operator-(const CpuStatData &rhs) const; + + uint64_t getUser() const { return user_; } + uint64_t getNice() const { return nice_; } + uint64_t getSystem() const { return system_; } + uint64_t getIdle() const { return idle_; } + uint64_t getIoWait() const { return io_wait_; } + uint64_t getIrq() const { return irq_; } + uint64_t getSoftIrq() const { return soft_irq_; } + uint64_t getSteal() const { return steal_; } + uint64_t getGuest() const { return guest_; } + uint64_t getGuestNice() const { return guest_nice_; } + + uint64_t getIdleAll() const { return idle_ + io_wait_; } + uint64_t getSystemAll() const { return system_ + irq_ + soft_irq_; } + uint64_t getVirtAll() const { return guest_ + guest_nice_; } + uint64_t getTotal() const { return user_ + nice_ + getSystemAll() + getIdleAll() + steal_ + getVirtAll(); } + + private: + uint64_t user_; + uint64_t nice_; + uint64_t system_; + uint64_t idle_; + uint64_t io_wait_; + uint64_t irq_; + uint64_t soft_irq_; + uint64_t steal_; + uint64_t guest_; + uint64_t guest_nice_; +}; + +class CpuStat { + public: + static constexpr const char USER_STR[] = "user time"; + static constexpr const char NICE_STR[] = "nice time"; + static constexpr const char SYSTEM_STR[] = "system time"; + static constexpr const char IDLE_STR[] = "idle time"; + static constexpr const char IO_WAIT_STR[] = "io wait time"; + static constexpr const char IRQ_STR[] = "irq time"; + static constexpr const char SOFT_IRQ_STR[] = "soft irq time"; + static constexpr const char STEAL_STR[] = "steal time"; + static constexpr const char GUEST_STR[] = "guest time"; + static constexpr const char GUEST_NICE_STR[] = "guest nice time"; + + CpuStat(const CpuStat& src) = default; + CpuStat(CpuStat&& src) noexcept = default; + Review comment: The class would be simpler without these lines, since all of the [rule of five](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all) operations are defaulted in one way or another. ########## File path: extensions/procfs/CpuStat.h ########## @@ -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. + */ + +#pragma once + +#include <sstream> +#include <string> +#include <utility> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class CpuStatData { + CpuStatData() = default; + + public: + CpuStatData(const CpuStatData& src) = default; + CpuStatData(CpuStatData&& src) noexcept = default; + + static std::optional<CpuStatData> parseCpuStatLine(std::istringstream& iss); + + bool operator>=(const CpuStatData &rhs) const; + bool operator==(const CpuStatData &rhs) const; + CpuStatData operator-(const CpuStatData &rhs) const; + + uint64_t getUser() const { return user_; } + uint64_t getNice() const { return nice_; } + uint64_t getSystem() const { return system_; } + uint64_t getIdle() const { return idle_; } + uint64_t getIoWait() const { return io_wait_; } + uint64_t getIrq() const { return irq_; } + uint64_t getSoftIrq() const { return soft_irq_; } + uint64_t getSteal() const { return steal_; } + uint64_t getGuest() const { return guest_; } + uint64_t getGuestNice() const { return guest_nice_; } + + uint64_t getIdleAll() const { return idle_ + io_wait_; } + uint64_t getSystemAll() const { return system_ + irq_ + soft_irq_; } + uint64_t getVirtAll() const { return guest_ + guest_nice_; } + uint64_t getTotal() const { return user_ + nice_ + getSystemAll() + getIdleAll() + steal_ + getVirtAll(); } + + private: + uint64_t user_; + uint64_t nice_; + uint64_t system_; + uint64_t idle_; + uint64_t io_wait_; + uint64_t irq_; + uint64_t soft_irq_; + uint64_t steal_; + uint64_t guest_; + uint64_t guest_nice_; +}; + +class CpuStat { + public: + static constexpr const char USER_STR[] = "user time"; + static constexpr const char NICE_STR[] = "nice time"; + static constexpr const char SYSTEM_STR[] = "system time"; + static constexpr const char IDLE_STR[] = "idle time"; + static constexpr const char IO_WAIT_STR[] = "io wait time"; + static constexpr const char IRQ_STR[] = "irq time"; + static constexpr const char SOFT_IRQ_STR[] = "soft irq time"; + static constexpr const char STEAL_STR[] = "steal time"; + static constexpr const char GUEST_STR[] = "guest time"; + static constexpr const char GUEST_NICE_STR[] = "guest nice time"; + + CpuStat(const CpuStat& src) = default; + CpuStat(CpuStat&& src) noexcept = default; + + CpuStat(std::string cpu_name, CpuStatData data) : cpu_name_(std::move(cpu_name)), data_(data) {} + + void addToJson(rapidjson::Value& body, rapidjson::Document::AllocatorType& alloc) const; + const CpuStatData& getData() const { return data_; } + const std::string& getName() const { return cpu_name_; } + + protected: + std::string cpu_name_; + CpuStatData data_; +}; + +class CpuStatPeriod { + CpuStatPeriod(std::string cpu_name, double period, CpuStatData data_diff) : cpu_name_(std::move(cpu_name)), period_(period), data_diff_(data_diff) {} + + public: + static constexpr const char USER_PERCENT_STR[] = "user time %"; + static constexpr const char NICE_PERCENT_STR[] = "nice time %"; + static constexpr const char SYSTEM_PERCENT_STR[] = "system time %"; + static constexpr const char IDLE_PERCENT_STR[] = "idle time %"; + static constexpr const char IO_WAIT_PERCENT_STR[] = "io wait time %"; + static constexpr const char IRQ_PERCENT_STR[] = "irq time %"; + static constexpr const char SOFT_IRQ_PERCENT_STR[] = "soft irq %"; + static constexpr const char STEAL_PERCENT_STR[] = "steal time %"; + static constexpr const char GUEST_PERCENT_STR[] = "guest time %"; + static constexpr const char GUEST_NICE_PERCENT_STR[] = "guest nice time %"; + + CpuStatPeriod(const CpuStatPeriod& src) = default; + CpuStatPeriod(CpuStatPeriod&& src) noexcept = default; Review comment: same here ########## File path: extensions/procfs/CpuStat.h ########## @@ -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. + */ + +#pragma once + +#include <sstream> +#include <string> +#include <utility> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class CpuStatData { + CpuStatData() = default; + + public: + CpuStatData(const CpuStatData& src) = default; + CpuStatData(CpuStatData&& src) noexcept = default; Review comment: Follow the [rule of zero](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c20-if-you-can-avoid-defining-default-operations-do) ########## File path: extensions/procfs/MemInfo.h ########## @@ -0,0 +1,51 @@ +/** + * 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. + */ + +#pragma once + +#include <string> +#include <sstream> +#include <fstream> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class MemInfo { + MemInfo() = default; + + public: + MemInfo(const MemInfo& src) = default; + MemInfo(MemInfo&& src) noexcept = default; + static std::optional<MemInfo> parseMemInfoFile(std::ifstream& mem_info_file); + + void addToJson(rapidjson::Value& body, rapidjson::Document::AllocatorType& alloc) const; + + uint64_t getTotalMemory() const { return memory_total_; } + uint64_t getFreeMemory() const { return memory_free_; } + uint64_t getAvailableMemory() const { return memory_available_; } + uint64_t getTotalSwap() const { return swap_total_; } + uint64_t getFreeSwap() const { return swap_free_; } Review comment: Please consider adding `[[nodiscard]]` and `noexcept` to all getters. ########## File path: extensions/procfs/DiskStat.h ########## @@ -0,0 +1,152 @@ +/** + * 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. + */ + +#pragma once + +#include <sstream> +#include <string> +#include <chrono> +#include <optional> +#include "rapidjson/document.h" + +namespace org::apache::nifi::minifi::procfs { + +class DiskStatData { + DiskStatData() = default; + + public: + struct MonotonicIncreasingMembers { + uint64_t reads_completed_; + uint64_t reads_merged_; + uint64_t sectors_read_; + uint64_t milliseconds_spent_reading_; + uint64_t writes_completed_; + uint64_t writes_merges_; + uint64_t sectors_written_; + uint64_t milliseconds_spent_writing_; + uint64_t milliseconds_spent_io_; + uint64_t weighted_milliseconds_spent_io_; + + bool operator>=(const MonotonicIncreasingMembers& rhs) const { + return reads_completed_ >= rhs.reads_completed_ + && reads_merged_ >= rhs.reads_merged_ + && sectors_read_ >= rhs.sectors_read_ + && milliseconds_spent_reading_ >= rhs.milliseconds_spent_reading_ + && writes_completed_ >= rhs.writes_completed_ + && writes_merges_ >= rhs.writes_merges_ + && sectors_written_ >= rhs.sectors_written_ + && milliseconds_spent_writing_ >= rhs.milliseconds_spent_writing_ + && milliseconds_spent_io_ >= rhs.milliseconds_spent_io_ + && weighted_milliseconds_spent_io_ >= rhs.weighted_milliseconds_spent_io_; + } + }; + static std::optional<DiskStatData> parseDiskStatLine(std::istringstream& iss) { + DiskStatData disk_stat_data; + iss >> disk_stat_data.major_device_number_ >> disk_stat_data.minor_device_number_ >> disk_stat_data.disk_name >> disk_stat_data.monotonic_increasing_members_.reads_completed_ + >> disk_stat_data.monotonic_increasing_members_.reads_merged_ >> disk_stat_data.monotonic_increasing_members_.sectors_read_ + >> disk_stat_data.monotonic_increasing_members_.milliseconds_spent_reading_ >> disk_stat_data.monotonic_increasing_members_.writes_completed_ + >> disk_stat_data.monotonic_increasing_members_.writes_merges_ >> disk_stat_data.monotonic_increasing_members_.sectors_written_ + >> disk_stat_data.monotonic_increasing_members_.milliseconds_spent_reading_ >> disk_stat_data.monotonic_increasing_members_.writes_completed_ + >> disk_stat_data.monotonic_increasing_members_.writes_merges_ >> disk_stat_data.monotonic_increasing_members_.sectors_written_ + >> disk_stat_data.monotonic_increasing_members_.milliseconds_spent_writing_ >> disk_stat_data.ios_in_progress_ >> disk_stat_data.ios_in_progress_ + >> disk_stat_data.monotonic_increasing_members_.milliseconds_spent_io_ >> disk_stat_data.monotonic_increasing_members_.weighted_milliseconds_spent_io_; + + if (iss.fail()) + return std::nullopt; + return disk_stat_data; + } + + uint64_t getMajorDeviceNumber() const { return major_device_number_; } + uint64_t getMinorDeviceNumber() const { return minor_device_number_; } + const std::string& getDiskName() const { return disk_name; } + uint64_t getReadsCompleted() const { return monotonic_increasing_members_.reads_completed_; } + uint64_t getReadsMerged() const { return monotonic_increasing_members_.reads_merged_; } + uint64_t getSectorsRead() const { return monotonic_increasing_members_.sectors_read_; } + uint64_t getMillisecondsSpentReading() const { return monotonic_increasing_members_.milliseconds_spent_reading_; } + uint64_t getWritesCompleted() const { return monotonic_increasing_members_.writes_completed_; } + uint64_t getWritesMerged() const { return monotonic_increasing_members_.writes_merges_; } + uint64_t getSectorsWritten() const { return monotonic_increasing_members_.sectors_written_; } + uint64_t getMillisecondsSpentWriting() const { return monotonic_increasing_members_.milliseconds_spent_writing_; } + uint64_t getIosInProgress() const { return ios_in_progress_; } + uint64_t getMillisecondsSpentIo() const { return monotonic_increasing_members_.milliseconds_spent_io_; } + uint64_t getWeightedMillisecondsSpentIo() const { return monotonic_increasing_members_.weighted_milliseconds_spent_io_; } + + const MonotonicIncreasingMembers& getMonotonicIncreasingMembers() const { return monotonic_increasing_members_; } + + private: + uint64_t major_device_number_; + uint64_t minor_device_number_; + std::string disk_name; + MonotonicIncreasingMembers monotonic_increasing_members_; + uint64_t ios_in_progress_; +}; + +class DiskStat { + public: + static constexpr const char DEVICE_MAJOR_NUMBER_STR[] = "Major Device Number"; + static constexpr const char DEVICE_MINOR_NUMBER_STR[] = "Minor Device Number"; + static constexpr const char READS_COMPLETED_STR[] = "Reads Completed"; + static constexpr const char READS_MERGED_STR[] = "Reads Merged"; + static constexpr const char SECTORS_READ_STR[] = "Sectors Read"; + static constexpr const char WRITES_COMPLETED_STR[] = "Writes Completed"; + static constexpr const char WRITES_MERGED_STR[] = "Writes Merged"; + static constexpr const char SECTORS_WRITTEN_STR[] = "Sectors Written"; + static constexpr const char IOS_IN_PROGRESS_STR[] = "IO-s in progress"; + + DiskStat(DiskStatData disk_stat_data, std::chrono::time_point<std::chrono::system_clock> time_point) : data_(disk_stat_data), time_point_(time_point) {} + void addToJson(rapidjson::Value& body, rapidjson::Document::AllocatorType& alloc) const; Review comment: Avoid mixing the data and serialization, because of the SRP. Would it be possible to refactor the code in a way that the data is an object and the serialization function is a free function that serializes the data? It doesn't even have to depend on rapidjson, just take a callback and call it for every data point. ```c++ void serialize(const DiskStatData& data, std::invocable<std::string_view /* key */, std::string_view /* value */> auto append_callback) { append_callback(DEVICE_MAJOR_NUMBER_STR, major_device_number_); // ... } // on the call site: serialize(data, [&device_stat_json, &alloc](std::string_view key, std::string_view data) { device_stat_json.AddMember(key, value, alloc); }); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org