mcp_transcoder: add response streaming option.#45076
Conversation
Signed-off-by: Yilin Guo <guoyilin@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/gemini review cc @paulhong01 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements streaming transcoding for unstructured text responses in the MCP JSON-REST bridge filter, allowing responses to be delivered in JSON-escaped chunks without full buffering. The implementation includes updates to the filter's header and data encoding logic, along with comprehensive unit and integration tests. Review feedback suggests adding error handling when locating the JSON-RPC envelope split point, enforcing the maximum response body size limit during streaming to prevent resource exhaustion, and optimizing the escaping process to minimize data copies.
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Thank you for the work! Let's align on the API first
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
|
/gemini review API LGTM, i am reviewing the implementation now |
There was a problem hiding this comment.
Code Review
This pull request introduces streaming transcoding for unstructured text responses within the MCP JSON-REST bridge filter, controlled by a new text_content_streaming_enabled configuration field. The implementation allows for JSON-escaping and wrapping response chunks on-the-fly, bypassing full body buffering. Review feedback suggests adding a safety check for the string search operation used to build the JSON-RPC envelope and evaluating whether a total response size limit should be enforced during streaming to mitigate potential resource exhaustion.
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
Thanks for contribution! Nice work @guoyilin42
API LGTM, and first pass on implementation
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
LGTM, module comments below.
Thanks!
Signed-off-by: Yilin Guo <guoyilin@google.com>
Signed-off-by: Yilin Guo <guoyilin@google.com>
| // output: [prefix+escaped_chunk1] → [escaped_chunk2] → [escaped_chunk3+suffix] | ||
| // | ||
| // Disabled by default. | ||
| bool text_content_streaming_enabled = 4; |
There was a problem hiding this comment.
Would it make sense to use google.protobuf.BoolValue here, so that data planes can have this be enabled by default if they want to? Or do we expect that the data plane would always want to have this disabled by default?
There was a problem hiding this comment.
We had a discussion before with @tyxia and thought we'd like to have this disabled by default for future structured content support which needs buffering anyway. Also could you elaborate more about how would google.protobuf.BoolValue make it enabled by default?
There was a problem hiding this comment.
Mark meant that: BoolValue is wrapper message for bool can be set to True by default in the future, while bool is only false by default.
For this case, we won't need to set it to True by default even in the future, so bool is fine here.
| // buffering the full response body. | ||
| if (!streaming_json_prefix_.empty()) { | ||
| uint64_t len = data.length(); | ||
| // Note: An empty chunk can arrive when the upstream uses the body + trailer pattern (end_stream |
There was a problem hiding this comment.
You probably meant a TODO, as encodeTrailer is not handled yet.
No need to address this in this PR.
|
/lgtm api |
|
/retest |
Commit Message: mcp_transcoder: add response streaming option. Additional Description: This PR adds a `text_content_streaming_enabled` configuration option to the `mcp_json_rest_bridge` HTTP filter. Currently, the transcoder buffers the entire response before sending it to the client. With this option enabled, the filter supports streaming unstructured text responses directly to the client chunk-by-chunk. Implementation details: * In `encodeHeaders`, the filter strips the `Content-Length` header and pre-builds a JSON-RPC envelope (prefix and suffix). * In `encodeData`, it JSON-escapes each arriving text chunk on-the-fly and streams it to the client wrapped inside the pre-built JSON-RPC envelope. * Added comprehensive unit and integration tests to verify streaming behavior, error handling, and JSON escaping of special characters. Risk Level: Low Testing: Unit and Integration tests. Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Yilin Guo <guoyilin@google.com>
4 participants
Commit Message: mcp_transcoder: add response streaming option.
Additional Description:
This PR adds a
text_content_streaming_enabledconfiguration option to themcp_json_rest_bridgeHTTP filter.Currently, the transcoder buffers the entire response before sending it to the client. With this option enabled, the filter supports streaming unstructured text responses directly to the client chunk-by-chunk.
Implementation details:
encodeHeaders, the filter strips theContent-Lengthheader and pre-builds a JSON-RPC envelope (prefix and suffix).encodeData, it JSON-escapes each arriving text chunk on-the-fly and streams it to the client wrapped inside the pre-built JSON-RPC envelope.Risk Level: Low
Testing: Unit and Integration tests.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A