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.

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

Endpoints are missing a content-type header #1185

Closed
wperron opened this issue Dec 21, 2021 · 3 comments · Fixed by #1306
Closed

Endpoints are missing a content-type header #1185

wperron opened this issue Dec 21, 2021 · 3 comments · Fixed by #1306

Comments

@wperron
Copy link
Contributor

wperron commented Dec 21, 2021

Some of the API endpoints are returning a content-type of text/plain even if the Accept header on the request is set to application/json or application/protobuf. I noticed it on /api/traces/{traceID} and /api/search at least, there could be others as well.

@kvrhdn
Copy link
Member

kvrhdn commented Dec 21, 2021

Thanks for creating this! The way Tempo deals with content encoding is a bit funky because we convert everything to protobuf internally. So the response coming from the querier will be in protobuf and the query-frontend is responsible for converting it into JSON if the user requested this.
It doesn't surprise me we forgot to set some headers along the way 😅

Some pointers about the code:

Trace lookup

This is where we determine the response content encoding:
https://github.com/grafana/tempo/blob/main/modules/frontend/frontend.go#L109-L113

And this is where we set the response body, we should probably set the content-type here as well:
https://github.com/grafana/tempo/blob/main/modules/frontend/frontend.go#L137-L151

Search

The search endpoint is a bit simpler because we only return search results in JSON.

We probably should set the content-type somewhere here: https://github.com/grafana/tempo/blob/main/modules/frontend/frontend.go#L163-L184

Which makes me think... the querier and ingester have the same/similar endpoints so we should also set this header there.

@wperron
Copy link
Contributor Author

wperron commented Dec 21, 2021

Which makes me think... the querier and ingester have the same/similar endpoints so we should also set this header there.

Probably won't hurt for sure, but as you said, everything is done over protobuf internally, so it would still require setting the content-type in the query frontend based on the accept header yeah?

@kvrhdn
Copy link
Member

kvrhdn commented Dec 22, 2021

Yeah. that's true. I was thinking of a scenario in which the user requests protobuf, in that case we don't have to do any conversion in the query-frontend. But we actually still have to manipulate the body a little bit to return the correct format so we never forward the request as-is.

wperron added a commit to wperron/tempo that referenced this issue Feb 23, 2022
wperron added a commit to wperron/tempo that referenced this issue Feb 24, 2022
joe-elliott pushed a commit that referenced this issue Feb 24, 2022
* Add Content-Type header on query-frontend paths

Fixes #1185

* Update CHANGELOG

* Add Content-Type header to TraceByIDHandler in querier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants