Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Content-Type header on query-frontend paths #1306

Merged
merged 3 commits into from
Feb 24, 2022
Merged

Add Content-Type header on query-frontend paths #1306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b704f36
Add Content-Type header on query-frontend paths
wperron Feb 23, 2022
5af4c1c
Update CHANGELOG
wperron Feb 24, 2022
3da55da
Add Content-Type header to TraceByIDHandler in querier
wperron Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* [ENHANCEMENT] Add new scaling alerts to the tempo-mixin [#1292](https://github.com/grafana/tempo/pull/1292) (@mapno)
* [ENHANCEMENT] Improve serverless handler error messages [#1305](https://github.com/grafana/tempo/pull/1305) (@joe-elliott)
* [ENHANCEMENT] Make trace combination/compaction more efficient [#1291](https://github.com/grafana/tempo/pull/1291) (@mdisibio)
* [ENHANCEMENT] Add Content-Type headers to query-frontend paths [#1306](https://github.com/grafana/tempo/pull/1306) (@wperron)
* [BUGFIX]: Remove unnecessary PersistentVolumeClaim [#1245](https://github.com/grafana/tempo/issues/1245)
* [BUGFIX] Fixed issue when query-frontend doesn't log request details when request is cancelled [#1136](https://github.com/grafana/tempo/issues/1136) (@adityapwr)
* [BUGFIX] Update OTLP port in examples (docker-compose & kubernetes) from legacy ports (55680/55681) to new ports (4317/4318) [#1294](https://github.com/grafana/tempo/pull/1294) (@mapno)
Expand Down
2 changes: 2 additions & 0 deletions modules/frontend/frontend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ func newTraceByIDMiddleware(cfg Config, logger log.Logger) Middleware {
span.SetTag("contentType", marshallingFormat)
}

resp.Header.Set(api.HeaderContentType, marshallingFormat)

return resp, err
})
})
Expand Down
11 changes: 10 additions & 1 deletion modules/frontend/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ func (f *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
return
}

// write header and body
// write headers, status code and body
copyHeader(w.Header(), resp.Header)
w.WriteHeader(resp.StatusCode)
if resp.Body != nil {
_, _ = io.Copy(w, resp.Body)
Expand Down Expand Up @@ -126,6 +127,14 @@ func (f *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
)
}

func copyHeader(dst, src http.Header) {
for k, vv := range src {
for _, v := range vv {
dst.Add(k, v)
}
}
}

// writeError handles writing errors to the http.ResponseWriter. It uses weavework common
// server.WriteError() to handle httpgrc errors. The handler handles all incoming HTTP requests
// to the query frontend which then distributes them via httpgrpc to the queriers. As a result
Expand Down
6 changes: 4 additions & 2 deletions modules/frontend/searchsharding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,10 @@ func (s searchSharder) RoundTrip(r *http.Request) (*http.Response, error) {
}

return &http.Response{
StatusCode: http.StatusOK,
Header: http.Header{},
StatusCode: http.StatusOK,
Header: http.Header{
api.HeaderContentType: {api.HeaderAcceptJSON},
},
Body: io.NopCloser(strings.NewReader(bodyString)),
ContentLength: int64(len([]byte(bodyString))),
}, nil
Expand Down
3 changes: 3 additions & 0 deletions modules/frontend/searchsharding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,9 @@ func TestSearchSharderRoundTrip(t *testing.T) {
}
require.NoError(t, err)
assert.Equal(t, tc.expectedStatus, resp.StatusCode)
if tc.expectedStatus == http.StatusOK {
assert.Equal(t, "application/json", resp.Header.Get("Content-Type"))
}
if tc.expectedResponse != nil {
actualResp := &tempopb.SearchResponse{}
bytesResp, err := io.ReadAll(resp.Body)
Expand Down
7 changes: 5 additions & 2 deletions modules/frontend/tracebyidsharding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/go-kit/log/level"
"github.com/golang/protobuf/proto"
"github.com/grafana/tempo/modules/querier"
"github.com/grafana/tempo/pkg/api"
"github.com/grafana/tempo/pkg/model/trace"
"github.com/grafana/tempo/pkg/tempopb"
"github.com/opentracing/opentracing-go"
Expand Down Expand Up @@ -177,8 +178,10 @@ func (s shardQuery) RoundTrip(r *http.Request) (*http.Response, error) {
}

return &http.Response{
StatusCode: http.StatusOK,
Header: http.Header{},
StatusCode: http.StatusOK,
Header: http.Header{
api.HeaderContentType: {api.HeaderAcceptProtobuf},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's weird that we assume protobuf here b/c of a header that was set in frontend.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

},
Body: io.NopCloser(bytes.NewReader(buff)),
ContentLength: int64(len(buff)),
}, nil
Expand Down
3 changes: 3 additions & 0 deletions modules/frontend/tracebyidsharding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,9 @@ func TestShardingWareDoRequest(t *testing.T) {
}
require.NoError(t, err)
assert.Equal(t, tc.expectedStatus, resp.StatusCode)
if tc.expectedStatus == http.StatusOK {
assert.Equal(t, "application/protobuf", resp.Header.Get("Content-Type"))
}
if tc.expectedTrace != nil {
actualResp := &tempopb.TraceByIDResponse{}
bytesTrace, err := io.ReadAll(resp.Body)
Expand Down
5 changes: 5 additions & 0 deletions modules/querier/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func (q *Querier) TraceByIDHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
w.Header().Set(api.HeaderContentType, api.HeaderAcceptProtobuf)
_, err = w.Write(b)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
Expand All @@ -94,6 +95,7 @@ func (q *Querier) TraceByIDHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
w.Header().Set(api.HeaderContentType, api.HeaderAcceptJSON)
}

// return values are (blockStart, blockEnd, queryMode, error)
Expand Down Expand Up @@ -193,6 +195,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set content type in TraceByIDHandler as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@joe-elliott joe-elliott Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

}

func (q *Querier) SearchTagsHandler(w http.ResponseWriter, r *http.Request) {
Expand All @@ -217,6 +220,7 @@ func (q *Querier) SearchTagsHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
w.Header().Set(api.HeaderContentType, api.HeaderAcceptJSON)
}

func (q *Querier) SearchTagValuesHandler(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -249,4 +253,5 @@ func (q *Querier) SearchTagValuesHandler(w http.ResponseWriter, r *http.Request)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
w.Header().Set(api.HeaderContentType, api.HeaderAcceptJSON)
}
1 change: 1 addition & 0 deletions pkg/api/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const (
urlParamVersion = "version"

HeaderAccept = "Accept"
HeaderContentType = "Content-Type"
HeaderAcceptProtobuf = "application/protobuf"
HeaderAcceptJSON = "application/json"

Expand Down