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

Support returning compressed responses from query-frontend #810

Closed
wants to merge 5 commits into from

Conversation

yvrhdn
Copy link
Member

@yvrhdn yvrhdn commented Jul 13, 2021

What this PR does:
The query-frontend will now gzip compress its response if the client requests this.

You can test this with curl by setting --compressed.

But... I thought this was already supported?

Despite what this config option might make you believe...

cfg.Config.CompressResponses = true

...Tempo does not compress responses yet

While updating Cortex to v1.9 (#808) I noticed this flag has been deprecated and removed. When I investigated how this was being used, I noticed it isn't part of the code Tempo uses.
https://github.com/cortexproject/cortex/blob/4afaa357469fb37fd92cb1e2a0c1d10cdddc5ecd/pkg/cortex/modules.go#L557-L559

@yvrhdn yvrhdn marked this pull request as draft July 13, 2021 17:24
@yvrhdn
Copy link
Member Author

yvrhdn commented Jul 13, 2021

So this works when I test it manually, but it doesn't in the e2e test. The HTTP server created by the Cortex e2e framework either doesn't support setting this header or filters it out somewhere...

@yvrhdn yvrhdn marked this pull request as ready for review July 13, 2021 18:54
@joe-elliott
Copy link
Member

I think the correct place to put functionality like this might be in weaveworks/common. @bboreham do you have any insight on this?

@bboreham
Copy link
Contributor

No strong feelings. If you put it in Server on by default it will help people who didn’t realise they needed it; it will be worse for people who know for sure they don’t want it (maybe they serve payloads that are always short, or always already compressed).

If it’s not on by default it won’t help much.

I note that the Go standard library http client will uncompress by default.

@joe-elliott
Copy link
Member

@kvrhdn it's fine with me to add it to just Tempo, but it does feel weird to just explicitly add this to the query frontend. I suppose all internal Tempo communication is through GRPC so maybe it is ok?

Can you investigate adding this when initializing the server so it impacts all components? Perhaps with one of these options:

https://github.com/weaveworks/common/blob/master/server/server.go#L79-L80

@yvrhdn
Copy link
Member Author

yvrhdn commented Jul 14, 2021

Sure, I'll see what is possible 🙂

fyi, Cortex moved this code into their api package and made it a helper function: https://github.com/cortexproject/cortex/blob/20f8931014bc9f3271380884f8eadee8de83ae96/pkg/api/api.go#L141-L143
That's also a possible solution.

@joe-elliott
Copy link
Member

The Cortex route probably has a lot of thought behind it. Perhaps some endpoints are better off not having this middleware?

Fine with either route. Pitch me something!

@yvrhdn
Copy link
Member Author

yvrhdn commented Aug 18, 2021

I'm working on a new PR at the moment.

@yvrhdn yvrhdn closed this Aug 18, 2021
@yvrhdn yvrhdn deleted the compression branch August 18, 2021 15:20
@yvrhdn yvrhdn mentioned this pull request Aug 18, 2021
3 tasks
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 this pull request may close these issues.

3 participants