<div dir="ltr">Hiya Guillaume,<div><br></div><div>The patches looks good, and quite complete. Though there are some structure things that I believe needs to be addressed here.</div><div><br></div><div>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.</div>
<div><br></div><div>So I think the following changes has to be made:</div><div><ul><li>accumulate() outer loop should skip transactions of the wrong type. correct type would be client requests (and possibly backend requests?)</li>
<li>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.</li><li>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)</li>
<li>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.</li>
<li>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)</li>
<li>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.</li>
</ul><div>Regards,</div></div><div>Martin Blix Grydeland</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 4 February 2014 15:45, Guillaume Quintard <span dir="ltr"><<a href="mailto:guillaume.quintard@smartjog.com" target="_blank">guillaume.quintard@smartjog.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Here are the patches. They should be pretty complete compared to the<br>
varnishtop ones :-) Let me know if you are missing something.<br>
<br>
I haven't touched the Makefile.phk though, as varnishadm was failing<br>
too, and IIRC, autotools are here to last. You may want to rename the<br>
"-p period" argument, but other than that, I think the work is done.<br>
<br>
</div></div>EDIT: patches!<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Guillaume Quintard<br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div><table border="0" cellpadding="0" cellspacing="0" style="font-size:12px;line-height:1.5em;font-family:'Helvetica Neue',Arial,sans-serif;color:rgb(102,102,102);width:550px;border-top-width:1px;border-top-style:solid;border-top-color:rgb(238,238,238);border-bottom-width:1px;border-bottom-style:solid;border-bottom-color:rgb(238,238,238);margin-top:20px;padding-top:5px;padding-bottom:5px">
<tbody><tr><td width="100"><a href="http://varnish-software.com" target="_blank"><img src="http://www.varnish-software.com/static/media/logo-email.png"></a><span></span><span></span></td><td><strong style="font-size:14px;color:rgb(34,34,34)">Martin Blix Grydeland</strong><br>
Senior Developer | Varnish Software AS<br>Cell: +47 21 98 92 60<br><span style="font-weight:bold">We Make Websites Fly!</span></td></tr></tbody></table></div>
</div>