Configuration options for Rack::Deflate #457
Conversation
|
Was this created with the intention of skipping image files with Deflator? Because that would be an excellent use case according to https://developers.google.com/speed/docs/best-practices/payload#GzipCompression. |
|
Yes, that's one use case I can think of. In general it's a bad idea to compress requests smaller than TCP packet size as it only adds an overhead on both ends. This threshold feature can always be configured on the web server level (nginx, Apache) but in case of services which do not proxy requests through a web server (e.g. Heroku Cedar) having such option in Rack::Deflater is the only option available. |
|
Definitely important. Would love to see this get merged! |
|
TCP packet size is different for everybody? But in general, seems to be about 64K? |
|
64kB is the theoretical upper limit for TCP/IP. In reality it depends on the connection type and does not exceed ~1 kB - still there's no need to compress such small amounts of data. |
|
Is this ready to be merged? |
|
Not yet as some new specs are failing under 1.8.7 / jruby / ree. It will be ready to merge within the next 24 hours. |
|
@bpinto it's ready for merge. I've refactored the tests a bit more, turned off deflating to see if all relevant tests fail (they do!) and reverted to a working state. Should be great now! |
| @app = app | ||
|
|
||
| @min_content_length = options[:min_content_length] || options['min_content_length'] |
There was a problem hiding this comment.
- Lets just support symbol option keys, not strings, consistent with other middleware options.
- Maybe
min_sizeormin_length? Just a little shorter?
There was a problem hiding this comment.
Lets default min-content-length to 1kb. There can be some advantages on mobile networks for smaller entities, but that domain has additional solutions and complexities - shared dictionaries are better, lets hope for them in http2.
|
In principle this looks great. I left you some line comments, thanks for your work so far! |
|
Thanks @raggi for all suggestions! I've just applied them so it should be more ready to merge. |
|
This needs rebasing on top of rack master. I'll be happy to merge this in the next release, but I am out of time for 1.5.0. |
|
That's a pity. I'll merge it as soon as possible to make it into 1.6. |
|
@raggi That should be it! |
|
To me, the safest default scenario is opt-in based on mime-type. HTML5 Bootstrap includes server-side configuration to compress the following mime-types I would consider the following list instead: If it's not too late, I would suggest mime-type evaluation instead of URL regexes. Providing a sane default set would be great; the current usage pattern is causing lots of issues with images and PDFs, since old browsers lie about Accept-Encoding. Rack::Deflate has become very important, as Heroku's new Cedar stack removed automatic gzip support, and requires that step be moved to the application itself. |
|
I definitely agree with you Nathanael. On Wednesday, January 30, 2013, Nathanael Jones wrote:
|
|
@raggi - any ideas about mime-type based evaluation? |
|
Should I submit a pull request? |
|
This is what I'm currently using: https://gist.github.com/nathanaeljones/4739210 |
|
Any ideas how to push it forward? |
|
Looks awesome to have; HAProxy 1.5dev19 dropped (temporarly) support for transparent gzip compression of chunked responses so we have to implement it at app levels, the more options, the better. |
| end | ||
|
|
||
| # Skip if response body is too short | ||
| if @min_length > headers['Content-Length'].to_i |
There was a problem hiding this comment.
I'm testing this but using Rainbows!, Rails 3.2.14, http 1.1 and I don't see any Content-Length header in this point, this thus fails and compression is skipped.
Rainbows! was started with the -N option to not insert any default middleware, so my config.ru is:
use Rack::ContentLength
use Rack::Chunked
run Myapp::Application
|
@jakubpawlowicz -- this is an awesome patch. It has a lot of features all together, which is probably part of why it's taking so long to merge. Here are some thoughts on how it can be simplified and improved. include/excludeIt seems like folks (including me) think that
I recommend:
skip_ifMaybe a better name for this to make it consistent with other DSLs would be min_content_lengthtl;dr: this option should be removed entirely, because it can be trivially implemented with There are several problems with making a default value.
There are 3 independent reasons I can think of to have a minimum value:
To increase the chances of your patch being merged, I think the default should be removed. Instead, there could be a recommended minimum in documentation. Furthermore, regarding point 3, this makes me think it would be nice to be able to adjust compression level as well. So after your patch is merged, I'd like to experiment with augmenting it so your compression_level option could be passed either an integer or a lambda, which would dictate if something is compressed at all, and with what compression level. ->(size){
case size
when 0..256
nil
when 256..1024
6
when 1025..Float::INFINITY
9
end
}And as I typed that out I realized: this could be achieved with |
|
as soon as i posted, that, i realized that
I've seen other libraries offer specific options and then an all-powerful |
|
@jjb appreciate your awesome feedback!
If you have a spare moment can you let me know what do you think about @nathanaeljones comment: #457 (comment) ? We could have it as an option for Thoughts? |
|
@jjb - that should be it! I decided to use Once we are all set I will squeeze all the commits into two - one for refactored Please let me know how it looks to you. |
|
And regarding failing specs I noticed master fails on them too. It shouldn't be an excuse though... |
|
@jakubpawlowicz awesome. the discussion about mime types was regarding where to get a list of defaults. it looks like you are using it to make sure that the provided types are valid. my feeling is that this isn't very useful and you can exclude this entirely. |
| # Skip if :if lambda is given and evaluates to false | ||
| if @if && | ||
| !@if.call(env, status, headers, body) | ||
| return false |
There was a problem hiding this comment.
return false if @if && !@if.call(env, status, headers, body)There was a problem hiding this comment.
Sure, it reads better.
|
@jjb sorry my mistake! So let's make sure we are on the same page regarding
|
|
that is one way. but, the current deflater approach is to deflate everything. so, you could keep this default behavior, and only change the behavior if So your code already does this :-D. so if you agree, you can just remove the check that it's in (i think i overexplained this but just trying to be extra clear :-D) |
| !(@include.match(headers['Content-Type']) && Rack::Mime::MIME_TYPES.values.include?(headers['Content-Type'])) | ||
| return false | ||
| end | ||
|
|
There was a problem hiding this comment.
return false if @include && !@include.match(headers['Content-Type'])There was a problem hiding this comment.
Since @include should be an array it should rather read:
return false if @include && !@include.include?(headers['Content-Type'])
or to make it more readable
return false if @include && @include.index(headers['Content-Type']).nil?
There was a problem hiding this comment.
sounds good. i find the first more readable, i'm not familiar with index and the required .nil? at the end seems complicated
There was a problem hiding this comment.
as long as it works as expected I'm fine with the first too
|
Cool, so we are on the same page. Will update the code shortly. |
| @app = app | ||
|
|
||
| @if = options[:if] | ||
| @include = options[:include] |
There was a problem hiding this comment.
the symbols are for the DSL but you could make more descriptive variables within the class
@condition = options[:if]
@compressible_types = options[:include]
|
I made this SO question to try to find a nice list of types for us to recommended in the documentation http://stackoverflow.com/questions/20477558/where-can-i-find-a-list-of-textual-mime-types |
|
@jjb here you go! Let's see what your SO question brings. |
|
@jjb So it's quashed into two commits, with better docs, and updated PR description @raggi @nathanaeljones You may like it much more right now! |
| end | ||
|
|
||
| # Skip if @compressible_types are given and does not include request's content type | ||
| return false if @compressible_types && !@compressible_types.include?(headers['Content-Type']) |
There was a problem hiding this comment.
First thing, thanks for this code it is a big help. One issue I ran into was this line. When the Content-type has the optional parameter value (in my case the character encoding) the content-type check fails. See RFC-1341. I am using .split(";")[0] and it is working for my use cases.
There was a problem hiding this comment.
Thanks @jayschab for a valuable input. I'll add a test case and your solution and we should be all good. 👍
There was a problem hiding this comment.
Found a bug in my suggested code in my testing. Content-Type isn't always set.
I ended up changing my version to this:
return false if @compressible_types && !(headers.has_key('Content-Type') && @compressible_types.include?(headers['Content-Type'][/[^;]*/]))
* Adds :if option which should be given a lambda accepting env, status, headers, and body options. * When :if evaluates to false a response body won't be compressed. * Adds :include option which should be given an array of compressible content types. * When :include don't include request's content type then response body won't be compressed.
| end | ||
|
|
||
| # Skip if @compressible_types are given and does not include request's content type | ||
| return false if @compressible_types && !(headers.has_key?('Content-Type') && @compressible_types.include?(headers['Content-Type'][/[^;]*/])) |
There was a problem hiding this comment.
@jayschab Updated it per your suggestion.
Found it problematic to test scenario without the 'Content-Type' header as it gets set every time in specs.
Any ideas?
|
is there anything we can do to push it forward after 1.5 years in PR? |
|
This would be nice to have. I'm currently shuffling around middleware to keep dragonfly jobs from being compressed. |
Configuration options for Rack::Deflate
|
😹 |
|
Wicked! Thanks @raggi! |
|
in case someone finds this thread, here's what I'm using for mime-types: include: Rack::Mime::MIME_TYPES.select{|k,v| v =~ /text|json|javascript/ }.values.uniq |
|
I had to do |
|
Note: the example has been updated again in #1211 Now it is: |
|
Even though I made that last update I was looking again at this today and this is a better solution: use Rack::Deflater, if: lambda { |*, body| body.each { |i| return i.bytesize > 512 } }The original change was due to the need to handle Line 177 in a05f8d5 As long as your threshold is below the buffer size(ie. I won't update the code documentation as this is not a general solution. |
Rack::Deflater currently does not support any configuration options which makes it cumbersome to control.
This pull request adds support for the following options:
Examples:
use Rack::Deflater, include: %w(text/json application/json)use Rack::Deflater, if: lambda { |env, status, headers, body| body.length > 512 }EDIT: description was update based on a PR discussion