-
Notifications
You must be signed in to change notification settings - Fork 790
Enable HTTP response compression for API and static assets #4774
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
base: master
Are you sure you want to change the base?
Conversation
|
Is there any measurable speed or added resource consumption with bigger servers? While I think the compression is awesome, if this adds measurable resource to the server I don't think this should be merged. Big libraries already suffer performance issues, saving a few mb (for an media server) won't make the difference. The 1kb threshold will be hit with nearly any request ABS does Due to speed e.g. compression for downloads was already removed But in general awesome idea |
|
Is this better than just enabling compression at the reverse proxy level? That will apply to all paths and other services running on a server. |
In terms of latency hitting the API it’s within noise levels (+/- 3ms per request on my potato NAS). Actual resources used may be tough to measure, unless there’s a node.js profiler we can use (I’m unfamiliar with the ecosystem, and a quick search brought up nothing). It’s likely a trade-off. More resources spent compressing the response, but fewer sending it. The savings aren’t negligible though.
Same result, but not everyone runs a reverse proxy. I just use Tailscale, for example. |
If one uses an RP, they are (assuming the connection between the RP and ABS is not bottlenecked). But I also see the use case in local networks when using a VPN where this could be helpful. ABS has a large payload problem, and with some third-party clients "caching" the whole library (items), this could significantly reduce traffic during setup, especially for apps that use the /me endpoint a lot. I know too little about how this interacts with an existing RP, but if it only compresses the payload without an RP, I think it could be worth it. If it also compresses with an RP, I think it's not worth it. It's not much added latency (but we should not add things that are not broken, but fix the root cause, the data model), but the only real use case is a VPN. |
|
This probably needs more testing before it can be added if it also applies to any media streaming or downloading. There was an issue that looks to be caused by reverse proxy adding compression to downloads as well, but that probably needs some more investigation before saying this will cause the same problem. advplyr/audiobookshelf-app#1553 |
It will still compress the response if the app is behind a RP. The RP won't (shouldn't?) attempt to compress it a second time, though. From the nginx docs, for example:
We should be good there, the middleware won't attempt to compress any non-text content. It uses compressible behind the scenes. |
Brief summary
This introduces response compression using Express’s compression middleware.
Which issue is fixed?
N/A
In-depth Description
A response is only compressed if:
Note that this also compresses static js/etc files served for the web app, not just API requests.
How have you tested this?
Manual testing by directly hitting the API and navigating around the web/mobile apps.
Some unscientific before/after numbers:
UI Page Loads
API Endpoints
/api/libraries/<id>/items/api/items/<id>Screenshots
N/A