Add support for gzip content-encoding#776
Conversation
ivan-valkov
force-pushed
the
gzip-support
branch
2 times, most recently
from
5b2c4c4 to
129f2ff
Compare
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
ivan-valkov
force-pushed
the
gzip-support
branch
from
129f2ff to
291894f
Compare
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
Thanks! Generally this looks reasonable though I have not fully tested it yet.
One thing to note, the golang client allows disabling compression as part of the options passed to the handler. I am wondering if we should do something similar here? The reason being, in some cases the cpu usage required to compress a large response can slow everything down too much.
Yeah, definitely, this makes sense. What would be the preferred semantic? GZIP compression enabled or disabled by default? I am conscious that if we enable it by default, some existing users of the client might see unexpected CPU spikes when using the new version of the client, so maybe we should require enabling GZIP compression explicitly? |
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
ivan-valkov
force-pushed
the
gzip-support
branch
from
b3b2bf6 to
c2dc1aa
Compare
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
| PYTHON376_OR_NEWER = sys.version_info > (3, 7, 5) | ||
|
|
||
|
|
||
| def _get_disable_compression() -> bool: |
There was a problem hiding this comment.
Instead of an environment variable what would you think of passing an additional argument to make_(w|a)sgi_app? My thought is that most users should have control over creating the apps so an env var is not necessary. We try to only use env vars in places where users might not have access to the code, such as registering metrics.
There was a problem hiding this comment.
Yeah, makes sense. I suppose we need to expose this option in start_http_server as well?
There was a problem hiding this comment.
I think that would be nice, though not completely required as start_http_server doesn't need to support all configuration for advanced user.
There was a problem hiding this comment.
awesome, yeah, this makes sense. I have updated the PR now and added documentation to the README. PTAL when you get some time. I don't think there is a release notes file we need to update but maybe worth mentioning this change when releasing the next version of the client (:
There was a problem hiding this comment.
Thanks! I plan to review this early next week. I will definitely mention it in the release notes for the next version!
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
Signed-off-by: ivan-valkov <iv.v.valkov@gmail.com>
ivan-valkov
force-pushed
the
gzip-support
branch
from
9ce658e to
245c3be
Compare
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
This looks great, thanks a lot!
3 participants
Resolves #775
If an
Accept-Encoding:gzipheader is present, will compress the output and set the correctContent-Encodingheader in the response. Added tests for both the wsgi and asgi implementations.