Tests for StructFormatter

Signed-off-by: Itamar Kaminski <itamark@google.com>

Delete some leftovers

Signed-off-by: Itamar Kaminski <itamark@google.com>

Delete some leftovers

Signed-off-by: Itamar Kaminski <itamark@google.com>

clang-format

Signed-off-by: Itamar Kaminski <itamark@google.com>

Revert "clang-format"

This reverts commit 4576bc86fdb8baa2f161bde2dbdd7871169f725f.

Signed-off-by: Itamar Kaminski <itamark@google.com>

Fix error message formatting test

Fix clang-tidy

Signed-off-by: Itamar Kaminski <itamark@google.com>

Fix clang tidy again

Signed-off-by: Itamar Kaminski <itamark@google.com>

source/common/formatter/substitution_formatter.cc

@@ -135,8 +135,8 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head
                                       const Http::ResponseTrailerMap& response_trailers,
                                       const StreamInfo::StreamInfo& stream_info,
                                       absl::string_view local_reply_body) const {
-  const auto output_struct =
-      struct_formatter_.format(request_headers, response_headers, response_trailers, stream_info, local_reply_body);
+  const auto output_struct = struct_formatter_.format(
+      request_headers, response_headers, response_trailers, stream_info, local_reply_body);
 
   const std::string log_line = MessageUtil::getJsonStringFromMessage(output_struct, false, true);
   return absl::StrCat(log_line, "\n");
@@ -154,18 +154,18 @@ StructFormatter::toFormatMap(const ProtobufWkt::Struct& struct_format) const {
       output->emplace(pair.first, toFormatMap(pair.second.struct_value()));
       break;
     default:
-      throw EnvoyException(
-          "Only string values or nested structs are supported in the Struct/JSON access log format.");
+      throw EnvoyException("Only string values or nested structs are supported in the Struct/JSON "
+                           "access log format.");
     }
   }
   return {std::move(output)};
 };
 
 ProtobufWkt::Struct StructFormatter::format(const Http::RequestHeaderMap& request_headers,
-                                                const Http::ResponseHeaderMap& response_headers,
-                                                const Http::ResponseTrailerMap& response_trailers,
-                                                const StreamInfo::StreamInfo& stream_info,
-                                                absl::string_view local_reply_body) const {
+                                            const Http::ResponseHeaderMap& response_headers,
+                                            const Http::ResponseTrailerMap& response_trailers,
+                                            const StreamInfo::StreamInfo& stream_info,
+                                            absl::string_view local_reply_body) const {
   const std::string& empty_value =
       omit_empty_values_ ? EMPTY_STRING : DefaultUnspecifiedValueString;
   const std::function<ProtobufWkt::Value(const std::vector<FormatterProviderPtr>&)>

source/common/formatter/substitution_formatter.h

@@ -112,17 +112,17 @@ class FormatterImpl : public Formatter {
  * can be converted easily into multiple formats.
  */
 class StructFormatter {
-  public:
+public:
   StructFormatter(const ProtobufWkt::Struct& format_mapping, bool preserve_types,
-                    bool omit_empty_values)
+                  bool omit_empty_values)
       : omit_empty_values_(omit_empty_values), preserve_types_(preserve_types),
         struct_output_format_(toFormatMap(format_mapping)) {}
 
   ProtobufWkt::Struct format(const Http::RequestHeaderMap& request_headers,
-                     const Http::ResponseHeaderMap& response_headers,
-                     const Http::ResponseTrailerMap& response_trailers,
-                     const StreamInfo::StreamInfo& stream_info,
-                     absl::string_view local_reply_body) const;
+                             const Http::ResponseHeaderMap& response_headers,
+                             const Http::ResponseTrailerMap& response_trailers,
+                             const StreamInfo::StreamInfo& stream_info,
+                             absl::string_view local_reply_body) const;
 
 private:
   struct StructFormatMapWrapper;
@@ -146,7 +146,7 @@ class JsonFormatterImpl : public Formatter {
 public:
   JsonFormatterImpl(const ProtobufWkt::Struct& format_mapping, bool preserve_types,
                     bool omit_empty_values)
-      : struct_formatter_(StructFormatter(format_mapping, preserve_types, omit_empty_values)) {}
+      : struct_formatter_(format_mapping, preserve_types, omit_empty_values) {}
 
   // Formatter::format
   std::string format(const Http::RequestHeaderMap& request_headers,

test/common/formatter/substitution_format_string_test.cc

@@ -90,7 +90,7 @@ TEST_F(SubstitutionFormatStringUtilsTest, TestInvalidConfigs) {
     TestUtility::loadFromYaml(yaml, config_);
     EXPECT_THROW_WITH_MESSAGE(
         SubstitutionFormatStringUtils::fromProtoConfig(config_), EnvoyException,
-        "Only string values or nested structs are supported in the JSON access log format.");
+        "Only string values or nested structs are supported in the Struct/JSON access log format.");
   }
 }
 

test/common/formatter/substitution_formatter_speed_test.cc

@@ -28,6 +28,24 @@ std::unique_ptr<Envoy::Formatter::JsonFormatterImpl> makeJsonFormatter(bool type
   return std::make_unique<Envoy::Formatter::JsonFormatterImpl>(JsonLogFormat, typed, false);
 }
 
+std::unique_ptr<Envoy::Formatter::StructFormatter> makeStructFormatter(bool typed) {
+  ProtobufWkt::Struct StructLogFormat;
+  const std::string format_yaml = R"EOF(
+    remote_address: '%DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT%'
+    start_time: '%START_TIME(%Y/%m/%dT%H:%M:%S%z %s)%'
+    method: '%REQ(:METHOD)%'
+    url: '%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%'
+    protocol: '%PROTOCOL%'
+    respoinse_code: '%RESPONSE_CODE%'
+    bytes_sent: '%BYTES_SENT%'
+    duration: '%DURATION%'
+    referer: '%REQ(REFERER)%'
+    user-agent: '%REQ(USER-AGENT)%'
+  )EOF";
+  TestUtility::loadFromYaml(format_yaml, StructLogFormat);
+  return std::make_unique<Envoy::Formatter::StructFormatter>(StructLogFormat, typed, false);
+}
+
 std::unique_ptr<Envoy::TestStreamInfo> makeStreamInfo() {
   auto stream_info = std::make_unique<Envoy::TestStreamInfo>();
   stream_info->setDownstreamRemoteAddress(
@@ -54,7 +72,7 @@ static void BM_AccessLogFormatter(benchmark::State& state) {
   Http::TestResponseHeaderMapImpl response_headers;
   Http::TestResponseTrailerMapImpl response_trailers;
   std::string body;
-  for (auto _ : state) {
+  for (auto _ : state) { // NOLINT: Silences warning about dead store
     output_bytes +=
         formatter->format(request_headers, response_headers, response_trailers, *stream_info, body)
             .length();
@@ -63,6 +81,47 @@ static void BM_AccessLogFormatter(benchmark::State& state) {
 }
 BENCHMARK(BM_AccessLogFormatter);
 
+// NOLINTNEXTLINE(readability-identifier-naming)
+static void BM_StructAccessLogFormatter(benchmark::State& state) {
+  std::unique_ptr<Envoy::TestStreamInfo> stream_info = makeStreamInfo();
+  std::unique_ptr<Envoy::Formatter::StructFormatter> struct_formatter = makeStructFormatter(false);
+
+  size_t output_bytes = 0;
+  Http::TestRequestHeaderMapImpl request_headers;
+  Http::TestResponseHeaderMapImpl response_headers;
+  Http::TestResponseTrailerMapImpl response_trailers;
+  std::string body;
+  for (auto _ : state) { // NOLINT: Silences warning about dead store
+    output_bytes +=
+        struct_formatter
+            ->format(request_headers, response_headers, response_trailers, *stream_info, body)
+            .ByteSize();
+  }
+  benchmark::DoNotOptimize(output_bytes);
+}
+BENCHMARK(BM_StructAccessLogFormatter);
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+static void BM_TypedStructAccessLogFormatter(benchmark::State& state) {
+  std::unique_ptr<Envoy::TestStreamInfo> stream_info = makeStreamInfo();
+  std::unique_ptr<Envoy::Formatter::StructFormatter> typed_struct_formatter =
+      makeStructFormatter(true);
+
+  size_t output_bytes = 0;
+  Http::TestRequestHeaderMapImpl request_headers;
+  Http::TestResponseHeaderMapImpl response_headers;
+  Http::TestResponseTrailerMapImpl response_trailers;
+  std::string body;
+  for (auto _ : state) { // NOLINT: Silences warning about dead store
+    output_bytes +=
+        typed_struct_formatter
+            ->format(request_headers, response_headers, response_trailers, *stream_info, body)
+            .ByteSize();
+  }
+  benchmark::DoNotOptimize(output_bytes);
+}
+BENCHMARK(BM_TypedStructAccessLogFormatter);
+
 // NOLINTNEXTLINE(readability-identifier-naming)
 static void BM_JsonAccessLogFormatter(benchmark::State& state) {
   std::unique_ptr<Envoy::TestStreamInfo> stream_info = makeStreamInfo();
@@ -73,7 +132,7 @@ static void BM_JsonAccessLogFormatter(benchmark::State& state) {
   Http::TestResponseHeaderMapImpl response_headers;
   Http::TestResponseTrailerMapImpl response_trailers;
   std::string body;
-  for (auto _ : state) {
+  for (auto _ : state) { // NOLINT: Silences warning about dead store
     output_bytes +=
         json_formatter
             ->format(request_headers, response_headers, response_trailers, *stream_info, body)
@@ -94,7 +153,7 @@ static void BM_TypedJsonAccessLogFormatter(benchmark::State& state) {
   Http::TestResponseHeaderMapImpl response_headers;
   Http::TestResponseTrailerMapImpl response_trailers;
   std::string body;
-  for (auto _ : state) {
+  for (auto _ : state) { // NOLINT: Silences warning about dead store
     output_bytes +=
         typed_json_formatter
             ->format(request_headers, response_headers, response_trailers, *stream_info, body)