[Zeek-Dev] Proposal: Improve Zeek's log-writing system with batch support and better status reporting

[Bob Murphy]

> On Jul 15, 2020, at 1:09 AM, Robin Sommer <robin at corelight.com> wrote:
> 
> On Thu, Jul 09, 2020 at 18:19 -0700, Bob Murphy wrote:
> 
>> Proposed Solution: Add a new optional API for writing a batch all at once, while
>> still supporting older log writers that don't need to write batches.
> 
> That sounds good to me, a PR with the proposed API would be great.

That’s sounds great. I wanted to bounce the ideas around with people who know more about Zeek than i do before going into detail on a proposed API.

> 
>> a. For non-batching log writers, change the "false" status to just mean
>>   "There was an error writing a log record". The log writing system will then
>>   report those failures to other Zeek components such as plug-ins, so they can
>>   monitor a log writer's health, and make more sophisticated decisions about
>>   whether a log writer can continue running or needs to be shut down.
> 
> Not quite sure what this would look like. Right now we just shut down
> the thread on error, right? Can you elaborate how "report those
> failures to other Zeek components" and "make more sophisticated
> decisions" would look like?

Yes, right now, any writer error just shuts down the entire thread.

That’s a good solution for destinations like a disk, because if a write fails, something really bad has probably happened. But Seth Hall pointed out that some log destinations can recover, and it’s not a good solution for those.

Here are a couple of examples:

1. A writer might send log records to a network destination. If the connection is temporarily congested, it would start working again when the congestion clears.

2. The logs go to another computer that’s hung, and everything would work again if somebody rebooted it.

Seth's idea was to report the failures to a plugin that could be configured by an administrator. A plugin for a writer that goes to disk could shut down the writer on the first failure, like Zeek does now. And plugins for other writers could approach the examples above with a little more intelligence:

1. The plugin for the network destination writer could decide to shut down the writer only after no records have been successfully sent for a minimum of ten minutes.

2. The plugin for the remote-computer writer could alert an administrator to reboot the other computer. After that, the writer would successfully resume sending logs.


> Could we just change the boolean result into a tri-state (1) all good;
> (2) recoverable error, and (3) fatal error? Here, (2) would mean that
> the writer failed with an individual write, but remains prepared to
> receive further messages for output. We could the also implicitly
> treat a current "false" as (3), so that existing writers wouldn't even
> notice the difference (at the source code level at least).

I don’t think that would work, because the member function in question returns a bool. To change that return value to represent more than two states, we’d have to do one of two things:

1. Change that bool to some other type.

If we did that, existing writers wouldn’t compile any more.

2. Use casts or a union to store and retrieve values other than 0 and 1 in that bool, and hope those values will be preserved across the function return and into the code that needs to analyze them.

We can’t count on values other than 0 or 1 being preserved, because the bool type in C++ is a little weird, and some behaviors are implementation-dependent. I wrote a test program using a pointer to store 0x0F into a bool, and other than looking at it in a debugger, everything I did to read the value out of that bool turned it into 0x01, including assigning it to another bool or an int. The only thing that saw 0x0F in there was taking a pointer to the bool, casting it to a pointer to char or uint8_t, and dereferencing that pointer.


> 
>> b. Batching log writers will have a new API anyway, so that will let log
>>   writers report more detail about write failures, including suggestions about
>>   possible ways to recover.
> 
> Similar question here: how would these "suggestions" look like?


For batching, I was thinking of having a way to send back a std::vector of structs that would be something like this:

struct failure_info {
    uint32_t index_in_batch;
    uint16_t failure_type;
    uint16_t recovery_suggestion;
};

The values of failure_type would be an enumeration indicating things like “fatal, shut down the writer”, “log record exceeds protocol limit”, “unable to send packet”, “unable to write to disk”, etc. Using a fixed-size struct member that’s larger than the enum would allow extra values to be added in the future.

recovery_suggestion would be a similar enum-in-larger-type, and let the writer convey more information, based on what it knows about the log destination. That could indicate things like, “the network connection has entirely dropped and no recovery is possible”, “the network connection is busy, try again later”, “this log record is too large for the protocol, but re-sending it might succeed if it’s truncated or split up”, etc.

- Bob