Strip newline in error when reading from remote client #16487
Conversation
|
@krajorama since you reviewed the last one and have some context on this |
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
5bc93a8 to
329f3a1
Compare
| @@ -384,7 +384,8 @@ func (c *Client) Read(ctx context.Context, query *prompb.Query, sortSeries bool) | |||
| _ = httpResp.Body.Close() | |||
|
|
|||
| cancel() | |||
| err := errors.New(string(body)) | |||
| errStr := strings.Trim(string(body), "\n") | |||
There was a problem hiding this comment.
have you checked why we have this new line in the first place?
There was a problem hiding this comment.
It seems to be dependent on how the source / tests write the error response, e.g.
w.WriteHeader(http.StatusInternalServerError)
_, err = w.Write([]byte("execution error"))
require.NoError(t, err)
doesn't result in a newline but
http.Error(w, "execution error", http.StatusInternalServerError)
does
There was a problem hiding this comment.
it's in the standard library. This is from the http package. Note the fmt.Fprintln(w, error) at the end
// Error replies to the request with the specified error message and HTTP code.
// It does not otherwise end the request; the caller should ensure no further
// writes are done to w.
// The error message should be plain text.
//
// Error deletes the Content-Length header,
// sets Content-Type to “text/plain; charset=utf-8”,
// and sets X-Content-Type-Options to “nosniff”.
// This configures the header properly for the error message,
// in case the caller had set it up expecting a successful output.
func Error(w ResponseWriter, error string, code int) {
h := w.Header()
// Delete the Content-Length header, which might be for some other content.
// Assuming the error string fits in the writer's buffer, we'll figure
// out the correct Content-Length for it later.
//
// We don't delete Content-Encoding, because some middleware sets
// Content-Encoding: gzip and wraps the ResponseWriter to compress on-the-fly.
// See https://go.dev/issue/66343.
h.Del("Content-Length")
// There might be content type already set, but we reset it to
// text/plain for the error message.
h.Set("Content-Type", "text/plain; charset=utf-8")
h.Set("X-Content-Type-Options", "nosniff")
w.WriteHeader(code)
fmt.Fprintln(w, error)
}There was a problem hiding this comment.
Thanks I see, but now we assume the read endpoint is using http.Error (or any other util that adds a new line at the end, which isn't true for all endpoints), it's easier/safer to not make such assumptions / not twist the code to address some setups; just forward the error as-is.
Also, note that we're also trimming the leading new lines.
Let's keep it like this, we can revert if anyone complains about it.
Follow up to #16437 , which did not really change the string form of the error returned (just made the error form multilayered to allow it to be unwrapped), the newline was always there.
This PR will now actually change the error, by removing the newline which is probably unwanted in most cases and is typically absent from most errors. (Merging this PR isn't particularly important to me as the newline can be easily removed downstream, but it seems better to get rid of it at the source if possible if no one has any use for it.)
Failures in the windows tests might be transient