Merge pull request #6158 from jtattermusch/performance_testing_polishing

Improvement to benchmarking

7 files changed, 78 insertions, 134 deletions

diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc
index c87ad6461d..9c0649cae6 100644
--- a/test/cpp/qps/driver.cc
+++ b/test/cpp/qps/driver.cc
@@ -343,8 +343,8 @@ std::unique_ptr<ScenarioResult> RunScenario(
 
   // Finish a run
   std::unique_ptr<ScenarioResult> result(new ScenarioResult);
-  result->client_config = result_client_config;
-  result->server_config = result_server_config;
+  Histogram merged_latencies;
+
   gpr_log(GPR_INFO, "Finishing clients");
   for (auto client = &clients[0]; client != &clients[num_clients]; client++) {
     GPR_ASSERT(client->stream->Write(client_mark));
@@ -353,9 +353,8 @@ std::unique_ptr<ScenarioResult> RunScenario(
   for (auto client = &clients[0]; client != &clients[num_clients]; client++) {
     GPR_ASSERT(client->stream->Read(&client_status));
     const auto& stats = client_status.stats();
-    result->latencies.MergeProto(stats.latencies());
-    result->client_resources.emplace_back(
-        stats.time_elapsed(), stats.time_user(), stats.time_system(), -1);
+    merged_latencies.MergeProto(stats.latencies());
+    result->add_client_stats()->CopyFrom(stats);
     GPR_ASSERT(!client->stream->Read(&client_status));
   }
   for (auto client = &clients[0]; client != &clients[num_clients]; client++) {
@@ -363,6 +362,8 @@ std::unique_ptr<ScenarioResult> RunScenario(
   }
   delete[] clients;
 
+  merged_latencies.FillProto(result->mutable_latencies());
+
   gpr_log(GPR_INFO, "Finishing servers");
   for (auto server = &servers[0]; server != &servers[num_servers]; server++) {
     GPR_ASSERT(server->stream->Write(server_mark));
@@ -370,10 +371,8 @@ std::unique_ptr<ScenarioResult> RunScenario(
   }
   for (auto server = &servers[0]; server != &servers[num_servers]; server++) {
     GPR_ASSERT(server->stream->Read(&server_status));
-    const auto& stats = server_status.stats();
-    result->server_resources.emplace_back(
-        stats.time_elapsed(), stats.time_user(), stats.time_system(),
-        server_status.cores());
+    result->add_server_stats()->CopyFrom(server_status.stats());
+    result->add_server_cores(server_status.cores());
     GPR_ASSERT(!server->stream->Read(&server_status));
   }
   for (auto server = &servers[0]; server != &servers[num_servers]; server++) {
diff --git a/test/cpp/qps/driver.h b/test/cpp/qps/driver.h
index 21e51529d5..3a5cf138f1 100644
--- a/test/cpp/qps/driver.h
+++ b/test/cpp/qps/driver.h
@@ -41,29 +41,6 @@
 
 namespace grpc {
 namespace testing {
-class ResourceUsage {
- public:
-  ResourceUsage(double w, double u, double s, int c)
-      : wall_time_(w), user_time_(u), system_time_(s), cores_(c) {}
-  double wall_time() const { return wall_time_; }
-  double user_time() const { return user_time_; }
-  double system_time() const { return system_time_; }
-  int cores() const { return cores_; }
-
- private:
-  double wall_time_;
-  double user_time_;
-  double system_time_;
-  int cores_;
-};
-
-struct ScenarioResult {
-  Histogram latencies;
-  std::vector<ResourceUsage> client_resources;
-  std::vector<ResourceUsage> server_resources;
-  ClientConfig client_config;
-  ServerConfig server_config;
-};
 
 std::unique_ptr<ScenarioResult> RunScenario(
     const grpc::testing::ClientConfig& client_config, size_t num_clients,
diff --git a/test/cpp/qps/qps_driver.cc b/test/cpp/qps/qps_driver.cc
index e412c6919a..608181f77f 100644
--- a/test/cpp/qps/qps_driver.cc
+++ b/test/cpp/qps/qps_driver.cc
@@ -85,7 +85,6 @@ using grpc::testing::ServerConfig;
 using grpc::testing::ClientType;
 using grpc::testing::ServerType;
 using grpc::testing::RpcType;
-using grpc::testing::ResourceUsage;
 using grpc::testing::SecurityParams;
 
 namespace grpc {
diff --git a/test/cpp/qps/qps_json_driver.cc b/test/cpp/qps/qps_json_driver.cc
index 8943a43ba8..e9266a5711 100644
--- a/test/cpp/qps/qps_json_driver.cc
+++ b/test/cpp/qps/qps_json_driver.cc
@@ -102,12 +102,16 @@ static void QpsDriver() {
   for (int i = 0; i < scenarios.scenarios_size(); i++) {
     const Scenario &scenario = scenarios.scenarios(i);
     std::cerr << "RUNNING SCENARIO: " << scenario.name() << "\n";
-    const auto result =
+    auto result =
         RunScenario(scenario.client_config(), scenario.num_clients(),
                     scenario.server_config(), scenario.num_servers(),
                     scenario.warmup_seconds(), scenario.benchmark_seconds(),
                     scenario.spawn_local_worker_count());
 
+    // Amend the result with scenario config. Eventually we should adjust
+    // RunScenario contract so we don't need to touch the result here.
+    result->mutable_scenario()->CopyFrom(scenario);
+
     GetReporter()->ReportQPS(*result);
     GetReporter()->ReportQPSPerCore(*result);
     GetReporter()->ReportLatency(*result);
diff --git a/test/cpp/qps/report.cc b/test/cpp/qps/report.cc
index b230eb441e..07ab0a8f28 100644
--- a/test/cpp/qps/report.cc
+++ b/test/cpp/qps/report.cc
@@ -33,6 +33,11 @@
 
 #include "test/cpp/qps/report.h"
 
+#include <fstream>
+
+#include <google/protobuf/util/json_util.h>
+#include <google/protobuf/util/type_resolver_util.h>
+
 #include <grpc/support/log.h>
 #include "test/cpp/qps/driver.h"
 #include "test/cpp/qps/stats.h"
@@ -40,10 +45,13 @@
 namespace grpc {
 namespace testing {
 
-static double WallTime(ResourceUsage u) { return u.wall_time(); }
-static double UserTime(ResourceUsage u) { return u.user_time(); }
-static double SystemTime(ResourceUsage u) { return u.system_time(); }
-static int Cores(ResourceUsage u) { return u.cores(); }
+static double WallTime(ClientStats s) { return s.time_elapsed(); }
+static double SystemTime(ClientStats s) { return s.time_system(); }
+static double UserTime(ClientStats s) { return s.time_user(); }
+static double ServerWallTime(ServerStats s) { return s.time_elapsed(); }
+static double ServerSystemTime(ServerStats s) { return s.time_system(); }
+static double ServerUserTime(ServerStats s) { return s.time_user(); }
+static int Cores(int n) { return n; }
 
 void CompositeReporter::add(std::unique_ptr<Reporter> reporter) {
   reporters_.emplace_back(std::move(reporter));
@@ -74,102 +82,74 @@ void CompositeReporter::ReportTimes(const ScenarioResult& result) {
 }
 
 void GprLogReporter::ReportQPS(const ScenarioResult& result) {
-  gpr_log(
-      GPR_INFO, "QPS: %.1f",
-      result.latencies.Count() / average(result.client_resources, WallTime));
+  Histogram histogram;
+  histogram.MergeProto(result.latencies());
+  gpr_log(GPR_INFO, "QPS: %.1f",
+          histogram.Count() / average(result.client_stats(), WallTime));
 }
 
 void GprLogReporter::ReportQPSPerCore(const ScenarioResult& result) {
-  auto qps =
-      result.latencies.Count() / average(result.client_resources, WallTime);
+  Histogram histogram;
+  histogram.MergeProto(result.latencies());
+  auto qps = histogram.Count() / average(result.client_stats(), WallTime);
 
   gpr_log(GPR_INFO, "QPS: %.1f (%.1f/server core)", qps,
-          qps / sum(result.server_resources, Cores));
+          qps / sum(result.server_cores(), Cores));
 }
 
 void GprLogReporter::ReportLatency(const ScenarioResult& result) {
+  Histogram histogram;
+  histogram.MergeProto(result.latencies());
   gpr_log(GPR_INFO,
           "Latencies (50/90/95/99/99.9%%-ile): %.1f/%.1f/%.1f/%.1f/%.1f us",
-          result.latencies.Percentile(50) / 1000,
-          result.latencies.Percentile(90) / 1000,
-          result.latencies.Percentile(95) / 1000,
-          result.latencies.Percentile(99) / 1000,
-          result.latencies.Percentile(99.9) / 1000);
+          histogram.Percentile(50) / 1000, histogram.Percentile(90) / 1000,
+          histogram.Percentile(95) / 1000, histogram.Percentile(99) / 1000,
+          histogram.Percentile(99.9) / 1000);
 }
 
 void GprLogReporter::ReportTimes(const ScenarioResult& result) {
   gpr_log(GPR_INFO, "Server system time: %.2f%%",
-          100.0 * sum(result.server_resources, SystemTime) /
-              sum(result.server_resources, WallTime));
+          100.0 * sum(result.server_stats(), ServerSystemTime) /
+              sum(result.server_stats(), ServerWallTime));
   gpr_log(GPR_INFO, "Server user time:   %.2f%%",
-          100.0 * sum(result.server_resources, UserTime) /
-              sum(result.server_resources, WallTime));
+          100.0 * sum(result.server_stats(), ServerUserTime) /
+              sum(result.server_stats(), ServerWallTime));
   gpr_log(GPR_INFO, "Client system time: %.2f%%",
-          100.0 * sum(result.client_resources, SystemTime) /
-              sum(result.client_resources, WallTime));
+          100.0 * sum(result.client_stats(), SystemTime) /
+              sum(result.client_stats(), WallTime));
   gpr_log(GPR_INFO, "Client user time:   %.2f%%",
-          100.0 * sum(result.client_resources, UserTime) /
-              sum(result.client_resources, WallTime));
+          100.0 * sum(result.client_stats(), UserTime) /
+              sum(result.client_stats(), WallTime));
 }
 
-void PerfDbReporter::ReportQPS(const ScenarioResult& result) {
-  auto qps =
-      result.latencies.Count() / average(result.client_resources, WallTime);
-
-  perf_db_client_.setQps(qps);
-  perf_db_client_.setConfigs(result.client_config, result.server_config);
+void JsonReporter::ReportQPS(const ScenarioResult& result) {
+  std::unique_ptr<google::protobuf::util::TypeResolver> type_resolver(
+      google::protobuf::util::NewTypeResolverForDescriptorPool(
+          "type.googleapis.com",
+          google::protobuf::DescriptorPool::generated_pool()));
+  grpc::string binary;
+  grpc::string json_string;
+  result.SerializeToString(&binary);
+  auto status = BinaryToJsonString(
+      type_resolver.get(), "type.googleapis.com/grpc.testing.ScenarioResult",
+      binary, &json_string);
+  GPR_ASSERT(status.ok());
+
+  std::ofstream output_file(report_file_);
+  output_file << json_string;
+  output_file.close();
 }
 
-void PerfDbReporter::ReportQPSPerCore(const ScenarioResult& result) {
-  auto qps =
-      result.latencies.Count() / average(result.client_resources, WallTime);
-
-  auto qps_per_core = qps / sum(result.server_resources, Cores);
-
-  perf_db_client_.setQps(qps);
-  perf_db_client_.setQpsPerCore(qps_per_core);
-  perf_db_client_.setConfigs(result.client_config, result.server_config);
+void JsonReporter::ReportQPSPerCore(const ScenarioResult& result) {
+  // NOP - all reporting is handled by ReportQPS.
 }
 
-void PerfDbReporter::ReportLatency(const ScenarioResult& result) {
-  perf_db_client_.setLatencies(result.latencies.Percentile(50) / 1000,
-                               result.latencies.Percentile(90) / 1000,
-                               result.latencies.Percentile(95) / 1000,
-                               result.latencies.Percentile(99) / 1000,
-                               result.latencies.Percentile(99.9) / 1000);
-  perf_db_client_.setConfigs(result.client_config, result.server_config);
+void JsonReporter::ReportLatency(const ScenarioResult& result) {
+  // NOP - all reporting is handled by ReportQPS.
 }
 
-void PerfDbReporter::ReportTimes(const ScenarioResult& result) {
-  const double server_system_time = 100.0 *
-                                    sum(result.server_resources, SystemTime) /
-                                    sum(result.server_resources, WallTime);
-  const double server_user_time = 100.0 *
-                                  sum(result.server_resources, UserTime) /
-                                  sum(result.server_resources, WallTime);
-  const double client_system_time = 100.0 *
-                                    sum(result.client_resources, SystemTime) /
-                                    sum(result.client_resources, WallTime);
-  const double client_user_time = 100.0 *
-                                  sum(result.client_resources, UserTime) /
-                                  sum(result.client_resources, WallTime);
-
-  perf_db_client_.setTimes(server_system_time, server_user_time,
-                           client_system_time, client_user_time);
-  perf_db_client_.setConfigs(result.client_config, result.server_config);
-}
-
-void PerfDbReporter::SendData() {
-  // send data to performance database
-  bool data_state =
-      perf_db_client_.sendData(hashed_id_, test_name_, sys_info_, tag_);
-
-  // check state of data sending
-  if (data_state) {
-    gpr_log(GPR_INFO, "Data sent to performance database successfully");
-  } else {
-    gpr_log(GPR_INFO, "Data could not be sent to performance database");
-  }
+void JsonReporter::ReportTimes(const ScenarioResult& result) {
+  // NOP - all reporting is handled by ReportQPS.
 }
 
 }  // namespace testing
diff --git a/test/cpp/qps/report.h b/test/cpp/qps/report.h
index 5caf3fe69a..8f04d84124 100644
--- a/test/cpp/qps/report.h
+++ b/test/cpp/qps/report.h
@@ -104,33 +104,19 @@ class GprLogReporter : public Reporter {
   void ReportTimes(const ScenarioResult& result) GRPC_OVERRIDE;
 };
 
-/** Reporter for performance database tool */
-class PerfDbReporter : public Reporter {
+/** Dumps the report to a JSON file. */
+class JsonReporter : public Reporter {
  public:
-  PerfDbReporter(const string& name, const string& hashed_id,
-                 const string& test_name, const string& sys_info,
-                 const string& server_address, const string& tag)
-      : Reporter(name),
-        hashed_id_(hashed_id),
-        test_name_(test_name),
-        sys_info_(sys_info),
-        tag_(tag) {
-    perf_db_client_.init(grpc::CreateChannel(
-        server_address, grpc::InsecureChannelCredentials()));
-  }
-  ~PerfDbReporter() GRPC_OVERRIDE { SendData(); };
+  JsonReporter(const string& name, const string& report_file)
+      : Reporter(name), report_file_(report_file) {}
 
  private:
-  PerfDbClient perf_db_client_;
-  std::string hashed_id_;
-  std::string test_name_;
-  std::string sys_info_;
-  std::string tag_;
   void ReportQPS(const ScenarioResult& result) GRPC_OVERRIDE;
   void ReportQPSPerCore(const ScenarioResult& result) GRPC_OVERRIDE;
   void ReportLatency(const ScenarioResult& result) GRPC_OVERRIDE;
   void ReportTimes(const ScenarioResult& result) GRPC_OVERRIDE;
-  void SendData();
+
+  const string report_file_;
 };
 
 }  // namespace testing
diff --git a/test/cpp/util/benchmark_config.cc b/test/cpp/util/benchmark_config.cc
index 746d3d7ae6..6fc864069e 100644
--- a/test/cpp/util/benchmark_config.cc
+++ b/test/cpp/util/benchmark_config.cc
@@ -37,8 +37,8 @@
 DEFINE_bool(enable_log_reporter, true,
             "Enable reporting of benchmark results through GprLog");
 
-DEFINE_bool(report_metrics_db, false,
-            "True if metrics to be reported to performance database");
+DEFINE_string(scenario_result_file, "",
+              "Write JSON benchmark report to the file specified.");
 
 DEFINE_string(hashed_id, "", "Hash of the user id");
 
@@ -71,10 +71,9 @@ static std::shared_ptr<Reporter> InitBenchmarkReporters() {
     composite_reporter->add(
         std::unique_ptr<Reporter>(new GprLogReporter("LogReporter")));
   }
-  if (FLAGS_report_metrics_db) {
+  if (FLAGS_scenario_result_file != "") {
     composite_reporter->add(std::unique_ptr<Reporter>(
-        new PerfDbReporter("PerfDbReporter", FLAGS_hashed_id, FLAGS_test_name,
-                           FLAGS_sys_info, FLAGS_server_address, FLAGS_tag)));
+        new JsonReporter("JsonReporter", FLAGS_scenario_result_file)));
   }
 
   return std::shared_ptr<Reporter>(composite_reporter);