Varnishhist

[Martin Blix Grydeland]

Hiya Guillaume,

The patches looks good, and quite complete. Though there are some structure
things that I believe needs to be addressed here.

Where varnishtop concerns itself with single log records and keeps
statistics on how many occurrences of these it finds, varnishhist concerns
itself with transactions. A transaction here then is the complete set of
log records for some activity, e.g. a client request. So a single request
transaction then should make a single data point for the graph, where the
necessary information (e.g. size of data / hit flag) is extracted from
those log records belonging to that transaction. The current loop design in
accumulate() gets this wrong, where the data point adding is done once for
every log record instead of once per transaction. This makes a lot more
data points than should have been inserted, using mostly bogus data.

So I think the following changes has to be made:

   - accumulate() outer loop should skip transactions of the wrong type.
   correct type would be client requests (and possibly backend requests?)
   - accumulate() inner loop needs to run over the log records and extract
   the necessary information (HIT flag, size/timing etc.). Should also make
   sure a ReqEnd record has been seen to filter out e.g. restarts.
   - accumulate() should then add data point only when inner loop sees it
   has the information necessary (e.g. found the log record that had the field
   we search for)
   - The -g option should be limited. Since we are looking for transactions
   only, the "raw" grouping mode doesn't make any sense (it makes every log
   record reported seen as a transaction in itself from the API). varnishncsa
   does something similar there.
   - The -i -I -x -X options I think should be removed. These options are
   there to filter output of utilities, controlling which records should go
   into the output. But since varnishhist deals with the transaction level
   they aren't applicable. (Also makes the call to VSL_Match() in the loop
   superfluous)
   - Some thought needs to be put into whether we support backend requests.
   First iteration could perhaps just look at client requests, and remove the
   -b and -c options too. Backend transactions can then be added later.

Regards,
Martin Blix Grydeland


On 4 February 2014 15:45, Guillaume Quintard <
guillaume.quintard at smartjog.com> wrote:

> Here are the patches. They should be pretty complete compared to the
> varnishtop ones :-) Let me know if you are missing something.
>
> I haven't touched the Makefile.phk though, as varnishadm was failing
> too, and IIRC, autotools are here to last. You may want to rename the
> "-p period" argument, but other than that, I think the work is done.
>
> EDIT: patches!
>
> --
> Guillaume Quintard
>
>


-- 
<http://varnish-software.com>*Martin Blix Grydeland*
Senior Developer | Varnish Software AS
Cell: +47 21 98 92 60
We Make Websites Fly!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20140206/a1d1ab64/attachment.html>