[PROPOSAL]: Add two fields to request_rec

[Bojan Smojver <bo...@rexursive.com>]

I'm proposing to add two extra fields to request_rec structure:

apr_off_t bytes_in
apr_off_t bytes_out

These babies would be used to record the number of bytes received and sent (on
the  wire) per each request. I'm writing a little filter module (with a lot of
help from Justin :-), to do the actual counting. The logging of these numbers
would require a patch to mod_log_config.c, which is already done. There would
also be some other changes required throughout Apache 2.0, mainly to do with
making sure that connection filter have a valid value of f->r.

I wanted to find out if there would be enough support for such a change as it
makes a significant difference to the code of the module.

The unfortunate side effect would be yet another MMN bump.

Bojan

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Re: [PROPOSAL]: Add two fields to request_rec

[Bojan Smojver <bo...@rexursive.com>]

Quoting Brian Pane <br...@apache.org>:

> The one problem I see is that, because the response data might
> not be written until after the request has been logged--in the
> case of a keep-alive request--the bytes_out field will need to
> be updated *before* setting aside any output in core_output_filter().
> I.e., output bytes should be counted when they're passed to the
> core out filter, not when they're actually written.  That's
> somewhat complicated, because the core output filter code is
> so convoluted.

The way mod_logio works at the moment (on my box) is at the connection level
(i.e. AP_FTYPE_CONNECTION), which should run before core_output_filter (i.e
AP_FTYPE_NETWORK). The counting is done by a single call to apr_brigade_length,
which does not seem all that heavy, especially given that I call with with
read_all set to 0. So, I think we should be OK there, unless I didn't understand
the order of output filters properly.

> But if you can make that work, then we probably can also stop
> computing r->bytes_sent in the content-length filter on streamed
> requests, which will save one more scan through the brigade on
> each request.

Huh, don't know enough (yet :-) to contribute anything useful there.

> Fortunately, this only requires an increment for the minor
> number.  (Put the new fields at the end of the structure to
> maintain backward compatibility.)

Yep, by all means.

> So, +1 on the proposal.

Thanks.

Bojan

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Re: [PROPOSAL]: Add two fields to request_rec

[Brian Pane <br...@apache.org>]

Bojan Smojver wrote:

>I'm proposing to add two extra fields to request_rec structure:
>
>apr_off_t bytes_in
>apr_off_t bytes_out
>
>These babies would be used to record the number of bytes received and sent (on
>the  wire) per each request. I'm writing a little filter module (with a lot of
>help from Justin :-), to do the actual counting. The logging of these numbers
>would require a patch to mod_log_config.c, which is already done. There would
>also be some other changes required throughout Apache 2.0, mainly to do with
>making sure that connection filter have a valid value of f->r.
>

The one problem I see is that, because the response data might
not be written until after the request has been logged--in the
case of a keep-alive request--the bytes_out field will need to
be updated *before* setting aside any output in core_output_filter().
I.e., output bytes should be counted when they're passed to the
core out filter, not when they're actually written.  That's
somewhat complicated, because the core output filter code is
so convoluted.

But if you can make that work, then we probably can also stop
computing r->bytes_sent in the content-length filter on streamed
requests, which will save one more scan through the brigade on
each request.

>I wanted to find out if there would be enough support for such a change as it
>makes a significant difference to the code of the module.
>
>The unfortunate side effect would be yet another MMN bump.
>

Fortunately, this only requires an increment for the minor
number.  (Put the new fields at the end of the structure to
maintain backward compatibility.)

So, +1 on the proposal.

Brian