Martin Blix Grydeland
martin at varnish-software.com
Thu Feb 6 14:41:23 CET 2014
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
- 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.
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...
More information about the varnish-dev