Change User Defined Output proto to have optional error status (#949)

Remove the counter that previously was intended to track these errors. This new solution provides more information to users and removes the weirdness of having a termination predicate that would only terminate after all work is done. Signed-off-by: Nathan Perry <nbperry@google.com>
envoyproxy · Nov 30, 2022 · 201b828 · 201b828
1 parent e489141
commit 201b828
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 16 deletions.
diff --git a/api/client/output.proto b/api/client/output.proto
@@ -49,8 +49,12 @@ message Statistic {
 message UserDefinedOutput {
   // The official name of the plugin as registered through Envoy's plugin registration system.
   string plugin_name = 1;
-  // The typed output defined by the plugin containing specific data that the plugin collected.
-  google.protobuf.Any typed_output = 2;
+  oneof output_type {
+    // The typed output defined by the plugin containing specific data that the plugin collected.
+    google.protobuf.Any typed_output = 2;
+    // The error returned by the plugin when trying to generate the output.
+    string error_message = 3;
+  }
 }
 
 // The set of output collected by Nighthawk for each worker, or for the aggregate of all workers.

diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc
@@ -278,17 +278,17 @@ 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()) {
+    absl::StatusOr<Envoy::ProtobufWkt::Any> per_worker_output = plugin.second->getPerWorkerOutput();
+    nighthawk::client::UserDefinedOutput output_result;
+    output_result.set_plugin_name(plugin.first);
+    if (!per_worker_output.ok()) {
       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();
+                per_worker_output.status().message());
+      *output_result.mutable_error_message() = per_worker_output.status().ToString();
     } else {
-      nighthawk::client::UserDefinedOutput output_result;
-      output_result.set_plugin_name(plugin.first);
-      *output_result.mutable_typed_output() = *message;
-      outputs.push_back(output_result);
+      *output_result.mutable_typed_output() = *per_worker_output;
     }
+    outputs.push_back(output_result);
   }
   return outputs;
 }

diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h
@@ -42,8 +42,7 @@ using namespace std::chrono_literals;
   COUNTER(pool_overflow)                                                                           \
   COUNTER(pool_connection_failure)                                                                 \
   COUNTER(user_defined_plugin_handle_headers_failure)                                              \
-  COUNTER(user_defined_plugin_handle_data_failure)                                                 \
-  COUNTER(user_defined_plugin_per_worker_output_failure)
+  COUNTER(user_defined_plugin_handle_data_failure)
 
 // For counter metrics, Nighthawk use Envoy Counter directly. For histogram metrics, Nighthawk uses
 // its own Statistic instead of Envoy Histogram. Here BenchmarkClientCounters contains only counters

diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc
@@ -616,8 +616,7 @@ TEST_F(BenchmarkClientHttpTest, GetUserDefinedOutputResultsReturnsResults) {
   EXPECT_THAT(outputs[0], EqualsProto(expected_output));
 }
 
-TEST_F(BenchmarkClientHttpTest,
-       getUserDefinedOutputResultsIncrementsCountersWhenPluginsReturnErrors) {
+TEST_F(BenchmarkClientHttpTest, GetUserDefinedOutputResultsIncludesErrorsWhenPluginsReturnErrors) {
   RequestGenerator default_request_generator = getDefaultRequestGenerator();
   UserDefinedOutputPluginPtr plugin = CreateTestUserDefinedOutputPlugin(R"(
     name: "nighthawk.fake_user_defined_output",
@@ -635,8 +634,13 @@ TEST_F(BenchmarkClientHttpTest,
 
   std::vector<nighthawk::client::UserDefinedOutput> outputs =
       client_->getUserDefinedOutputResults();
-  EXPECT_TRUE(outputs.empty());
-  EXPECT_EQ(getCounter("user_defined_plugin_per_worker_output_failure"), 1);
+  EXPECT_EQ(outputs.size(), 1);
+
+  nighthawk::client::UserDefinedOutput expected_output;
+  expected_output.set_plugin_name("nighthawk.fake_user_defined_output");
+  *expected_output.mutable_error_message() =
+      "INTERNAL: Intentional FakeUserDefinedOutputPlugin failure on getting PerWorkerOutput";
+  EXPECT_THAT(outputs[0], EqualsProto(expected_output));
 }
 
 } // namespace Nighthawk