adding logging & support for better Client response #847
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| logger.V(logutil.DEFAULT).Error(err, "Error unmarshaling request body") | ||
| // TODO: short circuit and send the body back as is (this could be an envoy error), currently we drop | ||
| // whatever the body request would have been and send our immediate response instead. | ||
| err = errutil.Error{Code: errutil.BadRequest, Msg: "Error unmarshaling request body: " + string(body)} |
There was a problem hiding this comment.
wouldn't adding the body make the message too large?
| }, | ||
| }, | ||
| } | ||
| break |
There was a problem hiding this comment.
What was the behavior before doing this short circuit?
There was a problem hiding this comment.
I am asking because we hit an issue when running large scale benchmark with model server streaming; surprisingly some requests were hitting this code path and so causing unmarshling errors. My theory was that for some reason the response header events were not always being sent before the response body events, and so the modelServerStreaming flag was being set late for some requests.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
3 participants
When requests/responses are not valid json, EPP clobbered any body message. This should improve UX & useful messaging in the face of failures.