From 6d6e613548de693022701ca9396de6f3e0697ec5 Mon Sep 17 00:00:00 2001 From: code Date: Wed, 16 Oct 2024 22:14:16 +0800 Subject: [PATCH] formatter: use the new json lib to relace protobuf json (#36530) Commit Message: formatter: use the new json lib to relace protobuf json Additional Description: This make our formatter and logger won't depend on the full proto and yaml support. But note the serializer has minor difference with the one that provided by the proto lib. The NaN and Infinity values of float will be serialized to ``null`` and ``"inf"`` respectively in the metadata (``DYNAMIC_METADATA``, ``CLUSTER_METADATA``, etc.) formatter. And the proto one will return an error message. I personally think in the scenario of formatter and logger, the `null` and `"inf"` is better. But this is still a minor behavior change. Risk Level: low. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: wangbaiping --- changelogs/current.yaml | 4 ++ source/common/formatter/BUILD | 2 + .../common/formatter/stream_info_formatter.cc | 14 ++---- .../common/formatter/substitution_formatter.h | 46 ++----------------- .../formatter/substitution_formatter_test.cc | 9 +--- 5 files changed, 14 insertions(+), 61 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 6a2e4058b598..6264ee661c77 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -5,6 +5,10 @@ behavior_changes: minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* +- area: formatter + change: | + The NaN and Infinity values of float will be serialized to ``null`` and ``"inf"`` respectively in the + metadata (``DYNAMIC_METADATA``, ``CLUSTER_METADATA``, etc.) formatter. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/source/common/formatter/BUILD b/source/common/formatter/BUILD index 76a0fc474cfe..57d08475ef2a 100644 --- a/source/common/formatter/BUILD +++ b/source/common/formatter/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//source/common/http:utility_lib", "//source/common/json:json_loader_lib", "//source/common/json:json_streamer_lib", + "//source/common/json:json_utility_lib", "@com_google_absl//absl/strings:str_format", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], @@ -101,6 +102,7 @@ envoy_cc_library( "//source/common/grpc:common_lib", "//source/common/http:utility_lib", "//source/common/json:json_loader_lib", + "//source/common/json:json_utility_lib", "//source/common/protobuf:message_validator_lib", "//source/common/stream_info:utility_lib", ], diff --git a/source/common/formatter/stream_info_formatter.cc b/source/common/formatter/stream_info_formatter.cc index 5ff679a731fd..5463da936071 100644 --- a/source/common/formatter/stream_info_formatter.cc +++ b/source/common/formatter/stream_info_formatter.cc @@ -3,6 +3,7 @@ #include "source/common/common/random_generator.h" #include "source/common/config/metadata.h" #include "source/common/http/utility.h" +#include "source/common/json/json_utility.h" #include "source/common/runtime/runtime_features.h" #include "source/common/stream_info/utility.h" @@ -61,20 +62,11 @@ MetadataFormatter::formatMetadata(const envoy::config::core::v3::Metadata& metad } std::string str; + str.reserve(256); if (value.kind_case() == ProtobufWkt::Value::kStringValue) { str = value.string_value(); } else { -#ifdef ENVOY_ENABLE_YAML - absl::StatusOr json_or_error = - MessageUtil::getJsonStringFromMessage(value, false, true); - if (json_or_error.ok()) { - str = json_or_error.value(); - } else { - str = json_or_error.status().message(); - } -#else - IS_ENVOY_BUG("Json support compiled out"); -#endif + Json::Utility::appendValueToString(value, str); } SubstitutionFormatUtils::truncate(str, max_length_); return str; diff --git a/source/common/formatter/substitution_formatter.h b/source/common/formatter/substitution_formatter.h index 2e04904423d5..e2480d1a2f1a 100644 --- a/source/common/formatter/substitution_formatter.h +++ b/source/common/formatter/substitution_formatter.h @@ -16,6 +16,7 @@ #include "source/common/formatter/http_formatter_context.h" #include "source/common/json/json_loader.h" #include "source/common/json/json_streamer.h" +#include "source/common/json/json_utility.h" #include "absl/types/optional.h" #include "re2/re2.h" @@ -439,7 +440,8 @@ class JsonFormatterImplBase : public FormatterBase { } else { // 3. Handle the formatter element with a single provider and value // type needs to be kept. - typedValueToLogLine(formatters, context, info, serializer); + auto value = formatters[0]->formatValueWithContext(context, info); + Json::Utility::appendValueToString(value, log_line); } } @@ -467,48 +469,6 @@ class JsonFormatterImplBase : public FormatterBase { serializer.addRawString(Json::Constants::DoubleQuote); // End the JSON string. } - void typedValueToLogLine(const Formatters& formatters, const FormatterContext& context, - const StreamInfo::StreamInfo& info, - JsonStringSerializer& serializer) const { - - const ProtobufWkt::Value value = formatters[0]->formatValueWithContext(context, info); - - switch (value.kind_case()) { - case ProtobufWkt::Value::KIND_NOT_SET: - case ProtobufWkt::Value::kNullValue: - serializer.addNull(); - break; - case ProtobufWkt::Value::kNumberValue: - serializer.addNumber(value.number_value()); - break; - case ProtobufWkt::Value::kStringValue: - serializer.addString(value.string_value()); - break; - case ProtobufWkt::Value::kBoolValue: - serializer.addBool(value.bool_value()); - break; - case ProtobufWkt::Value::kStructValue: - case ProtobufWkt::Value::kListValue: - // Convert the struct or list to string. This may result in a performance - // degradation. But We rarely hit this case. - // Basically only metadata or filter state formatter may hit this case. -#ifdef ENVOY_ENABLE_YAML - absl::StatusOr json_or = - MessageUtil::getJsonStringFromMessage(value, false, true); - if (json_or.ok()) { - // We assume the output of getJsonStringFromMessage is a valid JSON string piece. - serializer.addRawString(json_or.value()); - } else { - serializer.addString(json_or.status().ToString()); - } -#else - IS_ENVOY_BUG("Json support compiled out"); - serializer.addRawString(R"EOF("Json support compiled out")EOF"); -#endif - break; - } - } - const std::string empty_value_; using ParsedFormatElement = absl::variant; diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 9c058bdf46ef..853e0eab1f91 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -2699,9 +2699,7 @@ TEST(SubstitutionFormatterTest, DynamicMetadataFieldExtractor) { DynamicMetadataFormatter formatter("com.test", {"nan_val"}, absl::optional()); absl::optional value = formatter.format(stream_info); - EXPECT_EQ("google.protobuf.Value cannot encode double values for nan, because it would be " - "parsed as a string", - value.value()); + EXPECT_EQ("null", value.value()); } { @@ -2713,9 +2711,7 @@ TEST(SubstitutionFormatterTest, DynamicMetadataFieldExtractor) { DynamicMetadataFormatter formatter("com.test", {"inf_val"}, absl::optional()); absl::optional value = formatter.format(stream_info); - EXPECT_EQ("google.protobuf.Value cannot encode double values for infinity, because it would be " - "parsed as a string", - value.value()); + EXPECT_EQ("inf", value.value()); } } @@ -4475,7 +4471,6 @@ TEST(SubstitutionFormatterTest, JsonFormatterTest) { JsonFormatterImpl formatter(key_mapping, false); const std::string out_json = formatter.formatWithContext(formatter_context, stream_info); - std::cout << out_json << std::endl; EXPECT_TRUE(TestUtility::jsonStringEqual(out_json, expected)); }