[bbr] Skip body re-serialization when unchanged#2556
[bbr] Skip body re-serialization when unchanged#2556k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @asaadbalum. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5ae56cc to
bea9ce8
Compare
bea9ce8 to
0be4cf4
Compare
nirrozenbaum
left a comment
There was a problem hiding this comment.
overall lgtm
left few minor nits
| } | ||
| } | ||
|
|
||
| func (r *InferenceMessage) BodyWasMutated() bool { |
There was a problem hiding this comment.
nit: rename to BodyMutated()
| func (r *InferenceMessage) BodyWasMutated() bool { | |
| func (r *InferenceMessage) BodyMutated() bool { |
| } | ||
|
|
||
| // Necessary so that the new headers are used in the routing decision. | ||
| resp := &eppb.CommonResponse{ |
There was a problem hiding this comment.
nit: can we call response?
| bodyMutated := reqCtx.Request.BodyWasMutated() | ||
| var mutatedBodyBytes []byte | ||
| if bodyMutated { | ||
| b, err := json.Marshal(reqCtx.Request.Body) |
There was a problem hiding this comment.
why do we need b? can't we marshal directly to mutatedBodyBytes?
| bodyMutated := reqCtx.Response.BodyWasMutated() | ||
| var mutatedBytes []byte | ||
| if bodyMutated { | ||
| b, err := json.Marshal(reqCtx.Response.Body) |
| return ret, nil | ||
| } | ||
|
|
||
| resp := &eppb.CommonResponse{ |
0be4cf4 to
b106e2b
Compare
|
/ok-to-test |
| r.bodyMutated = true | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we need here additional function to set the whole body.
something like:
func (r *InferenceMessage) SetBody(body map[string]any) {
r.Body = body
r.bodyMutated = true
}this might be useful for plugins that transform the body in some way (e.g., api translation from OpenAI to Anthropic) and want to set the whole body at once.
There was a problem hiding this comment.
Agree, sending a patch in a min
Add body mutation tracking to InferenceMessage via SetBody, SetBodyField, RemoveBodyField, and BodyMutated so the request and response handlers skip json.Marshal and Content-Length update when no plugin modified the body. SetBody replaces the entire body map for plugins that transform the full payload (e.g., API translation). SetBodyField and RemoveBodyField handle granular field-level mutations. For unary mode, omit BodyMutation from the ext_proc response when unchanged. For streaming mode, forward original bytes instead of re-marshaling. Add integration tests that verify body mutation tracking over real gRPC for both unary and streaming paths. Signed-off-by: Asaad Balum <asaad.balum@gmail.com>
b106e2b to
b60e5b8
Compare
nirrozenbaum
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thanks!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asaadbalum, nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-gateway-api-inference-extension-test-e2e-main |
…tes-sigs#2556) Add body mutation tracking to InferenceMessage via SetBody, SetBodyField, RemoveBodyField, and BodyMutated so the request and response handlers skip json.Marshal and Content-Length update when no plugin modified the body. SetBody replaces the entire body map for plugins that transform the full payload (e.g., API translation). SetBodyField and RemoveBodyField handle granular field-level mutations. For unary mode, omit BodyMutation from the ext_proc response when unchanged. For streaming mode, forward original bytes instead of re-marshaling. Add integration tests that verify body mutation tracking over real gRPC for both unary and streaming paths. Signed-off-by: Asaad Balum <asaad.balum@gmail.com>
…tes-sigs/gateway-api-inference-extension#2556) Add body mutation tracking to InferenceMessage via SetBody, SetBodyField, RemoveBodyField, and BodyMutated so the request and response handlers skip json.Marshal and Content-Length update when no plugin modified the body. SetBody replaces the entire body map for plugins that transform the full payload (e.g., API translation). SetBodyField and RemoveBodyField handle granular field-level mutations. For unary mode, omit BodyMutation from the ext_proc response when unchanged. For streaming mode, forward original bytes instead of re-marshaling. Add integration tests that verify body mutation tracking over real gRPC for both unary and streaming paths. Signed-off-by: Asaad Balum <asaad.balum@gmail.com>
Summary
InferenceMessageviaSetBody,SetBodyField,RemoveBodyField, andBodyMutatedso the request and response handlers skipjson.MarshalandContent-Lengthupdate when no plugin modified the bodyrequest.go) and response (response.go) paths: for unary, omitBodyMutationwhen unchanged; for streaming, forward original bytes when unchangedThis is a follow-up to #2551, which added request body mutation support but always re-marshaled regardless of whether plugins modified the body.
What type of PR is this?
/kind feature
What's changed:
pkg/bbr/framework/types.gobodyMutatedflag;SetBodyreplaces entire body;SetBodyField/RemoveBodyFieldfor field-level mutations;BodyMutatedaccessorpkg/bbr/framework/types_test.goSetBody,SetBodyField,RemoveBodyField,BodyMutated)pkg/bbr/handlers/request.goBodyMutationonly when body changedpkg/bbr/handlers/response.gopkg/bbr/handlers/request_test.gopkg/bbr/handlers/response_test.gopkg/bbr/handlers/server_test.goContent-Length/BodyMutationexpectationstest/integration/bbr/harness.goNewBBRHarnessWithPluginsfor custom plugin injectiontest/integration/bbr/body_mutation_test.gotest/integration/bbr/util.gopromptparametertest/integration/bbr/hermetic_test.goDoes this PR introduce a user-facing change?
Users writing BBR plugins that do NOT modify the request/response body will see reduced overhead — no unnecessary JSON marshaling or
Content-Lengthrewriting. Plugins that modify the body should useSetBody(full replacement),SetBodyField, orRemoveBodyFieldto ensure the dirty flag is set.