Hi Bhaskar,
On Tue, Feb 04, 2014 at 04:11:58PM -0500, Bhaskar Maddala wrote:
> Hello,
>
> I took a stab at implementing
>
> - add a last activity date for each server (req/resp) that will be
> displayed in the stats. It will be useful with soft stop.
>
> from the ROADMAP document.
Cool!
> I digressed from the "last activity date (req/resp)" by logging a timestamp
> for the last session. I am not certain if this is an adequate replacement,
> however in my testing (http mode) with keep alive it seemed to suffice.
It will work for short sessions such as HTTP. But some people also have to
deal with long sessions (websocket, RDP, ...) and on these ones, it's not
really easy to know if things continue to move or not.
I know it will be more difficult to update these counters from the lower
layers. I suspect we could do that from the stream interfaces, because we
have the target information in the connection and if the target is a server
then we can update the counter. But I also think that what you did is
already a significant improvement over what we currently have, and we
could start with this and avoid over-complicating things as a first step.
I have some comments about the patch itself :
+/* set the time of last session on the backend */
+static void inline be_set_sess_last(struct proxy *be)
+{
+ tv_zero(&be->be_counters.last_sess);
+ tv_now(&be->be_counters.last_sess);
+}
You should just do the following above :
be->be_counters.last_sess = now;
The reason is that tv_now() uses gettimeofday() (which costs a syscall)
but more importantly which is not corrected for time drift and is not
monotonous. "now" is corrected. This is especially important on VMs
which tend to experience huge time drifts, because you don't want to
see negative time offsets or large ones.
+/* set the time of last session on the designated server */
+static void inline srv_set_sess_last(struct server *s)
+{
+ tv_zero(&s->counters.last_sess);
+ tv_now(&s->counters.last_sess);
+}
+
So same here, of course. You could even just save the seconds since
the microseconds are not used.
> I included testing of soft stop and start since that was explicitly mentioned.
>
> Let me know if this works or if there are better alternative that you would
> like me to pursue.
>
> [1] http://tinyurl.com/odlnvza
It's looking nice and natural. I would have expected to put this on the right
close to the checks but in fact it's much more logical where you put it,
especially since it allows to shrink the column title to 4 letters :-)
That's a nice work. If you could address the comments above, I'd happily
merge it.
Thanks Bhaskar!
Willy
