Add the option of hi-res (millisecond) timestamps #329
Conversation
Plus some misc tidying
There is otherwise no straightforward way for an application to know if hi-res timestamps are implemented
|
@chrissie-c Thank you for the feature-test macro. Just as a reminder (talked on IRC already) of weird behavior I've found. Without patch, current corosync master (707a9afa304dff3ad142739d6193dc2287c3fcc1) timestamps logged to stderr, files or blackbox are in sync. With patches applied (I've just compiled aa6bc9f branch from your libqb fork) behavior looks like: stderr /var/log/cluster/corosync.log and blackbox is similar. |
| struct qb_log_callsite *cs, | ||
| time_t timestamp, | ||
| struct timespec *timestamp, | ||
| va_list ap); |
There was a problem hiding this comment.
Actually pacemaker uses these to implement its PCMK_trace_blackbox option (which dumps a blackbox when a message at a specified function+line is logged). Would it be possible to add these as new methods, and somehow wrap the old ones around the new ones? I can use the new constant to know which signature to use going forward, but it would be nice if newer libqb could be used with existing pacemaker versions.
Awesome that we're getting the capability, btw.
There was a problem hiding this comment.
I'll have a look into that. The callback hooks quite low into the libqb code but it's doable I think.
It probably mean 2 API calls for registering a custom logger and expanding the target struct to contain 2 logger functions. Or I could overload vlogger if I'm feeling evil.
There was a problem hiding this comment.
Thinking closely about this, I don't think there's any point in doing it. Partly because this is for libqb 2.0, but mainly because we've already remove the linker-section based logging callsites, so that means anything linked against libqb1 is not going to work anyway.
There was a problem hiding this comment.
I wasn't thinking of already linked executables so much as someone upgrading libqb and rebuilding but not upgrading pacemaker, but that case may not be worth a lot of effort.
|
@chrissie-c Last fix (6bfba48) solved the issue I've had, so ACK by me. |
I'm going to do anothe patch to add to this that makes qb_blackbox backwards compatible with old BB files, so this new file format is interim.
| * from a new one with this header. | ||
| */ | ||
|
|
||
| struct _blackbox_file_header { |
There was a problem hiding this comment.
Because this structure is used directly in fwrite it should be marked as a packed (because qb_rb_write_to_file is saving field by field).
Yet another crazy idea (please ignore it if you feel like I had already too many of them :) ), what about storing it in network order so blackbox is "portable" between different endian systems (so it would be possible to share blackbox)?
There was a problem hiding this comment.
I'm not convinced it's worth the effort TBH.
There was a problem hiding this comment.
Understood (and kind of agree, we can always ask for text dump).
* log: Add high-resolution timestamp option for log files This adds the %T option to the log format for millisecond timestamps. There's a feature test macro QB_FEATURE_LOG_HIRES_TIMESTAMPS so that applications know that they are available. Because this changes the internal logging API, applications that use custom loggers will also need to change their custom logging destinations to take a struct timespec instead of a time_t. The above feature test macro will help in deciding which is appropriate.
3 participants
This adds a new %T option to the logging format for millisecond resolution timestamps.
It breaks a little-used API call (custom logging) but as we're heading for a 2.0 release anyway (because of the callsite changes) I don't see this as a big problem.