On Thu, Apr 06, 2023 at 09:17:34PM +0200, Willy Tarreau wrote: > > > > +/* limit compression rate */ > > > > + if (global.comp_rate_lim > 0) > > > > + if (read_freq_ctr(&global.comp_bps_in) > > > > > global.comp_rate_lim) > > > > + goto fail; > > > > > > I had a doubt regarding this one but I think it's indeed reasonable to > > > use the same global value for all directions as the goal was precisely > > > to be able to enforce a total limit on all compressed streams to limit > > > the resource impacts. > > > > I think it makes sense, unless someone wants to give more priority to > > request compression vs response compression, or vice versa, but I'll > > rethink it if somebody gives me a valid use-case. > > We'll see, but I strongly doubt this would ever happen because those who > want to enforce speed limits are often hosting providers who don't want > that a single site eats all their CPU, so they configure maximum compression > bandwidths. It dates back from the era where it was still common to start > several daemons, which is no more the case these days. So actually if > anything would change in the short term it would rather be to have > per-frontend and/or per-backend total limits rather than splitting > req/res which still concern the same application. Yeah that makes sense.
> > > I have not seen anything updating the stats, so I suspect that request > > > and response compression stats are accounted as response stats, which > > > can be a problem showing compression rates larger than 100%. If you hover > > > over your frontend's "Bytes / Out" fields, you'll see a yellow box with > > > numbers such as: > > > > > > Response bytes in: 468197882863 > > > Compression in: 117157038990 > > > Compression out: 93277104103 (79%) > > > Compression bypass: 0 > > > Total bytes saved: 23879934887 (5%) > > > > > > Here I'm figuring that we'll need to add the same for the upload, so > > > these are extra stats. I can possibly help locate where to place them, > > > but I'll need that you figure where they are updated. > > > > Ok so I've had mixed feelings about this, because I thought a global > > counter could be useful too. But ultimately I think it's better to keep > > them separated, if only so that the percentages make sense. > > Yes, and another point is that when you start with a global one and > finally decide to introduce a separate one for one direction so that > you can subtract it from the total to get the other one, it's not > uncommon to see negative numbers due to rounding errors or timings, > and that's extremely confusing for those who deal with that. Thus I > prefer to let users sum data on their side rather than providing > totals and separate ones later. That's a good point. > > We could > > have had Request + Response bytes there, but I think it's less useful. > > So I added the counters, but do not expose them for requests yet, as I'm > > unsure where to do that exactly, but that can easily be addressed with a > > separate commit. > > Yes we can have a look later. I think we'll add new metric in the stats > that will appear in a new tooltip on the bytes/in column of the stats > page and that will be OK. Yeah I was considering something like this, but wasn't so sure. > Just a few tiny cosmetic comments below: [Took care of unaligned comments] > > From: Olivier Houchard <ohouch...@backtrace.io> > > Date: Thu, 6 Apr 2023 00:33:48 +0200 > > Subject: [PATCH 5/5] MEDIUM: compression: Make it so we can compress > > requests > > as well. > (...) > > diff --git a/doc/configuration.txt b/doc/configuration.txt > > index 3468c78f3..63e8d3440 100644 > > --- a/doc/configuration.txt > > +++ b/doc/configuration.txt > > @@ -5099,7 +5111,15 @@ compression offload > > If this setting is used in a defaults section, a warning is emitted and > > the > > option is ignored. > > > > - See also : "compression type", "compression algo" > > + See also : "compression type", "compression algo", "compression " > ^^^ > A word is missing here, I suspect it was "direction" since that one > references "offload" which is where we are. Ahahah yeah, pretty sure I removed "tocompress" to replace it with direction, but was probably distracted before I did so, and forgot. > OK otherwise it looks good to me. I suggest you adjust these cosmetic > details and directly push it. Done, thanks! > I'm having one question by the way: how did you manage to test this ? > Did you find a server that supports decompressing requests, or do you > only have some in-house stuff for this ? I'm wondering if we could manage > to make a regtest for this, at least by putting some expect rules on > the content-encoding header probably. My test server was actually a nginx with custom LUA code to decompress stuff. Maybe we can do something similar with haproxy? Regards, Olivier
