Pass around plugin names along with plugins and output (#929)

This change does the following three broad things: 1. Passes plugin names whenever the plugin pointers are passed. 2. getUserDefinedOutputResults now returns UserDefinedOutput instead of Any, so that it can include the name of the plugin the config goes with 3. created using statements for a couple std::pairs that are long and used often Signed-off-by: Nathan Perry <[email protected]>
envoyproxy · Oct 25, 2022 · f42add9 · f42add9
1 parent f5a3d84
commit f42add9
Show file tree

Hide file tree

Showing 20 changed files with 105 additions and 78 deletions.
diff --git a/ci/do_ci.sh b/ci/do_ci.sh
@@ -90,7 +90,7 @@ function do_integration_test_coverage() {
     export TEST_TARGETS="//test:python_test"
     # TODO(#830): Raise the integration test coverage.
     # TODO(nbperry): Raise back to 73 when User Defined Output plugin completed
-    export COVERAGE_THRESHOLD=72.3
+    export COVERAGE_THRESHOLD=72.2
     echo "bazel coverage build with tests ${TEST_TARGETS}"
     test/run_nighthawk_bazel_coverage.sh ${TEST_TARGETS}
     exit 0

diff --git a/include/nighthawk/client/benchmark_client.h b/include/nighthawk/client/benchmark_client.h
@@ -65,10 +65,10 @@ class BenchmarkClient {
    * Get additional output generated by UserDefinedOutput plugins associated with this benchmark
    * client.
    *
-   * @return a vector of protos. Not guaranteed for all of the Any protobufs to have the same
-   * underlying type.
+   * @return a vector of protos, each the output result to a User Defined Output Plugin.
    */
-  virtual std::vector<Envoy::ProtobufWkt::Any> getUserDefinedOutputResults() const PURE;
+  virtual std::vector<nighthawk::client::UserDefinedOutput>
+  getUserDefinedOutputResults() const PURE;
 };
 
 using BenchmarkClientPtr = std::unique_ptr<BenchmarkClient>;

diff --git a/include/nighthawk/client/client_worker.h b/include/nighthawk/client/client_worker.h
@@ -44,10 +44,10 @@ class ClientWorker : virtual public Worker {
    * Get additional output generated by UserDefinedOutput plugins associated with this client
    * worker.
    *
-   * @return a vector of protos. Not guaranteed for all of the Any protobufs to have the same
-   * underlying type.
+   * @return a vector of protos, each the output result to a User Defined Output Plugin.
    */
-  virtual std::vector<Envoy::ProtobufWkt::Any> getUserDefinedOutputResults() const PURE;
+  virtual std::vector<nighthawk::client::UserDefinedOutput>
+  getUserDefinedOutputResults() const PURE;
 };
 
 using ClientWorkerPtr = std::unique_ptr<ClientWorker>;

diff --git a/include/nighthawk/client/factories.h b/include/nighthawk/client/factories.h
@@ -50,7 +50,7 @@ class BenchmarkClientFactory {
          Envoy::Upstream::ClusterManagerPtr& cluster_manager,
          Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name,
          int worker_id, RequestSource& request_source,
-         std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins) const PURE;
+         std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins) const PURE;
 };
 
 class OutputFormatterFactory {

diff --git a/include/nighthawk/user_defined_output/user_defined_output_plugin.h b/include/nighthawk/user_defined_output/user_defined_output_plugin.h
@@ -8,6 +8,7 @@
 
 #include "external/envoy/source/common/common/statusor.h"
 #include "external/envoy/source/common/http/header_map_impl.h"
+#include "external/envoy_api/envoy/config/core/v3/extension.pb.h"
 
 namespace Nighthawk {
 
@@ -138,4 +139,9 @@ class UserDefinedOutputPluginFactory : public Envoy::Config::TypedFactory {
   virtual absl::StatusOr<Envoy::ProtobufWkt::Any>
   AggregateGlobalOutput(absl::Span<const Envoy::ProtobufWkt::Any> per_worker_outputs) PURE;
 };
+
+using UserDefinedOutputConfigFactoryPair =
+    std::pair<envoy::config::core::v3::TypedExtensionConfig, UserDefinedOutputPluginFactory*>;
+using UserDefinedOutputNamePluginPair = std::pair<std::string, UserDefinedOutputPluginPtr>;
+
 } // namespace Nighthawk
diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc
@@ -88,7 +88,7 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl(
     Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name,
     RequestGenerator request_generator, const bool provide_resource_backpressure,
     absl::string_view latency_response_header_name,
-    std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins)
+    std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins)
     : api_(api), dispatcher_(dispatcher), scope_(scope.createScope("benchmark.")),
       statistic_(std::move(statistic)), protocol_(protocol),
       benchmark_client_counters_({ALL_BENCHMARK_CLIENT_COUNTERS(POOL_COUNTER(*scope_))}),
@@ -224,17 +224,17 @@ void BenchmarkClientHttpImpl::onComplete(bool success,
       benchmark_client_counters_.http_xxx_.inc();
     }
   }
-  for (UserDefinedOutputPluginPtr& plugin : user_defined_output_plugins_) {
-    absl::Status status = plugin->handleResponseHeaders(headers);
+  for (UserDefinedOutputNamePluginPair& plugin : user_defined_output_plugins_) {
+    absl::Status status = plugin.second->handleResponseHeaders(headers);
     if (!status.ok()) {
       benchmark_client_counters_.user_defined_plugin_handle_headers_failure_.inc();
     }
   }
 }
 
 void BenchmarkClientHttpImpl::handleResponseData(const Envoy::Buffer::Instance& response_data) {
-  for (UserDefinedOutputPluginPtr& plugin : user_defined_output_plugins_) {
-    absl::Status status = plugin->handleResponseData(response_data);
+  for (UserDefinedOutputNamePluginPair& plugin : user_defined_output_plugins_) {
+    absl::Status status = plugin.second->handleResponseData(response_data);
     if (!status.ok()) {
       benchmark_client_counters_.user_defined_plugin_handle_data_failure_.inc();
     }
@@ -274,16 +274,20 @@ void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code,
   }
 }
 
-std::vector<Envoy::ProtobufWkt::Any> BenchmarkClientHttpImpl::getUserDefinedOutputResults() const {
-  std::vector<Envoy::ProtobufWkt::Any> outputs;
-  for (const UserDefinedOutputPluginPtr& plugin : user_defined_output_plugins_) {
-    absl::StatusOr<Envoy::ProtobufWkt::Any> message = plugin->getPerWorkerOutput();
+std::vector<nighthawk::client::UserDefinedOutput>
+BenchmarkClientHttpImpl::getUserDefinedOutputResults() const {
+  std::vector<nighthawk::client::UserDefinedOutput> outputs;
+  for (const UserDefinedOutputNamePluginPair& plugin : user_defined_output_plugins_) {
+    absl::StatusOr<Envoy::ProtobufWkt::Any> message = plugin.second->getPerWorkerOutput();
     if (!message.ok()) {
-      ENVOY_LOG(error, "Plugin with class type {} received error status: ", typeid(plugin).name(),
+      ENVOY_LOG(error, "Plugin with class type {} received error status: ", plugin.first,
                 message.status().message());
       benchmark_client_counters_.user_defined_plugin_per_worker_output_failure_.inc();
     } else {
-      outputs.push_back(*message);
+      nighthawk::client::UserDefinedOutput output_result;
+      output_result.set_plugin_name(plugin.first);
+      *output_result.mutable_typed_output() = *message;
+      outputs.push_back(output_result);
     }
   }
   return outputs;

diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h
@@ -113,7 +113,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
                           absl::string_view cluster_name, RequestGenerator request_generator,
                           const bool provide_resource_backpressure,
                           absl::string_view latency_response_header_name,
-                          std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins);
+                          std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins);
   void setConnectionLimit(uint32_t connection_limit) { connection_limit_ = connection_limit; }
   void setMaxPendingRequests(uint32_t max_pending_requests) {
     max_pending_requests_ = max_pending_requests;
@@ -137,10 +137,8 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
 
   /**
    * Returns additional output from any specified User Defined Output plugins.
-   *
-   * @return vector of Envoy::ProtobufWkt::Any, each of which may be a different underlying proto.
    */
-  std::vector<Envoy::ProtobufWkt::Any> getUserDefinedOutputResults() const override;
+  std::vector<nighthawk::client::UserDefinedOutput> getUserDefinedOutputResults() const override;
 
   // StreamDecoderCompletionCallback
   void onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) override;
@@ -179,7 +177,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient,
   const bool provide_resource_backpressure_;
   const std::string latency_response_header_name_;
   Envoy::Event::TimerPtr drain_timer_;
-  std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins_;
+  std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins_;
 };
 
 } // namespace Client

diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc
@@ -22,7 +22,7 @@ ClientWorkerImpl::ClientWorkerImpl(
     const int worker_number, const Envoy::MonotonicTime starting_time,
     Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
     const HardCodedWarmupStyle hardcoded_warmup_style,
-    std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins)
+    std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins)
     : WorkerImpl(api, tls, store),
       time_source_(std::make_unique<CachedTimeSourceImpl>(*dispatcher_)),
       termination_predicate_factory_(termination_predicate_factory),
@@ -110,7 +110,8 @@ StatisticPtrMap ClientWorkerImpl::statistics() const {
   return statistics;
 }
 
-std::vector<Envoy::ProtobufWkt::Any> ClientWorkerImpl::getUserDefinedOutputResults() const {
+std::vector<nighthawk::client::UserDefinedOutput>
+ClientWorkerImpl::getUserDefinedOutputResults() const {
   return benchmark_client_->getUserDefinedOutputResults();
 }
 

diff --git a/source/client/client_worker_impl.h b/source/client/client_worker_impl.h
@@ -37,7 +37,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker {
                    const Envoy::MonotonicTime starting_time,
                    Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
                    const HardCodedWarmupStyle hardcoded_warmup_style,
-                   std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins);
+                   std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins);
   StatisticPtrMap statistics() const override;
 
   const std::map<std::string, uint64_t>& threadLocalCounterValues() override {
@@ -52,10 +52,8 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker {
 
   /**
    * Returns additional output from any specified User Defined Output plugins.
-   *
-   * @return vector of Envoy::ProtobufWkt::Any, each of which may be a different underlying proto.
    */
-  std::vector<Envoy::ProtobufWkt::Any> getUserDefinedOutputResults() const override;
+  std::vector<nighthawk::client::UserDefinedOutput> getUserDefinedOutputResults() const override;
 
 protected:
   void work() override;

diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc
@@ -34,7 +34,7 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create(
     Envoy::Upstream::ClusterManagerPtr& cluster_manager,
     Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, int worker_id,
     RequestSource& request_generator,
-    std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins) const {
+    std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins) const {
   StatisticFactoryImpl statistic_factory(options_);
   // While we lack options to configure which statistic backend goes where, we directly pass
   // StreamingStatistic for the stats that track response sizes. Ideally we would have options

diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h
@@ -35,7 +35,7 @@ class BenchmarkClientFactoryImpl : public OptionBasedFactoryImpl, public Benchma
          Envoy::Upstream::ClusterManagerPtr& cluster_manager,
          Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name,
          int worker_id, RequestSource& request_generator,
-         std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins) const override;
+         std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins) const override;
 };
 
 class SequencerFactoryImpl : public OptionBasedFactoryImpl, public SequencerFactory {

diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc
@@ -325,14 +325,13 @@ class NighthawkServerFactoryContext : public Envoy::Server::Configuration::Serve
  * @return std::vector<std::pair<TypedExtensionConfig, UserDefinedOutputPluginFactory*>> vector of
  * pairs, each containing a factory and its corresponding configuration.
  */
-std::vector<std::pair<TypedExtensionConfig, UserDefinedOutputPluginFactory*>>
+std::vector<UserDefinedOutputConfigFactoryPair>
 getUserDefinedFactoryConfigPairs(const Options& options) {
-  std::vector<std::pair<TypedExtensionConfig, UserDefinedOutputPluginFactory*>>
-      factory_config_pairs;
+  std::vector<UserDefinedOutputConfigFactoryPair> factory_config_pairs;
   for (const TypedExtensionConfig& config : options.userDefinedOutputPluginConfigs()) {
     auto* factory = Envoy::Config::Utility::getAndCheckFactory<UserDefinedOutputPluginFactory>(
         config, /*is_optional=*/false);
-    std::pair<TypedExtensionConfig, UserDefinedOutputPluginFactory*> pair(config, factory);
+    UserDefinedOutputConfigFactoryPair pair(config, factory);
     factory_config_pairs.push_back(pair);
   }
   return factory_config_pairs;
@@ -544,7 +543,7 @@ absl::Status ProcessImpl::createWorkers(const uint32_t concurrency,
       computeInterWorkerDelay(concurrency, options_.requestsPerSecond());
   int worker_number = 0;
   while (workers_.size() < concurrency) {
-    std::vector<UserDefinedOutputPluginPtr> plugins =
+    std::vector<UserDefinedOutputNamePluginPair> plugins =
         createUserDefinedOutputPlugins(user_defined_output_factories_, worker_number);
     if (!plugins.empty()) {
       return absl::UnimplementedError(

diff --git a/source/client/process_impl.h b/source/client/process_impl.h
@@ -222,9 +222,7 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
   std::unique_ptr<Envoy::Server::Configuration::ServerFactoryContext> server_factory_context_;
   // The set of User Defined Output plugin factories and their corresponding configuration, used to
   // add plugin instances to each worker, and to aggregate outputs for the global result.
-  std::vector<
-      std::pair<envoy::config::core::v3::TypedExtensionConfig, UserDefinedOutputPluginFactory*>>
-      user_defined_output_factories_{};
+  std::vector<UserDefinedOutputConfigFactoryPair> user_defined_output_factories_{};
 };
 
 } // namespace Client

diff --git a/source/user_defined_output/user_defined_output_plugin_creator.cc b/source/user_defined_output/user_defined_output_plugin_creator.cc
@@ -9,18 +9,21 @@ namespace Nighthawk {
 
 using envoy::config::core::v3::TypedExtensionConfig;
 
-std::vector<UserDefinedOutputPluginPtr> createUserDefinedOutputPlugins(
-    std::vector<std::pair<TypedExtensionConfig, UserDefinedOutputPluginFactory*>>&
-        factory_config_pairs,
-    int worker_number) {
-  std::vector<UserDefinedOutputPluginPtr> plugins;
+std::vector<UserDefinedOutputNamePluginPair> createUserDefinedOutputPlugins(
+    std::vector<UserDefinedOutputConfigFactoryPair>& factory_config_pairs, int worker_number) {
+  std::vector<UserDefinedOutputNamePluginPair> plugins;
 
   for (auto& pair : factory_config_pairs) {
     WorkerMetadata metadata;
     metadata.worker_number = worker_number;
     TypedExtensionConfig config = pair.first;
     UserDefinedOutputPluginFactory* factory = pair.second;
-    plugins.push_back(factory->createUserDefinedOutputPlugin(config.typed_config(), metadata));
+
+    UserDefinedOutputNamePluginPair name_plugin_pair;
+    name_plugin_pair.first = factory->name();
+    name_plugin_pair.second =
+        factory->createUserDefinedOutputPlugin(config.typed_config(), metadata);
+    plugins.emplace_back(std::move(name_plugin_pair));
   }
 
   return plugins;

diff --git a/source/user_defined_output/user_defined_output_plugin_creator.h b/source/user_defined_output/user_defined_output_plugin_creator.h
@@ -20,9 +20,8 @@ namespace Nighthawk {
  * @throws EnvoyException if the config is invalid or couldn't find a corresponding User Defined
  * Output Plugin.
  */
-std::vector<UserDefinedOutputPluginPtr> createUserDefinedOutputPlugins(
-    std::vector<std::pair<envoy::config::core::v3::TypedExtensionConfig,
-                          UserDefinedOutputPluginFactory*>>& factory_pairs,
-    int worker_number);
+std::vector<UserDefinedOutputNamePluginPair>
+createUserDefinedOutputPlugins(std::vector<UserDefinedOutputConfigFactoryPair>& factory_pairs,
+                               int worker_number);
 
 } // namespace Nighthawk
diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc
@@ -227,7 +227,7 @@ class BenchmarkClientHttpTest : public Test {
   int worker_number_{0};
   Client::BenchmarkClientStatistic statistic_;
   std::shared_ptr<Envoy::Http::RequestHeaderMap> default_header_map_;
-  std::vector<UserDefinedOutputPluginPtr> user_defined_output_plugins_{};
+  std::vector<UserDefinedOutputNamePluginPair> user_defined_output_plugins_{};
 };
 
 TEST_F(BenchmarkClientHttpTest, BasicTestH1200) {
@@ -481,7 +481,10 @@ TEST_F(BenchmarkClientHttpTest, CallsUserDefinedPluginHandleHeaders) {
     }
   )");
   UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
   client_->onComplete(true, headers);
@@ -509,7 +512,10 @@ TEST_F(BenchmarkClientHttpTest, IncrementsCounterWhenUserDefinedPluginHandleHead
     }
   )");
   UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
   client_->onComplete(true, headers);
@@ -532,7 +538,10 @@ TEST_F(BenchmarkClientHttpTest, CallsUserDefinedPluginHandleData) {
     }
   )");
   UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
   client_->handleResponseData(buffer);
@@ -557,7 +566,10 @@ TEST_F(BenchmarkClientHttpTest, IncrementsCounterWhenUserDefinedPluginHandleData
     }
   )");
   UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
   client_->handleResponseData(buffer);
@@ -584,17 +596,24 @@ TEST_F(BenchmarkClientHttpTest, GetUserDefinedOutputResultsReturnsResults) {
     }
   )");
   UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
   client_->onComplete(true, headers);
   client_->handleResponseData(buffer);
-  absl::StatusOr<Envoy::ProtobufWkt::Any> expected_output = plugin_ptr->getPerWorkerOutput();
-  ASSERT_TRUE(expected_output.ok());
-
-  std::vector<Envoy::ProtobufWkt::Any> outputs = client_->getUserDefinedOutputResults();
+  absl::StatusOr<Envoy::ProtobufWkt::Any> expected_any = plugin_ptr->getPerWorkerOutput();
+  ASSERT_TRUE(expected_any.ok());
+  nighthawk::client::UserDefinedOutput expected_output;
+  *expected_output.mutable_typed_output() = *expected_any;
+  expected_output.set_plugin_name("nighthawk.fake_user_defined_output");
+
+  std::vector<nighthawk::client::UserDefinedOutput> outputs =
+      client_->getUserDefinedOutputResults();
   EXPECT_EQ(outputs.size(), 1);
-  EXPECT_THAT(outputs[0], EqualsProto(*expected_output));
+  EXPECT_THAT(outputs[0], EqualsProto(expected_output));
 }
 
 TEST_F(BenchmarkClientHttpTest,
@@ -608,11 +627,14 @@ TEST_F(BenchmarkClientHttpTest,
       }
     }
   )");
-  // UserDefinedOutputPlugin* plugin_ptr = plugin.get();
-  user_defined_output_plugins_.push_back(std::move(plugin));
+  UserDefinedOutputNamePluginPair pair;
+  pair.first = "nighthawk.fake_user_defined_output";
+  pair.second = std::move(plugin);
+  user_defined_output_plugins_.push_back(std::move(pair));
   setupBenchmarkClient(default_request_generator);
 
-  std::vector<Envoy::ProtobufWkt::Any> outputs = client_->getUserDefinedOutputResults();
+  std::vector<nighthawk::client::UserDefinedOutput> outputs =
+      client_->getUserDefinedOutputResults();
   EXPECT_TRUE(outputs.empty());
   EXPECT_EQ(getCounter("user_defined_plugin_per_worker_output_failure"), 1);
 }