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 <ale...@apache.org> 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 <achenn...@cloudera.com> --- 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"); }