Skip to content

Commit

Permalink
formatter: use the new json lib to relace protobuf json (#36530)
Browse files Browse the repository at this point in the history
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 <wangbaiping@bytedance.com>
  • Loading branch information
wbpcode authored Oct 16, 2024
1 parent d2e0e20 commit 6d6e613
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 61 deletions.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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*
Expand Down
2 changes: 2 additions & 0 deletions source/common/formatter/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down Expand Up @@ -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",
],
Expand Down
14 changes: 3 additions & 11 deletions source/common/formatter/stream_info_formatter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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<std::string> 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;
Expand Down
46 changes: 3 additions & 43 deletions source/common/formatter/substitution_formatter.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -439,7 +440,8 @@ class JsonFormatterImplBase : public FormatterBase<FormatterContext> {
} 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);
}
}

Expand Down Expand Up @@ -467,48 +469,6 @@ class JsonFormatterImplBase : public FormatterBase<FormatterContext> {
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<std::string> 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<std::string, Formatters>;
Expand Down
9 changes: 2 additions & 7 deletions test/common/formatter/substitution_formatter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2699,9 +2699,7 @@ TEST(SubstitutionFormatterTest, DynamicMetadataFieldExtractor) {

DynamicMetadataFormatter formatter("com.test", {"nan_val"}, absl::optional<size_t>());
absl::optional<std::string> 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());
}

{
Expand All @@ -2713,9 +2711,7 @@ TEST(SubstitutionFormatterTest, DynamicMetadataFieldExtractor) {

DynamicMetadataFormatter formatter("com.test", {"inf_val"}, absl::optional<size_t>());
absl::optional<std::string> 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());
}
}

Expand Down Expand Up @@ -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));
}

Expand Down

0 comments on commit 6d6e613

Please sign in to comment.