Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvs: add RFC 18 eventlog support #1671

Merged
merged 7 commits into from Sep 25, 2018

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Sep 21, 2018

This is a small class for manipulating RFC 18 KVS eventlogs, as discussed in #1654.

@grondo's idea of keeping this decoupled from futures and KVS transactions was a great one. This is completely standalone and can be covered well with unit tests.

I added a flux kvs eventlog append|get subcommand that allows events to be appended to an eventlog, and the logs to be dumped and also watched. The get implementation illustrates how the iterators can be used with flux_kvs_eventlog_update() in a continuation to "consume" only the new eventlog entries.

This still needs a man page but any feedback on the API would be useful at this point.

One thing I waffled on was the static buffer typedefs for event name and context. The RFC didn't establish any maximum field widths for this, but I assumed 16 and 256, respectively here to avoid mallocs during parsing. Either the RFC needs to be updated or this needs to change.

Commit messages contain additional commentary.

Here's the current proposed API:

/* RFC 18 KVS Event Log
 *
 * Events are of the form:
 *   "timestamp name [context ...]\n"
 */
typedef char flux_kvs_event_name_t[17];
typedef char flux_kvs_event_context_t[257];

/* Create/destroy an eventlog
 */
struct flux_kvs_eventlog *flux_kvs_eventlog_create (void);
void flux_kvs_eventlog_destroy (struct flux_kvs_eventlog *eventlog);

/* Encode/decode an eventlog to/from a string (caller must free)
 */
char *flux_kvs_eventlog_encode (const struct flux_kvs_eventlog *eventlog);
struct flux_kvs_eventlog *flux_kvs_eventlog_decode (const char *s);

/* Update an eventlog with new encoded snapshot 's'.
 */
int flux_kvs_eventlog_update (struct flux_kvs_eventlog *eventlog,
                              const char *s);

/* Append an encoded event to eventlog.
 */
int flux_kvs_eventlog_append (struct flux_kvs_eventlog *eventlog,
                              const char *s);

/* Iterator for events.
 */
const char *flux_kvs_eventlog_first (struct flux_kvs_eventlog *eventlog);
const char *flux_kvs_eventlog_next (struct flux_kvs_eventlog *eventlog);

/* Encode/decode an event.
 * flux_kvs_event_encode() sets timestamp to current wallclock.
 * flux_kvs_event_encode_timestmap() allows timestamp to be set to any value.
 * Caller must free return value of flux_kvs_event_encode().
 */
int flux_kvs_event_decode (const char *s,
                           double *timestamp,
                           flux_kvs_event_name_t name,
                           flux_kvs_event_context_t context);
char *flux_kvs_event_encode (const char *name, const char *context);
char *flux_kvs_event_encode_timestamp (double timestamp,
                                       const char *name,
                                       const char *context);

@garlick garlick force-pushed the garlick:eventlog branch from cf9c01e to f312dc4 Sep 21, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 21, 2018

Coverage Status

Coverage increased (+0.03%) to 80.018% when pulling 16093e7 on garlick:eventlog into 54ce1fb on flux-framework:master.

@garlick garlick force-pushed the garlick:eventlog branch from f312dc4 to 88b9179 Sep 21, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 21, 2018

Codecov Report

Merging #1671 into master will decrease coverage by 0.01%.
The diff coverage is 87.39%.

@@            Coverage Diff             @@
##           master    #1671      +/-   ##
==========================================
- Coverage   79.69%   79.67%   -0.02%     
==========================================
  Files         185      186       +1     
  Lines       34084    34330     +246     
==========================================
+ Hits        27162    27353     +191     
- Misses       6922     6977      +55
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 81.29% <79.24%> (-0.28%) ⬇️
src/common/libkvs/kvs_eventlog.c 93.57% <93.57%> (ø)
src/modules/connector-local/local.c 72.76% <0%> (-1.63%) ⬇️
src/common/libflux/mrpc.c 86.11% <0%> (-1.2%) ⬇️
src/common/libflux/rpc.c 93.23% <0%> (-0.76%) ⬇️
src/bindings/lua/flux-lua.c 82.15% <0%> (-0.7%) ⬇️
src/common/libflux/response.c 82.89% <0%> (-0.66%) ⬇️
src/broker/content-cache.c 72.88% <0%> (-0.64%) ⬇️
src/common/libflux/future.c 85.57% <0%> (-0.32%) ⬇️
src/cmd/flux-module.c 85.06% <0%> (-0.31%) ⬇️
... and 7 more

@garlick garlick requested review from grondo and chu11 Sep 21, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 21, 2018

Great! I will try to poke at this today.

The RFC didn't establish any maximum field widths for this, but I assumed 16 and 256, respectively here to avoid mallocs during parsing. Either the RFC needs to be updated or this needs to change.

My initial reaction was that these limits seem small, especially if we started namespacing event names, e.g. "debugger.starting" already overflows 16 characters.

Probably a stupid idea, but since the event log data is already in memory to be "parsed", could you use the strings "in place" to avoid any mallocs or even copying? (If that is an optimization we need right now). I'm actually fine with using statically sized buffers, but just thought I'd throw the idea out there.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 21, 2018

since the event log data is already in memory to be "parsed", could you use the strings "in place" to avoid any mallocs or even copying?

I had something like that at first, but it seemed like both the API and implementation were getting a bit complicated, so I backed up and did this. Perhaps revisiting this makes sense.

I'd also be fine with increasing the static limits, getting this in, getting our sprint done, and coming back to it.

(i'll be offline soon - traveling back to Livermore)

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 21, 2018

I'd also be fine with increasing the static limits, getting this in, getting our sprint done, and coming back to it.

This sounds like a good plan to me. The name seems to me like the more important thing to increase the size of. If someone wants to record a large event context, they can always put the full context somewhere in the KVS (as a separate operation) and then store that KVS key as the event context.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 21, 2018

I'd also be fine with increasing the static limits, getting this in, getting our sprint done, and coming back to it.

This sounds like a good plan to me. The name seems to me like the more important thing to increase the size of. If someone wants to record a large event context, they can always put the full context somewhere in the KVS (as a separate operation) and then store that KVS key as the event context.

I agree, if nothing else it may encourage users to avoid abusing the eventlog when some other mechanism would suffice.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 21, 2018

Great, I'll submit a PR to the rfc project to add static limits.

I'll also add a man page here.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 24, 2018

Fixed a couple of man page typos. LMK if I should squash the incremental development. (Didn't want to do that while people were potentially reviewing).

@garlick garlick removed the request for review from chu11 Sep 24, 2018

garlick added some commits Sep 18, 2018

libkvs/eventlog: add RFC 18 KVS event log support
Add functions for manipulating RFC 18 KVS event logs,
which consist of newline-terminated events that look like:

timestamp name [context ...]\n

Where timestamp is seconds since UNIX Epoch UTC, with
subsecond precision, name is a single word event name,
and context is free form text that may include arguments
or human readable log data, depending on how the eventlog
is being used.

The eventlog API is decoupled from KVS lookup and txn API's
so that it can be indepedently tested.  Higher level
API functions may prove useful later on, but for now let's
start with the basics.

The eventlogs support multiple un-coordinated writers,
utilizing the FLUX_KVS_APPEND operation to update the log
without rewriting it.  Appends are compatible with the
new FLUX_KVS_WATCH flag as well, since each append is
a direct operation on a key that is attached to the KVS
setroot event message.

The entire eventlog snapshot is returned by a KVS lookup.
There is currently no KVS mechanism to receive only the
updates to the log.  Therefore, a flux_kvs_eventlog_update()
function is provided which appends only the new log entries
to a struct flux_kvs_eventlog object.  Pointers to events
obtained through the iterators before an update remain valid
after the update.

To avoid the need to allocate memory, the event encode/decode
functions copy the name and optional context to fixed length
buffers.  The maximum length of these fields was not initially
specified in RFC 18, so that will need to be addressed if
the fixed limits are acceptable.
cmd/flux-kvs: add eventlog subcommand
Add a flux-kvs eventlog subcommand, with "append" and "get"
sub-subcommands.

Usage: flux-kvs eventlog append [-t SECONDS] key name [context ...]
   or: flux-kvs eventlog get [-u] [-w] [-c COUNT] key
Manipulate a KVS eventlog
  -h, --help             Display this message.

Append adds an event to the event log specified by 'key'.
Get retrieves the eventlog and formats the dates to make
them human readable.

The --watch option may be used to monitor the eventlog,
displaying new events as they are added.

--count=COUNT limits the number of events that are
displayed before exiting, otherwise --watch runs until
an error occurs.

@garlick garlick force-pushed the garlick:eventlog branch from ced5b36 to 16093e7 Sep 25, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 25, 2018

I went ahead and squashed down the incremental development on verbal assent by @grondo.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 25, 2018

Restarted a builder that seemed to exit early out of the hydra test.

@grondo

grondo approved these changes Sep 25, 2018

Copy link
Contributor

grondo left a comment

Looks good! I went through the PR and tested it on my desktop and ipa and didn't see any issues.

@grondo grondo merged commit 1b55e4f into flux-framework:master Sep 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 80.018%
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 25, 2018

Thanks!

@garlick garlick deleted the garlick:eventlog branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.