Address code review comments

test/cpp/interop/metrics_client.cc

@@ -67,9 +67,13 @@ void PrintMetrics(grpc::string& server_address) {
   long overall_qps = 0;
   int idx = 0;
   while (reader->Read(&gauge_response)) {
-    gpr_log(GPR_INFO, "Gauge: %d (%s: %ld)", ++idx,
-            gauge_response.name().c_str(), gauge_response.value());
-    overall_qps += gauge_response.value();
+    if (gauge_response.value_case() == GaugeResponse::kLongValue) {
+      gpr_log(GPR_INFO, "Gauge: %d (%s: %ld)", ++idx,
+              gauge_response.name().c_str(), gauge_response.long_value());
+      overall_qps += gauge_response.long_value();
+    } else {
+      gpr_log(GPR_INFO, "Gauge %s is not a long value", gauge_response.name().c_str());
+    }
   }
 
   gpr_log(GPR_INFO, "OVERALL: %ld", overall_qps);
@@ -84,7 +88,7 @@ int main(int argc, char** argv) {
   grpc::testing::InitTest(&argc, &argv, true);
 
   // Make sure server_addresses flag is not empty
-  if (FLAGS_metrics_server_address.length() == 0) {
+  if (FLAGS_metrics_server_address.empty()) {
     gpr_log(
         GPR_ERROR,
         "Cannot connect to the Metrics server. Please pass the address of the"

test/cpp/interop/stress_interop_client.h

@@ -90,7 +90,7 @@ class StressTestInteropClient {
                           long test_duration_secs, long sleep_duration_ms,
                           long metrics_collection_interval_secs);
 
-  // The main funciton. Use this as the thread entry point.
+  // The main function. Use this as the thread entry point.
   // qps_gauge is the Gauge to record the requests per second metric
   void MainLoop(std::shared_ptr<Gauge> qps_gauge);
 

test/cpp/interop/stress_test.cc

@@ -41,6 +41,7 @@
 #include <grpc/support/time.h>
 #include <grpc++/create_channel.h>
 #include <grpc++/grpc++.h>
+#include <grpc++/impl/thd.h>
 
 #include "test/cpp/interop/interop_client.h"
 #include "test/cpp/interop/stress_interop_client.h"
@@ -91,11 +92,6 @@ DEFINE_string(test_cases, "",
               " 'large_unary', 10% of the time and 'empty_stream' the remaining"
               " 70% of the time");
 
-using std::make_pair;
-using std::pair;
-using std::thread;
-using std::vector;
-
 using grpc::testing::kTestCaseList;
 using grpc::testing::MetricsService;
 using grpc::testing::MetricsServiceImpl;
@@ -119,7 +115,7 @@ TestCaseType GetTestTypeFromName(const grpc::string& test_name) {
 
 // Converts a string of comma delimited tokens to a vector of tokens
 bool ParseCommaDelimitedString(const grpc::string& comma_delimited_str,
-                               vector<grpc::string>& tokens) {
+                               std::vector<grpc::string>& tokens) {
   size_t bpos = 0;
   size_t epos = grpc::string::npos;
 
@@ -137,10 +133,10 @@ bool ParseCommaDelimitedString(const grpc::string& comma_delimited_str,
 //   - Whether parsing was successful (return value)
 //   - Vector of (test_type_enum, weight) pairs returned via 'tests' parameter
 bool ParseTestCasesString(const grpc::string& test_cases,
-                          vector<pair<TestCaseType, int>>& tests) {
+                          std::vector<std::pair<TestCaseType, int>>& tests) {
   bool is_success = true;
 
-  vector<grpc::string> tokens;
+  std::vector<grpc::string> tokens;
   ParseCommaDelimitedString(test_cases, tokens);
 
   for (auto it = tokens.begin(); it != tokens.end(); it++) {
@@ -168,8 +164,8 @@ bool ParseTestCasesString(const grpc::string& test_cases,
 }
 
 // For debugging purposes
-void LogParameterInfo(const vector<grpc::string>& addresses,
-                      const vector<pair<TestCaseType, int>>& tests) {
+void LogParameterInfo(const std::vector<grpc::string>& addresses,
+                      const std::vector<std::pair<TestCaseType, int>>& tests) {
   gpr_log(GPR_INFO, "server_addresses: %s", FLAGS_server_addresses.c_str());
   gpr_log(GPR_INFO, "test_cases : %s", FLAGS_test_cases.c_str());
   gpr_log(GPR_INFO, "sleep_duration_ms: %d", FLAGS_sleep_duration_ms);
@@ -195,7 +191,7 @@ int main(int argc, char** argv) {
   srand(time(NULL));
 
   // Parse the server addresses
-  vector<grpc::string> server_addresses;
+  std::vector<grpc::string> server_addresses;
   ParseCommaDelimitedString(FLAGS_server_addresses, server_addresses);
 
   // Parse test cases and weights
@@ -204,7 +200,7 @@ int main(int argc, char** argv) {
     return 1;
   }
 
-  vector<pair<TestCaseType, int>> tests;
+  std::vector<std::pair<TestCaseType, int>> tests;
   if (!ParseTestCasesString(FLAGS_test_cases, tests)) {
     gpr_log(GPR_ERROR, "Error in parsing test cases string %s ",
             FLAGS_test_cases.c_str());
@@ -218,7 +214,7 @@ int main(int argc, char** argv) {
 
   gpr_log(GPR_INFO, "Starting test(s)..");
 
-  vector<thread> test_threads;
+  std::vector<grpc::thread> test_threads;
 
   int thread_idx = 0;
   for (auto it = server_addresses.begin(); it != server_addresses.end(); it++) {
@@ -239,8 +235,8 @@ int main(int argc, char** argv) {
       grpc::string metricName =
           "/stress_test/qps/thread/" + std::to_string(thread_idx);
       test_threads.emplace_back(
-          thread(&StressTestInteropClient::MainLoop, client,
-                 metrics_service.CreateGauge(metricName, is_already_created)));
+          grpc::thread(&StressTestInteropClient::MainLoop, client,
+                 metrics_service.CreateGauge(metricName, &is_already_created)));
 
       // The Gauge should not have been already created
       GPR_ASSERT(!is_already_created);

test/cpp/util/metrics_server.cc

@@ -61,8 +61,8 @@ grpc::Status MetricsServiceImpl::GetAllGauges(
   std::lock_guard<std::mutex> lock(mu_);
   for (auto it = gauges_.begin(); it != gauges_.end(); it++) {
     GaugeResponse resp;
-    resp.set_name(it->first);           // Gauge name
-    resp.set_value(it->second->Get());  // Gauge value
+    resp.set_name(it->first);                // Gauge name
+    resp.set_long_value(it->second->Get());  // Gauge value
     writer->Write(resp);
   }
 
@@ -77,14 +77,14 @@ grpc::Status MetricsServiceImpl::GetGauge(ServerContext* context,
   auto it = gauges_.find(request->name());
   if (it != gauges_.end()) {
     response->set_name(it->first);
-    response->set_value(it->second->Get());
+    response->set_long_value(it->second->Get());
   }
 
   return Status::OK;
 }
 
-std::shared_ptr<Gauge> MetricsServiceImpl::CreateGauge(string name,
-                                                       bool& already_present) {
+std::shared_ptr<Gauge> MetricsServiceImpl::CreateGauge(const grpc::string& name,
+                                                       bool* already_present) {
   std::lock_guard<std::mutex> lock(mu_);
 
   std::shared_ptr<Gauge> gauge(new Gauge(0));
@@ -92,7 +92,7 @@ std::shared_ptr<Gauge> MetricsServiceImpl::CreateGauge(string name,
 
   // p.first is an iterator pointing to <name, shared_ptr<Gauge>> pair. p.second
   // is a boolean indicating if the Gauge is already present in the map
-  already_present = !p.second;
+  *already_present = !p.second;
   return p.first->second;
 }
 

test/cpp/util/metrics_server.h

@@ -36,7 +36,6 @@
 #include <atomic>
 #include <map>
 #include <mutex>
-#include <vector>
 
 #include "test/proto/metrics.grpc.pb.h"
 #include "test/proto/metrics.pb.h"
@@ -61,9 +60,8 @@
 namespace grpc {
 namespace testing {
 
-using std::map;
-using std::vector;
-
+// TODO(sreek): Add support for other types of Gauges like Double, String in
+// future
 class Gauge {
  public:
   Gauge(long initial_val);
@@ -86,7 +84,8 @@ class MetricsServiceImpl GRPC_FINAL : public MetricsService::Service {
   // is already present in the map.
   // NOTE: CreateGauge can be called anytime (i.e before or after calling
   // StartServer).
-  std::shared_ptr<Gauge> CreateGauge(string name, bool& is_present);
+  std::shared_ptr<Gauge> CreateGauge(const grpc::string& name,
+                                     bool* already_present);
 
   std::unique_ptr<grpc::Server> StartServer(int port);
 

test/proto/metrics.proto

@@ -36,7 +36,11 @@ package grpc.testing;
 
 message GaugeResponse {
   string name = 1;
-  int64 value = 2;
+  oneof value {
+    int64 long_value = 2;
+    double double_vale = 3;
+    string string_value = 4;
+  }
 }
 
 message GaugeRequest {