[libvirt] [PATCH 2/4] virsh: Add timestamps to events

[Andrea Bolognani]

On Thu, 2016-01-07 at 22:23 +0100, Jiri Denemark wrote:
> On Mon, Dec 21, 2015 at 16:25:56 +0100, Andrea Bolognani wrote:
> > On Mon, 2015-12-21 at 11:04 +0100, Jiri Denemark wrote:
> > >  
> > > @@ -12154,7 +12155,16 @@ virshEventPrint(virshDomEventData *data,
> > >       if (!data->loop && *data->count)
> > >           goto cleanup;
> > >   
> > > -    vshPrint(data->ctl, "%s", msg);
> > > +    if (data->timestamp) {
> > > +        char timestamp[VIR_TIME_STRING_BUFLEN];
> > > +
> > > +        if (virTimeStringNowRaw(timestamp) < 0)
> > > +            timestamp[0] = '\0';
> > > +
> > > +        vshPrint(data->ctl, "%s: %s", timestamp, msg);
> > > +    } else {
> > > +        vshPrint(data->ctl, "%s", msg);
> > > +    }
> > 
> > So the timestamp is not really for the moment the even occurred,
> > rather the moment it was printed. I don't know if the difference is
> > something we should really care about.
> 
> The main reason for adding the timestamp was to be able to look at the
> output and see, e.g., that a few events came very close to each other,
> then there was a 30s delay before another event came. I don't think
> getting the exact timing from libvirtd would be useful for anything.
> 
> > The signature for virConnectDomainEventGenericCallback, unlike the one
> > for virConnectDomainQemuMonitorEventCallback, does not provide timining
> > information. Would it be a good idea to maybe have a new kind of
> > callback for generic events that includes such information, instead of
> > using the timestamp from when we printed the message?
> 
> It's not just about the callback signature, each event would have to
> provide the timestamp... IMHO it's not worth the effort at all.

I see, let's just keep it as it is then.

> > What about having qemu-monitor-event support --timestamp as well? I know
> > the timing information are already part of the output, but we could use
> > the same format used here (much more readable) and avoid duplicating
> > them in the message. POC attached.
> 
> qemu-monitor-event is designed to provide more-or-less raw data from
> QEMU. Personally, I'd leave it alone, but I don't feel strongly either
> way :-) The main reason for doing it this way was laziness (what a
> surprise!).

Since you don't feel strongly either way, I'll just check my POC again
and then send it out for review :)

In the meantime, ACK on your patch.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team