-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
server: implement websocket compression setting (fixes #3292) #5928
Conversation
As discussed, the websocket package has a compression setting built-in. This commit implements the wiring to allow such configuration through the `connection-compression` command line flag, or the `HASURA_GRAPHQL_CONNECTION_COMPRESSION` environment variable.
Beep boop! 🤖 Hey @gahag, thanks for your PR! One of my human friends will review this PR and get back to you as soon as possible. Stay awesome! 😎 |
Deploy preview for hasura-docs ready! Built with commit f559c00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As requested in code review.
@lexi-lambda I've made a new commit with the suggested changes. Thanks for the review! Should I squash the two commits? |
You can leave the commits as-is, we usually squash the commits in a PR at merge time. Let's give Alexis some more time to have another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog
Beep boop! 🤖 Awesome work @gahag! All of us at Hasura ❤️ what you did. Thanks again 🤗 |
Hello @gahag! Thanks so much for your contribution to Hasura OSS during the Hacktoberfest 2020! We'd love to send you some Hasura swag as a token of appreciation and you will be getting an email with the swag form shortly. ✨ |
Compression on websockets doesn't seem to actually work anymore. The headers indicate that permessage-deflate is on, but the packets are still not compressed. After checking with wireshark, even with that header present, the frame size is the same as the length of the payload. Manually using the deflate tool shows that the text is 1000% compressible. |
Description
As discussed, the websocket package has a compression setting built-in.
This commit implements the wiring to allow such configuration through
the
connection-compression
command line flag, or theHASURA_GRAPHQL_CONNECTION_COMPRESSION
environment variable.Changelog
CHANGELOG.md
is updated with user-facing content relevant to this PR. If no changelog is required, then add theno-changelog-required
label.Affected components
Related Issues
#3292
Solution and Design
As discussed, the websocket package has a compression setting built-in.
This commit implements the wiring to allow such configuration through
the
connection-compression
command line flag, or theHASURA_GRAPHQL_CONNECTION_COMPRESSION
environment variable.Steps to test and verify
Run graphql with the compression flag, and send a big but low entropy string through a graphql client.
Limitations, known bugs & workarounds
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
run_sql
auto manages the new metadata through schema diffing?run_sql
auto manages the definitions of metadata on renaming?export_metadata
/replace_metadata
supports the new metadata added?GraphQL
Breaking changes
No Breaking changes
There are breaking changes:
Metadata API
Existing
query
types:args
payload which is not backward compatibleJSON
schemaGraphQL API
Schema Generation:
NamedType
Schema Resolve:-
null
value for any input fieldsLogging
JSON
schema has changedtype
names have changed