[filter state]: make jsonConvert no throw#18863
Conversation
|
/retest |
|
Retrying Azure Pipelines: |
envoy/stream_info/filter_state.h
Outdated
There was a problem hiding this comment.
This logic still leave possibility for throwing an exception from MessageToJsonString. and it is changing the behavior, ProtobufWkt::Value is proto representation of JSON itself, so the JSON shouldn't set as string.
There was a problem hiding this comment.
I think it is ok if we define serializeAsProtoValue in all the places where serializeAsProto is defined.
There was a problem hiding this comment.
IIUC MessageToJsonString is doesn't throw.
There was a problem hiding this comment.
Why not just keep existing behavior? We can catch those exceptions and turn into null (theoretically they can throw but not realistically). This is an extension point so we never cover all places.
There was a problem hiding this comment.
Because we plan to remove all the exceptions from worker thread(#14320). And the current behavior that covert a MessagePtr to a protobuf::val through intermediate json string format doesn't seems desirable.
There was a problem hiding this comment.
We can just change the protobuf utility to not throw exceptions for those are used in worker thread. We should keep current behavior, a JSON string in ProtobufWkt::Value is not desired at all.
There was a problem hiding this comment.
The reason why we need to provide a default implementation here is that serializeAsProto is not virtual and return nullptr by default. It is difficult to change the utility function jsonConvertValue which have a deep call stack and throw from multiple places, this requires changing the function signature of a few functions which is used frequently in the code base. From my understanding, if we override serializeAsProtoVal for all the classes whose serializeAsProto is being override, that wouldn't change the behavior in the current code base. we can leave comment here to remind the developer to override the serializeAsProtoVal function whenever they override the serializeAsProto.
There was a problem hiding this comment.
As an alternative, we can keep an extra non-throw version of jsonConvertValue without changing the current function signature, which do you prefer?
There was a problem hiding this comment.
The former solution avoid converting to json and back to proto val, which is good for performance, but as you said, would change behavior when user define their serializeAsProto method without defining serializeAsProtoVal.
envoy/stream_info/filter_state.h
Outdated
There was a problem hiding this comment.
I think this function should return nullptr (and return ProtobufWkt::Value by pointer_, the same as the serializeAsProto(). Then it makes the discussion below obsolete.
There was a problem hiding this comment.
How shall we handle the nullptr in formatValue? Shall we return a ProtobufWkt::NULL_VALUE for formatting?
There was a problem hiding this comment.
This is not quite right. You actually need to recreate the envoy::extensions::filters::http::grpc_stats::v3::FilterObject as a Value proto. It is not hard since it just has two uint64 fields. The code would look something like this:
::ProtobufWkt::Value value;
auto *struct_value = value.mutable_struct_value();
::ProtobufWkt::Value::Value string_value;
string_value.set_string_value(absl::StrCat(request_message_count));
(*struct_value->mutable_fields())["request_message_count"] = string_value;
// same for the response_message_count
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
chaoqin-li1123
force-pushed
the
json_exception
branch
from
80caaaf to
0961268
Compare
chaoqin-li1123
changed the title
|
./retest |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>
chaoqin-li1123
dismissed stale reviews from yanavlasov and lizan
via
f58dacd
|
/retest |
|
Retrying Azure Pipelines: |
4 participants
Commit Message: Currently FilterStateFormatter::formatValue is converting ProtobufTypes::Message to ProtobufWkt::Value through intermediate json string format, which is not efficient and may throw exception in data plane. Add serializeAsProtoValue virtual method to do the conversion directly.
Additional Description: NA
Risk Level: low
Testing: NA
Docs Changes: NA
Release Notes: NA