add aws_eventstream_parser filter for aws stream parsing#44094
add aws_eventstream_parser filter for aws stream parsing#44094cqi1217 wants to merge 15 commits into
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
What's the difference between this PR and the sse_to_metadata? And which maintainer will sponsor this PR? Note, any new extension will require a maintainer to sponsor. See https://github.com/envoyproxy/envoy/blob/main/EXTENSION_POLICY.md |
|
@wbpcode the sse_to_metadata filter parse the standard SSE response, which is used by openAI, vertexAI and others. The aws_eventstream_to_metadata parse the AWS eventstream response, whicih is used by AWS bedrock. The maintainer could be @JuniorHsu @PeterL328 |
It needs maintainer on the top two lists Hello @tyxia, this is another AI related parser in HTTP filter chain. I noticed that you show great interest in SSE filter. Are we able to get your sponsor here so we could share the filter to open source? Peter and/or I could do the first pass to reduce the review effort. |
Hi @JuniorHsu , thanks for reaching out. Yes I am happy to sponsor here . I will take a closer look at PR/proposal later today |
|
seems ci fails due to some format checks, I'll take a look later today |
|
/retest |
|
looks like test coverage is low, I'll fix it |
|
If possible, I will to see the possibility to merge this to see_to_metadata :) |
|
@wbpcode |
Sorry for delay here. Thank you for the contribution! From high level design point view. I am hoping we don't lock in on metadata: With that said, i am think we can give this filter a bit generic name like AWS_eventstream_parser_filter and sse_parser_filter. This opens the flexibility and possibility for future development. What do you guys think? |
|
I agree with tianyu for a general name. metadata is just so widely and easily supported, and it should be fine if we only care about some attributes. |
|
aws_eventstream_parser seems reasonable for me, but sse_to_metadata filter is merged already, not sure how hard to rename it. @PeterL328 what do you think? |
With the power of AI, i would think renaming should be straightforward :) IMHO, given the status of sse_to_metadata (just merged and in alpha status) renaming should be fine. I actually left similar comment w.r.t. metadata vs filter state on that PR. @PeterL328 please share your thoughts here as well. In the meantime, i will kick off the review with Gemini :) let see how it works |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the aws_eventstream_to_metadata HTTP filter, which extracts values from AWS EventStream response bodies—commonly used by AWS Bedrock—and writes them to dynamic metadata. The implementation features a pluggable content parser architecture, currently supporting JSON, and includes comprehensive documentation and tests. Feedback suggests optimizing buffer handling by avoiding repeated linearization of the data buffer and adding a dedicated counter for metadata type conversion failures to improve observability.
|
one filter to do both is better. Those are couple lines of code that should result in two filters. You can use config to control whether to write to metadata or filter state |
|
@tyxia @cqi1217 |
|
/retest |
There was a problem hiding this comment.
Code Review
This pull request introduces the aws_eventstream_parser HTTP filter, which extracts values from AWS EventStream response bodies and headers into dynamic metadata. The implementation includes a pluggable content parser architecture, comprehensive documentation, and extensive unit and integration tests. Feedback highlights the need for a default case in the header value conversion logic to improve robustness and notes potential precision loss when mapping int64_t values to double in metadata.
| Protobuf::Value Filter::headerValueToProtobufValue( | ||
| const Extensions::Common::Aws::Eventstream::HeaderValue& header_value) { | ||
| Protobuf::Value pb_value; | ||
|
|
||
| switch (header_value.type) { | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::BoolTrue: | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::BoolFalse: | ||
| pb_value.set_bool_value(absl::get<bool>(header_value.value)); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Byte: | ||
| pb_value.set_number_value(static_cast<double>(absl::get<int8_t>(header_value.value))); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Short: | ||
| pb_value.set_number_value(static_cast<double>(absl::get<int16_t>(header_value.value))); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Int32: | ||
| pb_value.set_number_value(static_cast<double>(absl::get<int32_t>(header_value.value))); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Int64: | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Timestamp: | ||
| pb_value.set_number_value(static_cast<double>(absl::get<int64_t>(header_value.value))); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::String: | ||
| pb_value.set_string_value(absl::get<std::string>(header_value.value)); | ||
| break; | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::ByteArray: { | ||
| const auto& bytes = absl::get<std::string>(header_value.value); | ||
| pb_value.set_string_value( | ||
| Hex::encode(reinterpret_cast<const uint8_t*>(bytes.data()), bytes.size())); | ||
| break; | ||
| } | ||
| case Extensions::Common::Aws::Eventstream::HeaderValueType::Uuid: { | ||
| const auto& uuid = absl::get<std::array<uint8_t, 16>>(header_value.value); | ||
| pb_value.set_string_value( | ||
| fmt::format("{:02x}{:02x}{:02x}{:02x}-{:02x}{:02x}-{:02x}{:02x}-{:02x}{:02x}-" | ||
| "{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", | ||
| uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5], uuid[6], uuid[7], uuid[8], | ||
| uuid[9], uuid[10], uuid[11], uuid[12], uuid[13], uuid[14], uuid[15])); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return pb_value; | ||
| } |
There was a problem hiding this comment.
The switch statement in headerValueToProtobufValue does not have a default case. While it currently covers all types defined in the EventstreamParser, adding a default case would make the code more robust against future additions to the HeaderValueType enum. When implementing this error-handling block, use IS_ENVOY_BUG to report the unexpected state.
References
- When reporting a bug within an explicit error-handling block in CPP, use IS_ENVOY_BUG instead of ENVOY_BUG(false, ...).
| // | ||
| // When all header rules AND all content parser rules have stop_processing_after_matches | ||
| // limits set and all limits are satisfied, the filter stops processing the stream entirely. | ||
| uint32 stop_processing_after_matches = 4 [(validate.rules).uint32 = {lte: 1}]; |
There was a problem hiding this comment.
Consider using google.protobuf.UInt32Value (or google.protobuf.BoolValue if you take my suggestion above) to allow individual data plane implementations to have their own defaults.
There was a problem hiding this comment.
google.protobuf.UInt32Value makes sense
|
/lgtm api |
|
/retest |
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
…ta_value in the header rule Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
…_after_matches Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
Signed-off-by: cqi1217 <caiqi1217@gmail.com>
|
PTAL @tyxia |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the aws_eventstream_parser HTTP filter to Envoy, which extracts values from AWS EventStream response bodies (such as AWS Bedrock streaming responses) and writes them to dynamic metadata. The review feedback highlights a critical bug in writeMetadata() where dynamic metadata is completely overwritten rather than merged, which could wipe out other filters' metadata. Additionally, the feedback suggests a performance optimization to avoid double JSON parsing in unwrapBedrockEnvelope by performing a quick substring check, and recommends replacing Protobuf Map's contains() with count() > 0 to ensure backward compatibility with older Protobuf versions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| void Filter::writeMetadata() { | ||
| for (const auto& entry : structs_by_namespace_) { | ||
| encoder_callbacks_->streamInfo().setDynamicMetadata(entry.first, entry.second); | ||
| } | ||
| structs_by_namespace_.clear(); | ||
| } |
There was a problem hiding this comment.
writeMetadata() is called at the end of encodeData for every chunk. It calls setDynamicMetadata(entry.first, entry.second) for each namespace. However, setDynamicMetadata completely overwrites the existing metadata struct for that namespace. Since structs_by_namespace_ is cleared at the end of writeMetadata(), any subsequent chunk or other filters' metadata in the same namespace will be completely wiped out! To prevent this, we must retrieve the existing metadata struct from StreamInfo, merge our new fields into it, and then call setDynamicMetadata with the merged struct.
void Filter::writeMetadata() {
for (const auto& entry : structs_by_namespace_) {
const std::string& ns = entry.first;
const Protobuf::Struct& new_struct = entry.second;
// Get existing metadata for this namespace to avoid overwriting other keys.
Protobuf::Struct merged_struct;
const auto& filter_metadata =
encoder_callbacks_->streamInfo().dynamicMetadata().filter_metadata();
const auto existing_it = filter_metadata.find(ns);
if (existing_it != filter_metadata.end()) {
merged_struct = existing_it->second;
}
// Merge new fields into the existing struct.
for (const auto& [key, value] : new_struct.fields()) {
(*merged_struct.mutable_fields())[key] = value;
}
encoder_callbacks_->streamInfo().setDynamicMetadata(ns, merged_struct);
}
structs_by_namespace_.clear();
}| absl::optional<std::string> Filter::unwrapBedrockEnvelope(absl::string_view payload) { | ||
| auto json_or = Json::Factory::loadFromString(std::string(payload)); | ||
| if (!json_or.ok()) { | ||
| return absl::nullopt; | ||
| } |
There was a problem hiding this comment.
For every single message in the EventStream, unwrapBedrockEnvelope parses the payload as JSON to check if it is a Bedrock envelope. If it is not (which is the case for normal JSON payloads or other EventStream messages), it is parsed again by the JSON content parser. This results in double JSON parsing for every message, which is highly inefficient. We can add a quick check using absl::StrContains to avoid parsing the JSON if it doesn't even contain the "bytes" key.
| absl::optional<std::string> Filter::unwrapBedrockEnvelope(absl::string_view payload) { | |
| auto json_or = Json::Factory::loadFromString(std::string(payload)); | |
| if (!json_or.ok()) { | |
| return absl::nullopt; | |
| } | |
| absl::optional<std::string> Filter::unwrapBedrockEnvelope(absl::string_view payload) { | |
| // Quick check to avoid expensive JSON parsing for payloads that are not Bedrock envelopes. | |
| if (!absl::StrContains(payload, "\"bytes\"")) { | |
| return absl::nullopt; | |
| } | |
| auto json_or = Json::Factory::loadFromString(std::string(payload)); | |
| if (!json_or.ok()) { | |
| return absl::nullopt; | |
| } |
| const auto pending_it = structs_by_namespace_.find(namespace_str); | ||
| if (pending_it != structs_by_namespace_.end() && | ||
| pending_it->second.fields().contains(action.key)) { |
There was a problem hiding this comment.
The code uses contains() on google::protobuf::Map (via fields().contains(...)). However, contains() is only supported in newer versions of Protobuf (v22.0+). To ensure backward compatibility with older Protobuf versions that Envoy might be compiled with, we should use count(...) > 0 instead.
| const auto pending_it = structs_by_namespace_.find(namespace_str); | |
| if (pending_it != structs_by_namespace_.end() && | |
| pending_it->second.fields().contains(action.key)) { | |
| const auto pending_it = structs_by_namespace_.find(namespace_str); | |
| if (pending_it != structs_by_namespace_.end() && | |
| pending_it->second.fields().count(action.key) > 0) { |
| const auto& filter_metadata = | ||
| encoder_callbacks_->streamInfo().dynamicMetadata().filter_metadata(); | ||
| const auto entry_it = filter_metadata.find(namespace_str); | ||
| if (entry_it != filter_metadata.end() && entry_it->second.fields().contains(action.key)) { |
There was a problem hiding this comment.
The code uses contains() on google::protobuf::Map (via fields().contains(...)). However, contains() is only supported in newer versions of Protobuf (v22.0+). To ensure backward compatibility with older Protobuf versions that Envoy might be compiled with, we should use count(...) > 0 instead.
| const auto& filter_metadata = | |
| encoder_callbacks_->streamInfo().dynamicMetadata().filter_metadata(); | |
| const auto entry_it = filter_metadata.find(namespace_str); | |
| if (entry_it != filter_metadata.end() && entry_it->second.fields().contains(action.key)) { | |
| const auto& filter_metadata = | |
| encoder_callbacks_->streamInfo().dynamicMetadata().filter_metadata(); | |
| const auto entry_it = filter_metadata.find(namespace_str); | |
| if (entry_it != filter_metadata.end() && entry_it->second.fields().count(action.key) > 0) { |
|
on-call ping @tyxia for the review. |
| bool content_type_matched_{false}; | ||
| // Set to true when all rules have reached their match limits. Stops further processing. | ||
| bool processing_complete_{false}; | ||
| Buffer::OwnedImpl buffer_; |
There was a problem hiding this comment.
This is free buffer instance that is unbounded? We should apply the configurable buffer limit.
| } | ||
|
|
||
| writeMetadata(); | ||
| return Http::FilterDataStatus::Continue; |
There was a problem hiding this comment.
In Incomplete message - need more data case, the parsing is not completed but we will send forward the data to downstream client. Is this expected/designed behavior? Should we return something like stopIterationNoBuffer
|
Waiting on fixes from the author /wait |
|
will work on it early next week |
Sounds good Thank you for your contribution! |
Commit Message: Add aws_eventstream_to_metadata filter
Additional Description:
This PR is adding a new HTTP filter for extracting values from AWS eventstream responses and writing them into dynamic metadata. This filter is similar to sse_to_metadata filter(#42762).
Key Architecture
Filter Layer (envoy.filters.http.sse_to_metadata): Handles SSE protocol parsing(aws: Adds extension common util for parsing AWS eventstream #43544), buffering, and metadata writing
Parser Interface (envoy.content_parser): Generic interface for parsing SSE event data fields
JSON Parser (envoy.content_parsers.json): First implementation supporting JSON path-based extraction with on_present/on_missing/on_error semantics
Risk Level: Low (new http filter)
Testing: unit tests and integration tests
Docs Changes: Added documentation for aws_eventstream_to_matadata filter
Release Notes:
Fixes #43492