formatter: use the new json lib to relace protobuf json #36530
Conversation
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
|
Hi, cc @jmarantz by the way, why we treat the NaN as null but keep the support to the Infinity? |
|
/retest |
| @@ -124,6 +124,10 @@ minor_behavior_changes: | |||
| change: | | |||
| Sanitize SNI for potential log injection. The invalid character will be replaced by ``_`` with an ``invalid:`` marker. If runtime | |||
| flag ``envoy.reloadable_features.sanitize_sni_in_access_log`` is set to ``false``, the sanitize behavior is disabled. | |||
| - area: formatter | |||
There was a problem hiding this comment.
Should this be runtime guarded?
There was a problem hiding this comment.
I am not sure. Because, this behavior change means:
For a NaN value, the output of the command will be changed from google.protobuf.Value cannot encode double values for nan, because it would be parsed as a string to null.
For a Infinity value, the output of the command will be changed from google.protobuf.Value cannot encode double values for infinity, because it would be parsed as a string to Inf.
I doubt no one will depend on original behavior. So, not sure if it deserve a runtime flag.
Defer this to you to determine should we need to add a runtime flag for this. Anyway decision is fine to me. :)
| #else | ||
| IS_ENVOY_BUG("Json support compiled out"); | ||
| #endif | ||
| Json::Utility::appendValueToString(value, str); |
There was a problem hiding this comment.
I do love having one less non-YAML special case, thanks :-)
|
I vaguely remember the decision to render NaN values as 'null' but do not remember thinking one way or the other about 'infinity'. Please do the right thing :) |
I actually don't know what is right. 😭 So, just kept the JsonStreamer's behavior. |
|
There is no concept of 'infinity' in json. If our current serializer can generate 'Infinity' that is invalid Json and we should make it stop :) https://medium.com/the-magic-pantry/infinity-and-json-cde6df62c17c has some thoughts to substitute 1e1000. 'null' would also be fine. |
|
I have no intuition on the json. I assume NAN shouldn't be used in any case so let's say you can land without runtime guard if you want but wait until after the Tuesday release? |
Of course. Thanks for the confirmation. |
…mprove-serialize-of-value
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
nulland"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
nulland"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:]