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 and expires to nginx server config. #5700

Merged
merged 3 commits into from
Apr 7, 2020

Conversation

Syonyk
Copy link
Contributor

@Syonyk Syonyk commented Apr 6, 2020

The provided nginx configs do not enable gzip by default, nor do they enable caching of the content. This pull request enables both gzip and content caching, via the expires header.

As the content has a version string appended, it should be safe to cache the content for longer periods of time, though the etag field should cover content changes as well. I'm flexible on the content caching part of this, if people have strong opinions there.

However, the gzip option changes the main page load from 4.6MB of content to 2.0MB of content - which makes a difference on low bandwidth connections, for minimal server CPU. The compression applies for meeting resources as well, and I see no downsides for enabling compression in the default configuration.

I've not added Apache config changes, as experimentally Apache has gzip enabled by default (but no expires headers - though mod_expires isn't enabled by default, so enabling caching headers would require playing with the enabled modules). As Apache is a second choice for servers, I don't think the change to enable mod_expires and then enable caching is worth the effort, although I could be convinced.

I've signed the contribution agreement document, you should be able to find that under my email address.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

saghul
saghul previously approved these changes Apr 6, 2020
@saghul
Copy link
Member

saghul commented Apr 6, 2020

LGTM! Just in case, @aaronkvanmeerten @damencho thoughts?

@rubenk
Copy link
Contributor

rubenk commented Apr 7, 2020

As for the expires headers, not all content has a version string appended (take watermark.png for example). I haven't checked. but iirc browsers only send an If-Match for an etag when a cache expires with Cache-Control: public. Landing this would mean changing the watermark would take at least 30 days to show up for all clients.

No objection to the gzip part, perhaps we can land just that part now?

@damencho
Copy link
Member

damencho commented Apr 7, 2020

Yes, the gzip part seems ok. Can you leave only that one. Thanks

Per discussion, expiration seems likely to cause more confusion than it solves.  Add gzip_vary to prevent content caches from caching un-compressed versions of the content and confusing browsers.
@Syonyk
Copy link
Contributor Author

Syonyk commented Apr 7, 2020

Ok, gzip left, expires headers removed.

I can open a separate tracking issue for content caching, as I think it would be useful, but given the bandwidth of videoconferencing, probably not a huge savings.

Current change just adds gzip to nginx config.

@damencho damencho merged commit b10aa42 into jitsi:master Apr 7, 2020
@Syonyk Syonyk deleted the enable-gzip branch April 7, 2020 15:14
Fair-Exchange pushed a commit to Fair-Exchange/safechat that referenced this pull request Apr 9, 2020
* Add gzip and expiration to nginx server config.

* Add application/json to gzip_types line to cover translations.

* Add gzip_vary for content caches, remove expires section.

Per discussion, expiration seems likely to cause more confusion than it solves.  Add gzip_vary to prevent content caches from caching un-compressed versions of the content and confusing browsers.
@luixxiul luixxiul added config Configuration related issues info:cla-signed The CLA has been signed labels May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Configuration related issues info:cla-signed The CLA has been signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants