Michaël-san, Yugo-san,
>> I am fine with this version, but I think it would be better if we have
>> a comment explaining what "tx" is for.
Yes. Done.
>> Also, how about adding Assert(tx) instead of using "else if (tx)" because
>> we are assuming that tx is always true when agg_interval is not used, right?
Ok. Done.
> Agreed on both points. From what I get, this code could be clarified
> much more,
I agree that the code is a little bit awkward.
> and perhaps partially refactored to have less spaghetti
> code between the point where we call it at the end of a thread or when
> gathering stats of a transaction mid-run, but that's not something to
> do post-beta1.
Yep.
> I am not completely sure that the result would be worth it either.
I'm not either.
> Let's document things and let's the readers know better the
> assumptions this area of the code relies on, for clarity.
Sure.
> The dependency between agg_interval and sample_rate is one of those
> things, somebody needs now to look at the option parsing why only one is
> possible at the time.
Actually it would work if both are mixed: the code would aggregate a
sample. However it does not look very useful to do that, so it is
arbitrary forbidden. Not sure whether this is that useful to prevent this
use case.
> Using an extra tx flag to track what to do after the loop for the
> aggregate print to the log file is an improvement in this direction.
Yep.
Attached v4 improves comments and moves tx as an assert.
--
Fabien.
