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

add gzip brotli compression to http responses (close #2674) #2751

Merged
merged 30 commits into from Sep 19, 2019

Conversation

rakeshkky
Copy link
Member

@rakeshkky rakeshkky commented Aug 19, 2019

Description

The server compresses the responses (success only) from /v1/query and /v1/graphql endpoints. The server chooses either brotli or gzip based on Accept-Encoding client header. If both are requested then the server picks brotli.

Affected components

  • Server
  • Docs
  • Build System (tests)
  • Tests

Related Issues

close #2674

Solution and Design

  • Add a new Hasura.Server.Compression module which exposes the compression type and function to compress.
  • Compress the end response (if required) and log the details of compression.

New things

Compression details are added to the logs. In the start-up log, the new field enable_compression is introduced which is a boolean value.

A new content_encoding field is added to http_info of http-log. It is either brotli or gzip and null if the encoding is not enabled or not requested by the client. See the example below.

{
  "timestamp": "2019-08-23T02:05:28.059-05:00",
  "level": "info",
  "type": "http-log",
  "detail": {
    "operation": {
      "query_execution_time": 0.018795933,
      "user_vars": {
        "x-hasura-role": "admin"
      },
      "error": null,
      "request_id": "6283add2-db7b-4eec-9b1e-d14781893b2f",
      "response_size": 45,
      "query": null
    },
    "http_info": {
      "status": 200,
      "http_version": "HTTP/1.1",
      "url": "/v1/query",
      "ip": "127.0.0.1",
      "method": "POST",
      "content_encoding": "brotli"
    }
  }
}

Steps to test and verify

In the console, make any query with the network tab opened in developer options. Check the size of response received when compression enabled and disabled.

Limitations, known bugs & workarounds

@rakeshkky rakeshkky added c/server Related to server c/docs Related to docs labels Aug 19, 2019
@rakeshkky rakeshkky self-assigned this Aug 19, 2019
@netlify
Copy link

netlify bot commented Aug 19, 2019

Deploy preview for hasura-docs ready!

Built with commit 15fe9eb

https://deploy-preview-2751--hasura-docs.netlify.com

@ecthiender
Copy link
Member

There is a wai middleware which does gzip https://hackage.haskell.org/package/wai-extra-3.0.28/docs/Network-Wai-Middleware-Gzip.html . Shouldn't we use this directly?

@ecthiender
Copy link
Member

There is a wai middleware which does gzip https://hackage.haskell.org/package/wai-extra-3.0.28/docs/Network-Wai-Middleware-Gzip.html . Shouldn't we use this directly?

@rakeshkky pointed out that we wanted to support Brotli compression as well (the wai middleware only does gzip), hence we rolled our own implementation.

@rakeshkky rakeshkky changed the title allow compressing responses, close #2674 allow compressing responses (close #2674) Aug 21, 2019
@rakeshkky rakeshkky marked this pull request as ready for review August 23, 2019 07:20
Copy link
Contributor

@marionschleifer marionschleifer left a comment

Choose a reason for hiding this comment

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

I left some comments 🙂

Copy link
Contributor

@marionschleifer marionschleifer left a comment

Choose a reason for hiding this comment

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

💯

@hasura-bot
Copy link
Contributor

Review app for commit d66c417 deployed to Heroku: https://hge-ci-pull-2751.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2751-d66c4176

Resolve Conflicts:
	server/src-lib/Hasura/Server/App.hs
	server/src-lib/Hasura/Server/Init.hs
@hasura-bot
Copy link
Contributor

Review app for commit 7d4ac90 deployed to Heroku: https://hge-ci-pull-2751.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2751-7d4ac906

lexi-lambda
lexi-lambda previously approved these changes Sep 18, 2019
@hasura-bot
Copy link
Contributor

Review app for commit befabf8 deployed to Heroku: https://hge-ci-pull-2751.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2751-befabf84

Copy link
Member

@shahidhk shahidhk left a comment

Choose a reason for hiding this comment

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

I have a feeling that ENABLE_COMPRESSION can be confusing with data compression in the database etc. Should we call it ENABLE_API_RESPONSE_COMPRESSION or ENABLE_API_COMPRESSION to be as explicit as we can?

@lexi-lambda
Copy link
Contributor

I don’t have strong feelings about it either way, but how about ENABLE_HTTP_COMPRESSION? That seems both terse and unambiguous.

@shahidhk
Copy link
Member

Also, why do we need a flag to enable this? The response is only compressed if the right Accept-Encoding header is present in the request, right? Are there any issues if this is enabled silently?

@lexi-lambda
Copy link
Contributor

Are there any issues if this is enabled silently?

Not that I know of. I’d have no problem enabling it by default, but I’ll defer to @rakeshkky’s judgement on that.

The server compresses the response based on `Accept-Encoding` header
value
@rakeshkky
Copy link
Member Author

@shahidhk @lexi-lambda I've removed the --enable-compression flag in the latest commit. The server shouldn't have control over compression. It is the client which requests the compression based on Accept-Encoding header.

@hasura-bot
Copy link
Contributor

Review app for commit 15fe9eb deployed to Heroku: https://hge-ci-pull-2751.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2751-15fe9eba

@shahidhk shahidhk changed the title allow compressing responses (close #2674) allow compressing http responses (close #2674) Sep 19, 2019
@shahidhk shahidhk changed the title allow compressing http responses (close #2674) add gzip brotli compression to http responses (close #2674) Sep 19, 2019
Copy link
Member

@shahidhk shahidhk left a comment

Choose a reason for hiding this comment

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

LGTM

@shahidhk shahidhk merged commit 8a0615f into hasura:master Sep 19, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-2751.herokuapp.com is deleted

0x777 pushed a commit that referenced this pull request Sep 24, 2019
Add brotli library dependencies to the server docker image.

The compression feature introduced in #2751 requires brotli shared libraries at runtime. In original PR, adding them to server packager image was missing.
@Toub Toub mentioned this pull request Oct 10, 2019
polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
Add brotli library dependencies to the server docker image.

The compression feature introduced in hasura#2751 requires brotli shared libraries at runtime. In original PR, adding them to server packager image was missing.
hasura-bot pushed a commit that referenced this pull request Aug 5, 2022
## Description ✍️
This PR fixes the config status update when the `Service configured successfully` message is written before the server is actually spawned. Now the status is updated only when the server is spawned successfully. To be specific, this change posts the status closer to where we log `starting API server`.

### Related Issues ✍
#2751

### Solution and Design ✍
We update the status inside `runHGEServer` function. This helps in adding the message only when the server is started. If any exception is thrown before the server is spawned, only that message is written to `config_status` table instead of the `Service configured successfully` message.

## Affected components ✍️

- ✅ Server

PR-URL: hasura/graphql-engine-mono#5179
Co-authored-by: Naveen Naidu <30195193+Naveenaidu@users.noreply.github.com>
Co-authored-by: Anon Ray <616387+ecthiender@users.noreply.github.com>
GitOrigin-RevId: 7860008403aa0645583e26915f620b66a5bbc531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/docs Related to docs c/server Related to server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow compressing all responses from Hasura via a server flag
7 participants