Repository navigation
brightstaff: add output_filter_mode: buffered for model-listener output filters - #1040
Open
ninadphalak wants to merge 1 commit into
Open
ninadphalak wants to merge 1 commit into
ninadphalak wants to merge 1 commit into
Conversation
…ut filters Output filters receive each upstream chunk on its own, so a value split across two chunks is never seen whole, and a filter error forwards the original chunk (katanemo#1036). output_filter_mode: buffered collects the whole upstream response, runs the filter chain once and returns the result. A stream error, a filter error or a body over 64 MiB withholds the body. The default stays streaming. Content-Length from the upstream is dropped when output filters run, since a filter can change the body length. Closes katanemo#1036
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1036.
What changes
A model listener gets an optional
output_filter_mode:streaming(default) is today's behaviour, unchanged: each upstream chunk goes through the filter chain on its own.bufferedcollects the whole upstream body, calls the filter chain once with it, and returns the result.This addresses both findings in #1036:
streamingmode a filter never sees text that spans a chunk boundary, soSECRET_+TOKEN.reaches the client unredacted. Inbufferedmode the filter getsThe token is SECRET_TOKEN.in one call.streamingmode a failed filter call forwards the original chunk. Inbufferedmode a filter error, an upstream stream error, or a body over 64 MiB withholds the body (the client gets the upstream status and headers with an empty body, and the error is logged and recorded on the span/metrics). This also covers thestream: falsecase from the issue, where the filter received gzip data in two pieces and could not decompress either.Why opt-in
Buffering costs streaming: the client sees nothing until the provider finishes. Filters that do not need whole values (logging, metrics, per-event transforms) should not pay that, so the default stays
streaming. A carry-over buffer would keep streaming, but the gateway does not know how long a filter's match can be, so that needs a filter-side protocol (the connection model in #834). This PR is the small, safe option in the meantime.Other changes
Content-Lengthfrom the upstream response is no longer copied when output filters are configured, since a filter can change the body length. This applies in both modes.config/plano_config_schema.yamlacceptsoutput_filter_mode(streaming|buffered); otherwiseplanoairejects the key.plano_config_full_reference*.yaml) and the model-listener demo README document the mode.Tests
streaming::output_filter_mode_tests(mockito filter, two-chunk upstreamThe token is SECRET_+TOKEN.):The token is [REDACTED].configuration::test::test_listener_output_filter_mode_deserialize:buffered, absent (defaults tostreaming), unknown value rejected.valid_listener_output_filter_mode_bufferedschema case (fails without the schema change).Run locally:
cargo fmt --all -- --check,cargo test -p brightstaff(250 passed, 2 ignored),cargo test -p common,pytest cli/test/test_config_generator.py(27 passed).cargo clippy --all-targets --all-features -- -D warningson Rust 1.99 reports onlyclippy::double_must_useatstate/mod.rs:64andsession_cache/mod.rs:175, identical onmain; with that lint allowed it is clean. I did not rebuild the Docker image to rerun the #1036 repro end to end.