This is an automated email from the ASF dual-hosted git repository.
achennaka pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 7f1bfb592 [tools] disambiguate parse_metrics' output
7f1bfb592 is described below
commit 7f1bfb59210d8298aacbaa6ce2937adb359d802e
Author: Alexey Serbin <[email protected]>
AuthorDate: Wed Nov 29 12:19:26 2023 -0800
[tools] disambiguate parse_metrics' output
This patch improves the output of the 'kudu diagnostic parse_metrics'
CLI tool. Prior to this patch, it was impossible to tell between
absent metric values at a particular timestamp and zeros. With this
patch, an absent value and undefined rate of change of a metric for
a particular timestamp are indicated with the '?' ASCII character.
Change-Id: Iba0bc90a6313bd9096fa1493dd902254cdda1dc1
Reviewed-on: http://gerrit.cloudera.org:8080/20741
Tested-by: Kudu Jenkins
Reviewed-by: Abhishek Chennaka <[email protected]>
---
src/kudu/tools/diagnostics_log_parser.cc | 88 ++++++++++++++++++++++----------
src/kudu/tools/diagnostics_log_parser.h | 10 ++--
src/kudu/tools/kudu-tool-test.cc | 26 +++++-----
3 files changed, 79 insertions(+), 45 deletions(-)
diff --git a/src/kudu/tools/diagnostics_log_parser.cc
b/src/kudu/tools/diagnostics_log_parser.cc
index 190f19b15..bfeab35e8 100644
--- a/src/kudu/tools/diagnostics_log_parser.cc
+++ b/src/kudu/tools/diagnostics_log_parser.cc
@@ -36,6 +36,7 @@
#include "kudu/gutil/map-util.h"
#include "kudu/gutil/port.h"
+#include "kudu/gutil/stringprintf.h"
#include "kudu/gutil/strings/join.h"
#include "kudu/gutil/strings/numbers.h"
#include "kudu/gutil/strings/split.h"
@@ -49,6 +50,8 @@ using std::array;
using std::cout;
using std::endl;
using std::ifstream;
+using std::nullopt;
+using std::optional;
using std::string;
using std::vector;
using strings::Substitute;
@@ -325,28 +328,51 @@ Status
MetricCollectingLogVisitor::VisitMetricsRecord(const MetricsRecord& mr) {
// Iterate through the user-requested metrics and display what we need to, as
// determined by 'opts_'.
cout << mr.timestamp;
- for (const auto& full_and_display : opts_.simple_metric_names) {
- const auto& full_name = full_and_display.first;
- int64_t sum = SumPlainWithMetricRecord(mr, full_name);
- cout << Substitute("\t$0", sum);
+ for (const auto& [full_name, _]: opts_.simple_metric_names) {
+ const auto sum = SumPlainWithMetricRecord(mr, full_name);
+ if (sum) {
+ cout << Substitute("\t$0", *sum);
+ } else {
+ cout << "\t?";
+ }
}
- const double time_delta_secs =
- static_cast<double>(mr.timestamp - last_visited_timestamp_) / 1e6;
- for (const auto& full_and_display : opts_.rate_metric_names) {
- // If this is the first record we're visiting, we can't compute a rate.
- const auto& full_name = full_and_display.first;
- int64_t sum = SumPlainWithMetricRecord(mr, full_name);
- if (last_visited_timestamp_ == 0) {
- InsertOrDie(&rate_metric_prev_sum_, full_name, sum);
- cout << "\t0";
- continue;
+ const optional<double> time_delta_secs = last_visited_timestamp_
+ ? optional(static_cast<double>(mr.timestamp - *last_visited_timestamp_)
/ 1e6)
+ : nullopt;
+ for (const auto& [full_name, _] : opts_.rate_metric_names) {
+ const auto sum = SumPlainWithMetricRecord(mr, full_name);
+ optional<double> rate = nullopt;
+ if (time_delta_secs) {
+ // It's possible to compute the rate of change for a metric only if
+ // the time delta is defined, i.e. if it's not the very first entry
+ // being visited.
+ if (const auto* prev_sum = FindOrNull(rate_metric_prev_sum_, full_name))
{
+ if (sum) {
+ // Both the previous and the current metric's snapshots are
available.
+ rate = static_cast<double>(*sum - *prev_sum) / *time_delta_secs;
+ } else {
+ // The value hasn't changed from previous snapshot, so the rate
+ // of change is zero for the reported time interval.
+ rate = 0.0;
+ }
+ }
+ }
+
+ // Update the sum with the value computed at the current timestamp.
+ if (sum) {
+ InsertOrUpdate(&rate_metric_prev_sum_, full_name, *sum);
+ }
+
+ if (rate) {
+ // Print out the rate of change in 'units per second', as computed.
+ cout << StringPrintf("\t%12.3f", *rate);
+ } else {
+ // Indicate that the rate of change for the metric is undefined
+ // since the necessary information to compute the rate isn't present.
+ cout << StringPrintf("\t%12s", "?");
}
- int64_t prev_sum = FindOrDie(rate_metric_prev_sum_, full_name);
- cout << Substitute("\t$0", static_cast<double>(sum - prev_sum) /
time_delta_secs);
- InsertOrUpdate(&rate_metric_prev_sum_, full_name, sum);
}
- for (const auto& full_and_display : opts_.hist_metric_names) {
- const auto& full_name = full_and_display.first;
+ for (const auto& [full_name, _]: opts_.hist_metric_names) {
const auto& mr_entities_to_vals = FindOrDie(mr.metric_to_entities,
full_name);
const auto& entities_to_vals = FindOrDie(metric_to_entities_, full_name);
std::map<int64_t, int> all_counts;
@@ -412,25 +438,31 @@ void
MetricCollectingLogVisitor::UpdateWithMetricsRecord(const MetricsRecord& mr
last_visited_timestamp_ = mr.timestamp;
}
-int64_t MetricCollectingLogVisitor::SumPlainWithMetricRecord(
+optional<int64_t> MetricCollectingLogVisitor::SumPlainWithMetricRecord(
const MetricsRecord& mr, const string& full_metric_name) const {
const auto& mr_entities_to_vals = FindOrDie(mr.metric_to_entities,
full_metric_name);
const auto& entities_to_vals = FindOrDie(metric_to_entities_,
full_metric_name);
int64_t sum = 0;
// Add all of the values for entities that didn't change, and are thus, not
// included in the record.
- for (const auto& e : entities_to_vals) {
- if (!ContainsKey(mr_entities_to_vals, e.first)) {
- CHECK(e.second.value_);
- sum += *e.second.value_;
+ bool has_val = false;
+ for (const auto& [id, val] : entities_to_vals) {
+ if (!ContainsKey(mr_entities_to_vals, id)) {
+ DCHECK(val.value_);
+ sum += *val.value_;
+ has_val = true;
}
}
// Now add the values for those that did.
- for (const auto& e : mr_entities_to_vals) {
- CHECK(e.second.value_);
- sum += *e.second.value_;
+ for (const auto& [_, val] : mr_entities_to_vals) {
+ DCHECK(val.value_);
+ sum += *val.value_;
+ has_val = true;
+ }
+ if (has_val) {
+ return sum;
}
- return sum;
+ return nullopt;
}
Status StackDumpingLogVisitor::ParseRecord(const ParsedLine& pl) {
diff --git a/src/kudu/tools/diagnostics_log_parser.h
b/src/kudu/tools/diagnostics_log_parser.h
index 620411bf2..aea06cbfa 100644
--- a/src/kudu/tools/diagnostics_log_parser.h
+++ b/src/kudu/tools/diagnostics_log_parser.h
@@ -212,9 +212,11 @@ class MetricCollectingLogVisitor : public LogVisitor {
// Calculate the sum of the plain metric (i.e. non-histogram) specified by
// 'full_metric_name', based on the existing values in our internal map and
- // including any new values for entities in 'mr'.
- int64_t SumPlainWithMetricRecord(const MetricsRecord& mr,
- const std::string& full_metric_name) const;
+ // including any new values for entities in 'mr'. Returns the sum wrapped
+ // into std::optional or std::nullopt if not a single value for the
+ // 'full_metric_name' is present in 'mr'.
+ std::optional<int64_t> SumPlainWithMetricRecord(
+ const MetricsRecord& mr, const std::string& full_metric_name) const;
// Maps the full metric name to the mapping between entity IDs and their
// metric value. As the visitor visits new metrics records, this gets updated
@@ -234,7 +236,7 @@ class MetricCollectingLogVisitor : public LogVisitor {
std::optional<JsonReader> json_;
//
// The timestamp of the last visited metrics record.
- int64_t last_visited_timestamp_ = 0;
+ std::optional<int64_t> last_visited_timestamp_;
const MetricsCollectingOpts opts_;
};
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index a0a5e1184..db21dec69 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -8796,11 +8796,11 @@ TEST_F(ToolTest, TestParseMetrics) {
&stdout));
// Spot check a few of the things that should be in the output.
ASSERT_STR_CONTAINS(stdout, "timestamp\tsize_bytes\tbytes_rate");
- ASSERT_STR_CONTAINS(stdout, "1521053715220449\t69705682\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053775220513\t69705682\t0");
- ASSERT_STR_CONTAINS(stdout,
"1521053835220611\t69712809\t118.78313932087244");
- ASSERT_STR_CONTAINS(stdout, "1521053895220710\t69712809\t0");
- ASSERT_STR_CONTAINS(stdout, "1661840031227204\t69712809\t0");
+ ASSERT_STR_CONTAINS(stdout, "1521053715220449\t69705682\t ?");
+ ASSERT_STR_CONTAINS(stdout, "1521053775220513\t69705682\t 0.000");
+ ASSERT_STR_CONTAINS(stdout, "1521053835220611\t69712809\t 118.783");
+ ASSERT_STR_CONTAINS(stdout, "1521053895220710\t69712809\t 0.000");
+ ASSERT_STR_CONTAINS(stdout, "1661840031227204\t69712809\t 0.000");
}
// Check out table metrics.
{
@@ -8812,10 +8812,10 @@ TEST_F(ToolTest, TestParseMetrics) {
&stdout));
// Spot check a few of the things that should be in the output.
ASSERT_STR_CONTAINS(stdout, "timestamp\trow_count\tsize");
- ASSERT_STR_CONTAINS(stdout, "1521053715220449\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053775220513\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053835220611\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053895220710\t0\t0");
+ ASSERT_STR_CONTAINS(stdout, "1521053715220449\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053775220513\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053835220611\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053895220710\t?\t?");
ASSERT_STR_CONTAINS(stdout, "1661840031227204\t228265\t78540528");
}
// Check out table metrics with default metric names.
@@ -8828,10 +8828,10 @@ TEST_F(ToolTest, TestParseMetrics) {
&stdout));
// Spot check a few of the things that should be in the output.
ASSERT_STR_CONTAINS(stdout, "timestamp\tlive_row_count\ton_disk_size");
- ASSERT_STR_CONTAINS(stdout, "1521053715220449\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053775220513\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053835220611\t0\t0");
- ASSERT_STR_CONTAINS(stdout, "1521053895220710\t0\t0");
+ ASSERT_STR_CONTAINS(stdout, "1521053715220449\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053775220513\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053835220611\t?\t?");
+ ASSERT_STR_CONTAINS(stdout, "1521053895220710\t?\t?");
ASSERT_STR_CONTAINS(stdout, "1661840031227204\t228265\t78540528");
}
