Tech debt: improve coverage & move some code (#99)

oschaaf · htuch · commit e15477b1b3a2 · 2019-07-16T00:26:25.000-04:00
cover switch cases in statIdtoFriendlyStatName()
Add CpuAffinityDetectionFailure test
Relocate determineCpuCoresWithAffinity to where it belongs

Signed-off-by: Otto van der Schaaf &lt;oschaaf@we-amp.com&gt;
diff --git a/ci/run_coverage.sh b/ci/run_coverage.sh
@@ -32,7 +32,7 @@ if [ "$VALIDATE_COVERAGE" == "true" ]
 then
     COVERAGE_VALUE=$(grep -Po '.*lines[.]*: \K(\d|\.)*' "${COVERAGE_SUMMARY}")
     # TODO(oschaaf): The target is 97.5%, so up this whenever possible in follow ups.
-    COVERAGE_THRESHOLD=96.9
+    COVERAGE_THRESHOLD=97.3
     COVERAGE_FAILED=$(echo "${COVERAGE_VALUE}<${COVERAGE_THRESHOLD}" | bc)
     
     echo "HTML coverage report is in ${COVERAGE_DIR}/coverage.html"
diff --git a/include/nighthawk/common/platform_util.h b/include/nighthawk/common/platform_util.h
@@ -22,6 +22,11 @@ class PlatformUtil {
    * @param duration duration that the calling thread should sleep.
    */
   virtual void sleep(std::chrono::microseconds duration) const PURE;
+
+  /**
+   * @return uint32_t 0 #CPUs that the current thread has affinity with. 0 on failure.
+   */
+  virtual uint32_t determineCpuCoresWithAffinity() const PURE;
 };
 
 using PlatformUtilPtr = std::unique_ptr<PlatformUtil>;
diff --git a/source/client/client.cc b/source/client/client.cc
@@ -48,7 +48,8 @@ bool Main::run() {
           nighthawk::client::Verbosity::VerbosityOptions_Name(options_->verbosity())),
       "[%T.%f][%t][%L] %v", log_lock);
   Envoy::Event::RealTimeSystem time_system; // NO_CHECK_FORMAT(real_time)
-  ProcessImpl process(*options_, time_system);
+  PlatformUtilImpl platform_util;
+  ProcessImpl process(*options_, time_system, platform_util);
   OutputCollectorFactoryImpl output_format_factory(time_system, *options_);
   auto collector = output_format_factory.create();
   if (process.run(*collector)) {
diff --git a/source/client/output_collector_impl.cc b/source/client/output_collector_impl.cc
@@ -78,7 +78,7 @@ ConsoleOutputCollectorImpl::formatProtoDuration(const Envoy::Protobuf::Duration&
                      (c % 1'000'000) / 1'000, c % 1'000);
 }
 
-std::string ConsoleOutputCollectorImpl::statIdtoFriendlyStatName(absl::string_view stat_id) const {
+std::string ConsoleOutputCollectorImpl::statIdtoFriendlyStatName(absl::string_view stat_id) {
   if (stat_id == "benchmark_http_client.queue_to_connect") {
     return "Queueing and connection setup latency";
   } else if (stat_id == "benchmark_http_client.request_to_response") {
diff --git a/source/client/output_collector_impl.h b/source/client/output_collector_impl.h
@@ -32,9 +32,9 @@ class ConsoleOutputCollectorImpl : public OutputCollectorImpl {
 public:
   ConsoleOutputCollectorImpl(Envoy::TimeSource& time_source, const Options& options);
   std::string toString() const override;
+  static std::string statIdtoFriendlyStatName(absl::string_view stat_id);
 
 private:
-  std::string statIdtoFriendlyStatName(absl::string_view stat_id) const;
   std::string formatProtoDuration(const Envoy::Protobuf::Duration& duration) const;
 };
 
diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc
@@ -39,11 +39,13 @@ using namespace std::chrono_literals;
 namespace Nighthawk {
 namespace Client {
 
-ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system)
+ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system,
+                         const PlatformUtil& platform_util)
     : time_system_(time_system), store_factory_(options), store_(store_factory_.create()),
       api_(thread_factory_, *store_, time_system_, file_system_),
       dispatcher_(api_.allocateDispatcher()), cleanup_([this] { tls_.shutdownGlobalThreading(); }),
-      benchmark_client_factory_(options), sequencer_factory_(options), options_(options) {
+      benchmark_client_factory_(options), sequencer_factory_(options), options_(options),
+      platform_util_(platform_util) {
   configureComponentLogLevels(spdlog::level::from_str(
       nighthawk::client::Verbosity::VerbosityOptions_Name(options_.verbosity())));
   tls_.registerThread(*dispatcher_, true);
@@ -89,7 +91,7 @@ void ProcessImpl::configureComponentLogLevels(spdlog::level::level_enum level) {
 }
 
 uint32_t ProcessImpl::determineConcurrency() const {
-  uint32_t cpu_cores_with_affinity = PlatformUtils::determineCpuCoresWithAffinity();
+  uint32_t cpu_cores_with_affinity = platform_util_.determineCpuCoresWithAffinity();
   if (cpu_cores_with_affinity == 0) {
     ENVOY_LOG(warn, "Failed to determine the number of cpus with affinity to our thread.");
     cpu_cores_with_affinity = std::thread::hardware_concurrency();
diff --git a/source/client/process_impl.h b/source/client/process_impl.h
@@ -36,7 +36,8 @@ namespace Client {
  */
 class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger::Id::main> {
 public:
-  ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system);
+  ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system,
+              const PlatformUtil& platform_util);
 
   uint32_t determineConcurrency() const;
   bool run(OutputCollector& collector) override;
@@ -67,6 +68,7 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
   const BenchmarkClientFactoryImpl benchmark_client_factory_;
   const SequencerFactoryImpl sequencer_factory_;
   const Options& options_;
+  const PlatformUtil& platform_util_;
 };
 
 } // namespace Client
diff --git a/source/client/service_impl.cc b/source/client/service_impl.cc
@@ -24,7 +24,8 @@ void ServiceImpl::handleExecutionRequest(const nighthawk::client::ExecutionReque
   // We scope here because the ProcessImpl instance must be destructed before we write the response
   // and set running to false.
   {
-    ProcessImpl process(*options, time_system_);
+    PlatformUtilImpl platform_util;
+    ProcessImpl process(*options, time_system_, platform_util);
     OutputCollectorFactoryImpl output_format_factory(time_system_, *options);
     auto logging_context = std::make_unique<Envoy::Logger::Context>(
         spdlog::level::from_str(
diff --git a/source/common/platform_util_impl.h b/source/common/platform_util_impl.h
@@ -16,6 +16,20 @@ class PlatformUtilImpl : public PlatformUtil {
   void sleep(std::chrono::microseconds duration) const override {
     std::this_thread::sleep_for(duration); // NO_CHECK_FORMAT(real_time)
   };
+
+  uint32_t determineCpuCoresWithAffinity() const override {
+    // TODO(oschaaf): mull over what to do w/regard to hyperthreading.
+    const pthread_t thread = pthread_self();
+    cpu_set_t cpuset;
+    int i;
+
+    CPU_ZERO(&cpuset);
+    i = pthread_getaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
+    if (i == 0) {
+      return CPU_COUNT(&cpuset);
+    }
+    return 0;
+  }
 };
 
 } // namespace Nighthawk
diff --git a/source/common/utility.cc b/source/common/utility.cc
@@ -11,26 +11,6 @@
 
 namespace Nighthawk {
 
-namespace PlatformUtils {
-
-// returns 0 on failure. returns the number of HW CPU's
-// that the current thread has affinity with.
-// TODO(oschaaf): mull over what to do w/regard to hyperthreading.
-uint32_t determineCpuCoresWithAffinity() {
-  const pthread_t thread = pthread_self();
-  cpu_set_t cpuset;
-  int i;
-
-  CPU_ZERO(&cpuset);
-  i = pthread_getaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
-  if (i == 0) {
-    return CPU_COUNT(&cpuset);
-  }
-  return 0;
-}
-
-} // namespace PlatformUtils
-
 std::map<std::string, uint64_t>
 Utility::mapCountersFromStore(const Envoy::Stats::Store& store,
                               const StoreCounterFilter& filter) const {
diff --git a/source/common/utility.h b/source/common/utility.h
@@ -16,10 +16,6 @@
 
 namespace Nighthawk {
 
-namespace PlatformUtils {
-uint32_t determineCpuCoresWithAffinity();
-}
-
 using StoreCounterFilter = std::function<bool(absl::string_view, const uint64_t)>;
 
 class Utility {
diff --git a/test/mocks.h b/test/mocks.h
@@ -37,6 +37,7 @@ class MockPlatformUtil : public PlatformUtil {
 
   MOCK_CONST_METHOD0(yieldCurrentThread, void());
   MOCK_CONST_METHOD1(sleep, void(std::chrono::microseconds));
+  MOCK_CONST_METHOD0(determineCpuCoresWithAffinity, uint32_t());
 };
 
 class MockRateLimiter : public RateLimiter {
diff --git a/test/output_collector_test.cc b/test/output_collector_test.cc
@@ -70,5 +70,18 @@ TEST_F(OutputCollectorTest, YamlFormatter) {
   expectEqualToGoldFile(collector, "test/test_data/output_collector.yaml.gold");
 }
 
+class StatidToNameTest : public Test {};
+
+TEST_F(StatidToNameTest, TestTranslations) {
+  // Well known id's shouldn't be returned as-is, but unknown ones should.
+  EXPECT_EQ(ConsoleOutputCollectorImpl::statIdtoFriendlyStatName("foo"), "foo");
+  const std::vector<std::string> ids = {"benchmark_http_client.queue_to_connect",
+                                        "benchmark_http_client.request_to_response",
+                                        "sequencer.callback", "sequencer.blocking"};
+  for (const std::string& id : ids) {
+    EXPECT_NE(ConsoleOutputCollectorImpl::statIdtoFriendlyStatName(id), id);
+  }
+}
+
 } // namespace Client
 } // namespace Nighthawk
diff --git a/test/platform_util_test.cc b/test/platform_util_test.cc
@@ -11,6 +11,7 @@ namespace Nighthawk {
 
 class PlatformUtilTest : public Test {
 public:
+  int32_t getCpuCountFromSet(cpu_set_t& set) { return CPU_COUNT(&set); }
   PlatformUtilImpl platform_util_;
 };
 
@@ -22,4 +23,26 @@ TEST_F(PlatformUtilTest, NoFatalFailureForSleep) {
   EXPECT_NO_FATAL_FAILURE(platform_util_.sleep(1us));
 }
 
+// TODO(oschaaf): we probably want to move this out to another file.
+TEST_F(PlatformUtilTest, CpusWithAffinity) {
+  cpu_set_t original_set;
+  CPU_ZERO(&original_set);
+  EXPECT_EQ(0, sched_getaffinity(0, sizeof(original_set), &original_set));
+
+  uint32_t original_cpu_count = platform_util_.determineCpuCoresWithAffinity();
+  EXPECT_EQ(original_cpu_count, getCpuCountFromSet(original_set));
+
+  // Now the test, we set affinity to just the first cpu. We expect that to be reflected.
+  // This will be a no-op on a single core system.
+  cpu_set_t test_set;
+  CPU_ZERO(&test_set);
+  CPU_SET(0, &test_set);
+  EXPECT_EQ(0, sched_setaffinity(0, sizeof(test_set), &test_set));
+  EXPECT_EQ(1, platform_util_.determineCpuCoresWithAffinity());
+
+  // Restore affinity to what it was.
+  EXPECT_EQ(0, sched_setaffinity(0, sizeof(original_set), &original_set));
+  EXPECT_EQ(original_cpu_count, platform_util_.determineCpuCoresWithAffinity());
+}
+
 } // namespace Nighthawk
diff --git a/test/process_test.cc b/test/process_test.cc
@@ -4,6 +4,7 @@
 #include "client/process_impl.h"
 
 #include "test/client/utility.h"
+#include "test/mocks.h"
 #include "test/test_common/utility.h"
 
 #include "gtest/gtest.h"
@@ -16,17 +17,15 @@ namespace Client {
 // TODO(oschaaf): when we have proper integration testing, update this.
 // For now we are covered via the client_tests.cc by proxy. Eventually we
 // want those tests in here, and mock Process in client_test.
-class ProcessTest : public Test {
+class ProcessTest : public testing::Test {
 public:
   void runProcess() {
-
     OptionsPtr options = TestUtility::createOptionsImpl(
         fmt::format("foo --address-family v4 --duration 2 --rps 10 http://127.0.0.1/"));
-
-    ProcessPtr process = std::make_unique<ProcessImpl>(*options, time_system_);
+    PlatformUtilImpl platform_util;
+    ProcessPtr process = std::make_unique<ProcessImpl>(*options, time_system_, platform_util);
     OutputCollectorFactoryImpl output_format_factory(time_system_, *options);
     auto collector = output_format_factory.create();
-
     EXPECT_TRUE(process->run(*collector));
     process.reset();
   }
@@ -39,5 +38,19 @@ TEST_F(ProcessTest, TwoProcesssInSequence) {
   runProcess();
 }
 
+TEST_F(ProcessTest, CpuAffinityDetectionFailure) {
+  OptionsPtr options = TestUtility::createOptionsImpl(
+      fmt::format("foo --address-family v4 --duration 2 --rps 10 http://127.0.0.1/"));
+  MockPlatformUtil platform_util;
+  ProcessPtr process = std::make_unique<ProcessImpl>(*options, time_system_, platform_util);
+  OutputCollectorFactoryImpl output_format_factory(time_system_, *options);
+  auto collector = output_format_factory.create();
+  // Will return 0, which happens on failure in the implementation.
+  EXPECT_CALL(platform_util, determineCpuCoresWithAffinity);
+  EXPECT_TRUE(process->run(*collector));
+  // TODO(oschaaf): check the proto output that we reflect the concurreny we actually used.
+  // I'm not sure we do so right now.
+}
+
 } // namespace Client
 } // namespace Nighthawk
diff --git a/test/utility_test.cc b/test/utility_test.cc
@@ -26,8 +26,6 @@ class UtilityTest : public Test {
     EXPECT_EQ(scheme, uri.scheme());
     EXPECT_EQ(path, uri.path());
   }
-
-  int32_t getCpuCountFromSet(cpu_set_t& set) { return CPU_COUNT(&set); }
 };
 
 TEST_F(UtilityTest, PerfectlyFineUrl) {
@@ -145,28 +143,6 @@ TEST_P(UtilityAddressResolutionTest, ResolveTwiceReturnsCached) {
             u.resolve(*dispatcher, address_family).get());
 }
 
-// TODO(oschaaf): we probably want to move this out to another file.
-TEST_F(UtilityTest, CpusWithAffinity) {
-  cpu_set_t original_set;
-  CPU_ZERO(&original_set);
-  EXPECT_EQ(0, sched_getaffinity(0, sizeof(original_set), &original_set));
-
-  uint32_t original_cpu_count = PlatformUtils::determineCpuCoresWithAffinity();
-  EXPECT_EQ(original_cpu_count, getCpuCountFromSet(original_set));
-
-  // Now the test, we set affinity to just the first cpu. We expect that to be reflected.
-  // This will be a no-op on a single core system.
-  cpu_set_t test_set;
-  CPU_ZERO(&test_set);
-  CPU_SET(0, &test_set);
-  EXPECT_EQ(0, sched_setaffinity(0, sizeof(test_set), &test_set));
-  EXPECT_EQ(1, PlatformUtils::determineCpuCoresWithAffinity());
-
-  // Restore affinity to what it was.
-  EXPECT_EQ(0, sched_setaffinity(0, sizeof(original_set), &original_set));
-  EXPECT_EQ(original_cpu_count, PlatformUtils::determineCpuCoresWithAffinity());
-}
-
 TEST_F(UtilityTest, translateAddressFamilyGoodValues) {
   EXPECT_EQ(Envoy::Network::DnsLookupFamily::V6Only,
             Utility::translateFamilyOptionString(

Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ ConsoleOutputCollectorImpl::formatProtoDuration(const Envoy::Protobuf::Duration&`
`78`	`78`	`(c % 1'000'000) / 1'000, c % 1'000);`
`79`	`79`	`}`
`80`	`80`
`81`		`-std::string ConsoleOutputCollectorImpl::statIdtoFriendlyStatName(absl::string_view stat_id) const {`
	`81`	`+std::string ConsoleOutputCollectorImpl::statIdtoFriendlyStatName(absl::string_view stat_id) {`
`82`	`82`	`if (stat_id == "benchmark_http_client.queue_to_connect") {`
`83`	`83`	`return "Queueing and connection setup latency";`
`84`	`84`	`} else if (stat_id == "benchmark_http_client.request_to_response") {`