http/transport tap: Support PROTO_TEXT format in UDP sink #38485
Conversation
|
looks good |
Signed-off-by: fchen7 <[email protected]>
| @@ -46,21 +46,26 @@ void UdpTapSink::UdpTapSinkHandle::submitTrace(TapCommon::TraceWrapperPtr&& trac | |||
| case envoy::config::tap::v3::OutputSink::PROTO_BINARY: | |||
| FALLTHRU; | |||
| case envoy::config::tap::v3::OutputSink::PROTO_BINARY_LENGTH_DELIMITED: | |||
| FALLTHRU; | |||
| case envoy::config::tap::v3::OutputSink::PROTO_TEXT: | |||
| // will implement above format if it is needed. | |||
| ENVOY_LOG_MISC(debug, | |||
| "{}: Not support PROTO_BINARY, PROTO_BINARY_LENGTH_DELIMITED, PROTO_TEXT", | |||
There was a problem hiding this comment.
Error message will need to be updated.
There was a problem hiding this comment.
Thanks! PROTO_TEXT is removed
| case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_BYTES: | ||
| FALLTHRU; | ||
| case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_STRING: { | ||
| if (!parent_.isUdpPacketWriterCreated()) { | ||
| ENVOY_LOG_MISC(debug, "{}: udp writter isn't created yet", __func__); | ||
| break; | ||
| } | ||
| std::string json_string = MessageUtil::getJsonStringFromMessageOrError(*trace, true, false); | ||
| std::string json_string; | ||
| if (format == envoy::config::tap::v3::OutputSink::PROTO_TEXT) { |
There was a problem hiding this comment.
nit: this if statement implies that a switch is probably the right choice here.
As this is a contrib extension, this is a suggestion rather than a request to refactor.
There was a problem hiding this comment.
This is a good suggestion, I have think about this before, see below code:
53 case envoy::config::tap::v3::OutputSink::PROTO_TEXT:
54 FALLTHRU;
55 case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_BYTES:
56 FALLTHRU;
57 case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_STRING: {
58 if (!parent_.isUdpPacketWriterCreated()) {
59 ENVOY_LOG_MISC(debug, "{}: udp writter isn't created yet", __func__);
60 break;
61 }
62 std::string json_string;
63 if (format == envoy::config::tap::v3::OutputSink::PROTO_TEXT) {
64 json_string = MessageUtil::toTextProto(*trace);
65 } else {
66 json_string = MessageUtil::getJsonStringFromMessageOrError(*trace, true, false);
67 }
68 Buffer::OwnedImpl udp_data(std::move(json_string));
69 Api::IoCallUint64Result write_result =
70 parent_.udp_packet_writer_->writePacket(udp_data, nullptr, *parent_.udp_server_address_);
71 if (!write_result.ok()) {
72 ENVOY_LOG_MISC(debug, "{}: Failed to send UDP packet!", __func__);
73 }
if using different switch, then line 58-61, 68-73 will be repeated. Other good suggestion is to write a function.
thanks again!
There was a problem hiding this comment.
I would probably not use a switch in this case, but convert to:
if (Non-supported-types) {
...
}
ASSERT(supported types);
//Handle supported types.
There was a problem hiding this comment.
Hi @adisuissa,
Very nice suggestion! the major reason that I keep switch is try to keep the same style as original tap code.
Appreciated your suggestion, I will try this in next code.
thanks
Cliff
Signed-off-by: fchen7 <[email protected]>
xiexie, Test is added! |
| @@ -193,6 +190,29 @@ TEST_F(UdpTapSinkTest, TestSubmitTraceSendNotOk) { | |||
| envoy::config::tap::v3::OutputSink::JSON_BODY_AS_STRING); | |||
| } | |||
|
|
|||
| TEST_F(UdpTapSinkTest, TestSubmitTraceSendforPROTOTEXTOk) { | |||
There was a problem hiding this comment.
Please change test name to use a consistent style.
There was a problem hiding this comment.
Good suggestion! Changed
Signed-off-by: fchen7 <[email protected]>
Commit Message: Support PROTO_TEXT format in UDP sink
Additional Description:
New request from custom, it needs raw format http2 message.
if use format JSON_BODY_AS_BYTES, then the http2 message is encoded as base64, then server side has to base64 decode to get raw http2 message.
if use format PROTO_TEXT, then server side get raw http2 message which doesn't need run base64 decode to save CPU time
Based above reason, add format PROTO_TEXT in UDP sink to meet raw http2 message needs.
if it set JSON_
Risk Level:low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]