feat: support network IO traffic monitoring #1733

yaoyinnan · 2023-07-15T03:55:23Z

Support network IO traffic monitoring. Including IO bytes and kps of Redis requests and master-slave replication.

Including the following indicators:

total_net_input_bytes: The total number of bytes read from the network
total_net_output_bytes: The total number of bytes written to the network
total_net_repl_input_bytes: The total number of bytes read from the network for replication purposes
total_net_repl_output_bytes: The total number of bytes written to the network for replication purposes
instantaneous_input_kbps: The network's read rate per second in KB/sec
instantaneous_output_kbps: The network's write rate per second in KB/sec
instantaneous_input_repl_kbps: The network's read rate per second in KB/sec for replication purposes
instantaneous_output_repl_kbps: The network's write rate per second in KB/sec for replication purposes

Execute the info stats command to view:

total_net_input_bytes:7655
total_net_output_bytes:84932
total_net_repl_input_bytes:5216
total_net_repl_output_bytes:4013
instantaneous_input_kbps:0.0751953
instantaneous_output_kbps:0.988281
instantaneous_input_repl_kbps:0.03125
instantaneous_output_repl_kbps:0.0185547

Collection rules:

For the Net IO traffic of Redis, it is collected in GetRequest and SendReply of RedisConn.
For the Net IO traffic of master-slave replication, it is collected in GetRequest and SendReply of PbConn.
For Net IO rate, the average per second is calculated by recording the value every 0.1s. Use an instantaneous_metric thread is started to collect instantaneous data every 0.1s.

Shown in Grafana:

Fixes: #1732

cr-gpt · 2023-07-15T03:55:41Z

include/pika_instant.h

+    std::unordered_map<std::string, inst_metric> inst_metrics_;
+};
+
+#endif  // PIKA_PIKA_INSTANT_H


Based on the provided code patch, here is a brief code review:

Licensing: The copyright and licensing information at the beginning of the file seem to be in order.

Header Guards: The header guards (#ifndef and #define) are correctly used to prevent multiple inclusion of the header file.

Includes: The necessary includes (<string> and <unordered_map>) are present.

Magic Numbers: The constant value STATS_METRIC_SAMPLES appears to be a magic number. Consider giving it a meaningful name or documenting its purpose.

Inline Constants: Using inline for the constants STATS_METRIC_NET_INPUT, STATS_METRIC_NET_OUTPUT, STATS_METRIC_NET_INPUT_REPLICATION, and STATS_METRIC_NET_OUTPUT_REPLICATION is unnecessary since they are defined in the header file. You can define them outside the class definition to avoid potential conflicts when including the header file in multiple places.

Macro Usage: The macro usage in run_with_period(_ms_) is generally discouraged, as it can lead to code that is harder to reason about and maintain. Consider refactoring this into a function or lambda.

Struct Definition: The inst_metric struct seems fine, although the purpose of its members could benefit from documentation or additional context.

Class Definition: The Instant class looks reasonable. It has public member functions for tracking and retrieving instantaneous metrics, and a private data member inst_metrics_, which is an unordered map of metric names to inst_metric structs.

Overall, the code patch seems to be well-structured and free from obvious bugs. However, further evaluation would require knowledge of the overall program structure and design goals.

cr-gpt · 2023-07-15T03:55:54Z

include/pika_monotonic_time.h

+
+#endif  // PIKA_MONOTONIC_TIME_H
+
+


The provided code patch appears to define a header file for obtaining monotonic time in microseconds. Here are some suggestions for improvement and potential bug risks to consider:

License and copyright: Ensure that the information in the comments accurately reflects the licensing and copyright details for your code.

Include guards: Including #ifndef and #define directives ensure that the header file is included only once, preventing duplicate definitions if the header is included multiple times.

Use the <chrono> library: Instead of using typedef uint64_t monotime, you can leverage the C++11 <chrono> library to provide a more standard and flexible way of handling time durations and points in time.

Consider using std::int64_t: If negative time values are not expected, you could use std::uint64_t as the type for monotime.

Error handling: Implement appropriate error-handling mechanisms if the function getMonotonicUs() encounters errors such as failures in obtaining the monotonic time value.

Unit testing: It would be beneficial to include unit tests to verify the correctness and accuracy of the getMonotonicUs() function.

Make sure you review the rest of your codebase to ensure that this patch is integrated correctly and consistently throughout your project.

yaoyinnan · 2023-07-15T04:11:35Z

I'm not sure if monotonic_time is doing the right thing, it has to be compatible with x86 and arm of linux and darwin, and each OS and architecture may have different underlying acquisition time. Please take a look here.

cr-gpt · 2023-07-15T08:15:54Z

include/pika_instant.h

+    std::unordered_map<std::string, inst_metric> inst_metrics_;
+};
+
+#endif  // PIKA_PIKA_INSTANT_H


The code patch appears to introduce a new header file called "pika_instant.h" that defines a class called "Instant" and a structure called "inst_metric." Here's a brief code review:

Licensing: The license information at the top of the file suggests that this code is copyrighted by Qihoo, Inc. and is licensed under the BSD-style license. It also mentions an additional grant of patent rights. If you are using this code, make sure you comply with the licensing terms.

Header guards: The header file has appropriate include guards to prevent multiple inclusions.

Dependencies: The code includes the necessary headers for std::string and std::unordered_map.

Constants: Several constant strings are defined using the inline const std::string syntax. This allows them to be used as constants while providing flexibility during compilation.

Macro: The run_with_period macro is defined but not used in the code snippet you provided. Make sure it is being used correctly elsewhere.

Structures and classes:

The inst_metric structure holds fields related to tracking instantaneous metrics, such as samples and index.

The Instant class encapsulates functionality to track and retrieve instantaneous metrics.

The public interface includes functions trackInstantaneousMetric and getInstantaneousMetric, which allow tracking and retrieving metric values, respectively.

Private member: The class has a private member, inst_metrics_, which is an unordered map that associates each metric name (a string) with an instance of inst_metric. This allows storing and managing multiple metrics within the Instant class.

Overall, without seeing the implementation details of the class methods, it is difficult to assess the correctness or potential improvements. However, based on the provided code, the structure looks reasonable, ensuring encapsulation of instantaneous metric tracking.

cr-gpt · 2023-07-15T08:16:05Z

include/pika_monotonic_time.h

+
+#endif  // PIKA_MONOTONIC_TIME_H
+
+


The code patch you provided appears to be a header file named "pika_monotonic_time.h." Below is a brief code review:

Licensing: The code includes a copyright notice and mentions the BSD-style license. However, since I can't see the contents of the LICENSE and PATENTS files, I can't verify the licensing compliance.

Header guards: The header file contains proper header guards to prevent multiple inclusions.

Dependencies: The code relies on <cstdint>, suggesting the usage of fixed-size integer types such as uint64_t.

Typedef: The code defines a typedef for monotime, which represents a counter in microseconds.

Function declaration: The code declares a function getMonotonicUs() that returns a monotime value. Presumably, this function retrieves the current monotonic time in microseconds.

Overall, the code seems fine from the provided information. However, without further context or implementation details, it's difficult to identify specific bug risks or improvement suggestions.

cr-gpt · 2023-07-15T11:28:01Z

include/pika_instant.h

+    std::unordered_map<std::string, inst_metric> inst_metrics_;
+};
+
+#endif  // PIKA_PIKA_INSTANT_H


The provided code patch appears to define a class called Instant and some related structures and functions. Here are some observations and suggestions for improvement:

In the Instant class, it would be better to mark the default constructor and destructor as default explicitly (e.g., Instant() = default;, ~Instant() = default;) instead of relying on implicit generation.

The stat_metrics definitions could be moved outside the class and declared as constant variables rather than using inline const in the class. For example:

const std::string STATS_METRIC_NET_INPUT = "stats_metric_net_input"; const std::string STATS_METRIC_NET_OUTPUT = "stats_metric_net_output"; // ...

The use of macros (#define) for STATS_METRIC_SAMPLES and run_with_period should be avoided. Prefer using constant values or constexpr variables instead.

It's generally recommended to use explicit access specifiers (public, protected, private) when declaring class members. So, consider adding public: before the trackInstantaneousMetric and getInstantaneousMetric function declarations.

The trackInstantaneousMetric function takes four parameters. It would be useful to provide comments describing each parameter's purpose and expected values.

Consider validating the input parameters within the trackInstantaneousMetric function to ensure they are within valid ranges or handle potential edge cases.

The getInstantaneousMetric function returns a uint64_t without any indication of failure. Depending on your specific use case, you might want to add error handling or specify a default value when the requested metric is not found.

The inst_metrics_ member in the Instant class uses an unordered_map to store metrics. Ensure that there are no concurrency issues if this class is used in a multi-threaded context. Consider using appropriate synchronization mechanisms (e.g., locks) if necessary.

Overall, the provided code patch seems to define a structure for tracking instantaneous metrics. However, without knowing the broader context and usage of this code, it's challenging to assess its correctness or identify potential bug risks specific to your application. Reviewing the surrounding code and considering the intended use of the Instant class would provide more insights into its overall quality.

cr-gpt · 2023-07-15T12:13:59Z

include/pika_instant.h

+    std::unordered_map<std::string, inst_metric> inst_metrics_;
+};
+
+#endif  // PIKA_PIKA_INSTANT_H


The provided code appears to define a class called Instant and some related structures and constants. Here is a brief review of the code:

Licensing: The code includes licensing information, which is good practice.

Header Guards: The code uses header guards (#ifndef and #endif) to prevent multiple inclusions of the header file, which is also good practice.

Standard Library Includes: The code includes the necessary header files (<string> and <unordered_map>) for the standard library classes it uses.

Constant Definitions: The code defines several constants using inline const std::string, which is fine. However, consider moving them outside the header file and into a source file, as they may cause linker errors if included in multiple translation units.

Macro Definition: The code defines a macro called run_with_period. It's generally recommended to avoid macros whenever possible due to various difficulties and potential issues they can introduce. Consider refactoring this into a function or using a lambda instead.

Data Structure: The code defines a data structure called inst_metric, which tracks instantaneous metrics. It uses an array to store samples and includes some members for managing the samples. This structure seems reasonable for its intended purpose.

Class Definition: The Instant class tracks instantaneous metrics using an unordered map to associate metric names with inst_metric objects. The class provides methods to track and retrieve instantaneous metrics. The implementation of these methods is not shown, so it's difficult to provide specific suggestions for improvement or bug risk assessment.

General Suggestions:

Consider following consistent naming conventions for variables, functions, and classes (e.g., camelCase or snake_case).

Examine the implementation of the trackInstantaneousMetric and getInstantaneousMetric methods to ensure correct functionality and error handling.

Verify that the std::unordered_map usage in the Instant class is appropriate for your requirements.

Consider writing unit tests to verify the behavior of the code and catch any potential bugs.

Overall, without a complete implementation or context, it's challenging to provide a detailed review. Please ensure you test the code thoroughly and consider the given suggestions as general guidelines.

chejinge · 2023-07-17T01:53:09Z

include/pika_instant.h

+#include <string>
+#include <unordered_map>
+
+#define STATS_METRIC_SAMPLES 16     /* Number of samples per metric. */


define改为const

Changed to inline constexpr size_t.

chejinge · 2023-07-17T01:53:38Z

include/pika_instant.h

+inline const std::string STATS_METRIC_NET_INPUT_REPLICATION = "stats_metric_net_input_replication";
+inline const std::string STATS_METRIC_NET_OUTPUT_REPLICATION = "stats_metric_net_output_replication";
+
+#define run_with_period(_ms_) if (((_ms_) <= 1000/server.hz) || !(server.cronloops%((_ms_)/(1000/server.hz))))


这里用lamda表达式

This function is actually not used and has been deleted. The timing execution is modified to start a thread at pika_server::Start() to collect instantaneous indicators at regular intervals.

chejinge · 2023-07-17T01:54:23Z

include/pika_server.h

+  uint64_t NetInputBytes();
+  uint64_t NetOutputBytes();
+  uint64_t NetReplInputBytes();
+  uint64_t NetReplOutputBytes();


uint64_t统一改为size_t

chejinge · 2023-07-17T01:55:15Z

src/net/include/net_stats.h

+  std::atomic<uint64_t> stat_net_input_bytes = {0}; /* Bytes read from network. */
+  std::atomic<uint64_t> stat_net_output_bytes = {0}; /* Bytes written to network. */
+  std::atomic<uint64_t> stat_net_repl_input_bytes = {0}; /* Bytes read during replication, added to stat_net_input_bytes in 'info'. */
+  std::atomic<uint64_t> stat_net_repl_output_bytes = {0}; /* Bytes written during replication, added to stat_net_output_bytes in 'info'. */


= {}二选一，不要两个都写

chejinge · 2023-07-17T01:55:50Z

src/net/src/net_stats.cc

+
+namespace net {
+
+uint64_t NetworkStatistic::NetInputBytes() { return stat_net_input_bytes.load(std::memory_order_relaxed); }


return 和return的内容放在同一行

chejinge · 2023-07-17T01:56:19Z

src/net/src/net_stats.cc

+
+uint64_t NetworkStatistic::NetReplOutputBytes() { return stat_net_repl_output_bytes.load(std::memory_order_relaxed); }
+
+void NetworkStatistic::IncrInputBytes(uint64_t bytes) { stat_net_input_bytes.fetch_add(bytes, std::memory_order_relaxed); }


函数体放在下一行

chejinge · 2023-07-17T01:57:46Z

src/pika_instant.cc

+  if (inst_metrics_[metric].last_sample_base > 0) {
+    uint64_t base = current_base - inst_metrics_[metric].last_sample_base;
+    uint64_t value = current_value - inst_metrics_[metric].last_sample_value;
+    uint64_t avg = base > 0 ? (value * factor / base) : 0;


uint64_t可以统一改成size_t

src/pika_instant.cc

chejinge · 2023-07-17T01:59:37Z

src/pika_server.cc

@@ -1446,6 +1486,19 @@ void PikaServer::AutoKeepAliveRSync() {
  }
 }

+void PikaServer::AutoInstantaneousMetric() {
+  monotime current_time = getMonotonicUs();
+  uint64_t factor = 1000000; // us


cr-gpt · 2023-07-17T02:37:02Z

include/pika_instant.h

+    std::unordered_map<std::string, inst_metric> inst_metrics_;
+};
+
+#endif  // PIKA_PIKA_INSTANT_H


The code patch appears to define a class called Instant and a corresponding structure inst_metric. The Instant class is used for tracking instantaneous metrics, such as the number of operations per second and network traffic. Here's a brief review and some suggestions:

License: The code includes a copyright notice and license information, which is good practice.

Naming Conventions: The naming conventions in the code seem reasonable, with meaningful names for classes, variables, and constants.

Header Guards: The header file includes proper header guards (#ifndef...#endif), preventing multiple inclusions of the same header.

Dependencies: The code relies on the C++ standard library (<string>, <unordered_map>). Ensure that these dependencies are correctly included, given the overall context of the codebase.

Magic Numbers: The use of the magic number 16 (STATS_METRIC_SAMPLES) should be replaced with a named constant or variable to improve code readability and maintainability.

Class Design: The Instant class seems well-designed for its purpose. It provides methods to track and retrieve instantaneous metrics. However, it may be helpful to add more documentation (comments or function descriptions) clarifying their usage.

Error Handling: Ensure that there are appropriate error handling mechanisms in place for cases when the requested metric does not exist.

Memory Management: The Instant class uses the default constructor and destructor, which is fine unless there is specific resource management involved. Verify that no additional memory allocation or deallocation needs to be performed.

Overall, the code patch looks reasonable, but a comprehensive review would depend on understanding the broader context and requirements of the codebase.

cr-gpt · 2023-07-17T02:37:16Z

include/pika_monotonic_time.h

+
+#endif  // PIKA_MONOTONIC_TIME_H
+
+


The provided code patch appears to define a C++ header file named "pika_monotonic_time.h," which includes functions and type definitions related to obtaining monotonic time in microseconds.

Here's a brief code review:

Licensing: The copyright and license information indicate that the code is copyrighted by Qihoo, Inc. and is licensed under the BSD-style license. It's generally good practice to include licensing information in the code.

Header Guards: The inclusion of header guards (#ifndef/#define/#endif) ensures that the contents of the header are only included once, preventing duplicate definitions if the header is included multiple times in a source file.

Type Definition: The code defines a monotime type as an alias for uint64_t. This allows for better documentation and clarification when using variables associated with monotonic time.

Function Declaration: The getMonotonicUs() function is declared, which suggests that it retrieves the current monotonic time in microseconds.

Overall, the code patch seems straightforward and does not raise any apparent bug risks. However, without further context or the implementation details of the getMonotonicUs() function, it's not possible to provide more specific improvement suggestions.

To ensure the accuracy and correctness of the code, it's recommended to thoroughly test and verify the behavior of the getMonotonicUs() function once it's implemented.

luky116 · 2023-07-17T01:47:58Z

include/pika_instant.h

+
+/* The following two are used to track instantaneous metrics, like
+* number of operations per second, network traffic. */
+struct inst_metric{


是否用大小写

是的，它应该大写，修改了。

luky116 · 2023-07-17T03:16:18Z

include/pika_server.h

+ /*
+  * Instantaneous Metric used
+  */
+ Instant instant_;


1、格式缩进对齐
2、考虑下是否使用 std::unique_ptr

luky116 · 2023-07-17T03:33:18Z

src/pika_server.cc

@@ -192,6 +206,8 @@ void PikaServer::Start() {
    }
  }
  LOG(INFO) << "Goodbye...";
+
+  instantaneous_metric.join();


为啥是在 pika exit 的时候进行 join 呢？

我想了想，这里不需要 join，instant_metric是后台线程，不需要阻塞主线程。我去掉了。

luky116 · 2023-07-17T03:36:29Z

src/net/src/redis_conn.cc

@@ -87,6 +87,7 @@ ReadStatus RedisConn::GetRequest() {
  }

  nread = read(fd(), rbuf_ + next_read_pos, remain);
+  g_network_statistic->IncrInputBytes(nread);


一个是在 redis_con 监控，一个是在 pb_conn 监控，命名方式可以考虑统一下：
IncrRedisInputBytes
IncrRedisOutputBytes
和
IncrReplInputBytes
IncrReplOutputBytes

是的，这样区分比较好，已经修改了。

luky116 · 2023-07-17T03:39:31Z

src/pika_admin.cc

+  tmp_stream << "total_net_output_bytes:" << g_pika_server->NetOutputBytes() + g_pika_server->NetReplOutputBytes() << "\r\n";
+  tmp_stream << "total_net_repl_input_bytes:" << g_pika_server->NetReplInputBytes() << "\r\n";
+  tmp_stream << "total_net_repl_output_bytes:" << g_pika_server->NetReplOutputBytes() << "\r\n";
+  tmp_stream << "instantaneous_input_kbps:" << g_pika_server->InstantaneousInputKbps() << "\r\n";


是否要说明是 pika 命令相关的瞬时流量？

这里instantaneous_input_kbps所代表的是Redis 命令和Repl的瞬时流量之和。当部署单节点时，total_net_output_bytes和instantaneous_input_kbps只代表Redis 命令的流量。当部署主从集群的时候，只需要通过差值就可以得到 Redis 命令的流量。

Support network IO traffic monitoring. Including IO bytes and kps of Redis requests and master-slave replication. Fixes: OpenAtomFoundation#1732 Signed-off-by: yaoyinnan <[email protected]>

cr-gpt · 2023-07-17T08:09:45Z

src/net/src/redis_conn.cc

@@ -129,6 +130,7 @@ WriteStatus RedisConn::SendReply() {
  size_t wbuf_len = response_.size();
  while (wbuf_len > 0) {
    nwritten = write(fd(), response_.data() + wbuf_pos_, wbuf_len - wbuf_pos_);
+    g_network_statistic->IncrRedisOutputBytes(nwritten);
    if (nwritten <= 0) {
      break;
    }


Code Review:

The inclusion of <climits> and <string> headers seems to be unnecessary as they are not used in the code.

You have included a new header file "net/include/net_stats.h", but it is missing from the patch. Make sure it is properly included and defined.

It is assumed that g_network_statistic is defined and initialized elsewhere in the code since it is used in the RedisConn class.

The use of read() and write() functions without proper error handling can be improved. Instead of checking for specific errno values, consider using non-blocking I/O or handling errors more explicitly.

Generally, it is recommended to use RAII (Resource Acquisition Is Initialization) to manage resources like file descriptors (fd()). Consider using smart pointers or wrapper classes to ensure proper resource management.

There might be additional improvements or bug risks specific to the functionality implemented in RedisConn that cannot be identified solely based on this code snippet. A thorough review of the entire codebase would be necessary for a comprehensive assessment.

cr-gpt · 2023-07-17T08:09:53Z

tools/pika_exporter/grafana/prometheus.yml

@@ -3,5 +3,8 @@ scrape_configs:
  scrape_interval: 15s
  static_configs:
    - targets: ['127.0.0.1:9121']
+      labels:
+        group: 'test'
+    - targets: ['127.0.0.1:9122']
      labels:
        group: 'test'


The code patch you provided appears to be a configuration file for Prometheus scrape_configs. Overall, the changes you made seem fine, but there are a couple of improvements and bug risks that can be addressed:

Syntax error: There is a missing indentation before scrape_interval in line 3. Make sure it aligns correctly with scrape_configs.

End-of-file newline: As mentioned in the diff (\ No newline at end of file), it seems like the patch does not have a newline character at the end of the file. While this might not cause any immediate issues, adding the newline can help ensure consistency and avoid potential parser errors.

Label naming conventions: Although not necessarily required, it is common practice to use lowercase and underscore-separated labels instead of using single quotes around label values. For example, 'test' could be changed to test.

These suggestions should help improve the readability and maintainability of your configuration file.

Support network IO traffic monitoring. Including IO bytes and kps of Redis requests and master-slave replication. Fixes: OpenAtomFoundation#1732 Signed-off-by: yaoyinnan <[email protected]>

fix exporter makefile Fixes: OpenAtomFoundation#1732 Signed-off-by: yaoyinnan <[email protected]>

cr-gpt · 2023-07-17T11:42:25Z

tools/pika_exporter/Makefile

+	CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 go build -o bin/$(PROJNAME)
+else ifeq ($(ARCH), arm64)
+	CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 go build -o bin/$(PROJNAME)
+endif
 endif

 deps: generateVer


The code patch you provided appears to be checking the operating system and architecture to determine how to build the project. Here are some suggestions for improvement and bug risk assessment:

Proper indentation: The indentation in the code snippet you provided is inconsistent. It's good practice to use consistent indentation to improve readability. Make sure all lines within the same block have the same indentation level.

Commenting: Consider adding comments to explain the purpose of each section or condition. This will make it easier for others (including yourself) to understand the intentions behind the code.

Use elif instead of else if: In some parts of the code, you're using else if to chain conditions. It's generally recommended to use elif instead for better readability.

Error handling: There is no explicit error handling in the code snippet you provided. It may be beneficial to add appropriate error handling mechanisms to handle cases where the build process fails.

Apart from these points, it's difficult to identify any specific bugs or improvements without understanding the bigger context of your project and the rest of the codebase.

* feat: support network IO traffic monitoring Support network IO traffic monitoring. Including IO bytes and kps of Redis requests and master-slave replication. Fixes: OpenAtomFoundation#1732 Signed-off-by: yaoyinnan <[email protected]>

yaoyinnan requested review from luky116, chenbt-hz and chejinge July 15, 2023 03:55