Add Content-Type header on query-frontend paths #1306
Conversation
wperron
requested review from
joe-elliott,
annanay25,
mdisibio,
dgzlopes,
mapno,
yvrhdn and
zalegrala
as code owners
There was a problem hiding this comment.
Thanks for taking a swing at this. "Content Negotation" is pretty rough in Tempo and I'm impressed with all the things you were able to find and correct.
I think the part that most concerns me is the way the the trace by id path works.
- frontend.go - force sets Accepts to protobuf
- tracebyidsharding.go - assumes that whatever it gets is protobuf b/c of something that happened in frontend.go
I know you didn't create this problem, but perhaps we could iron it out just a little bit? Thoughts on how we could clean this up?
| Header: http.Header{}, | ||
| StatusCode: http.StatusOK, | ||
| Header: http.Header{ | ||
| api.HeaderContentType: {api.HeaderAcceptProtobuf}, |
There was a problem hiding this comment.
it's weird that we assume protobuf here b/c of a header that was set in frontend.go.
There was a problem hiding this comment.
It's not, the Trace Sharder only ever sends a protobuf message, it's then unmarshalled and re-marshalled back into either protobuf or json in the frontend.go in the newTraceByIDMiddleware function here
There was a problem hiding this comment.
My bad, I meant it's weird we assume that the data received in the trace by id sharder (from the querier) is proto, but the only reason it is proto is because a header that's set in frontend.go.
I think we should set the Accept for proto in tracebyidsharding.go since that code also assumes that the returned data is proto. But these gross corners have been around for long before you made this improvement.
There was a problem hiding this comment.
Oh I see! I assumed this was intentional after reading the comment on that same line you linked, I guess it kinda make sense to keep content negotiation as close the edge as possible? Then again if internal communication is expected to be only protobuf, then setting the Accept header might just be superfluous 🤷
| @@ -193,6 +193,7 @@ func (q *Querier) SearchHandler(w http.ResponseWriter, r *http.Request) { | |||
| http.Error(w, err.Error(), http.StatusInternalServerError) | |||
| return | |||
| } | |||
| w.Header().Set(api.HeaderContentType, api.HeaderAcceptJSON) | |||
There was a problem hiding this comment.
set content type in TraceByIDHandler as well?
There was a problem hiding this comment.
Mhm, that's a good point 🤔 The TraceByID path is a bit odd because eventually it all reaches the newTraceByIdMiddleware in frontend.go where the payload is serialized according to the Accept header. I had to modify the tracebyidsharding.go because the tests weren't passing, but as it is, the tests are all passing. So while it would make sense to also add it to the TraceByIDHandler I also don't know if it'll make any practical difference.
There was a problem hiding this comment.
where the payload is serialized according to the Accept header
Sadly I think tracebyidsharding.go makes no checks and always assumes that the returned payload is proto. I don't expect a fix for that in this PR. It then uses the Accept header in frontend.go to determine how to marshal the response for the caller.
Since we are adding the Content-Type header to other responses here I would like to just slap it in the TraceByIDHandler as well, but don't feel obligated to adjust the frontend.
|
CI seems to be stuck? If you can resolve the changelog diff and push it will trigger it again. |
wperron
force-pushed
the
query-frontend-content-type
branch
from
f95442c to
3da55da
Compare
This commit adds the
Content-Typeheader on the read paths of the Query Frontend.Fixes #1185
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]