VIP23 (VSL refactoring) design sketch

[Dridi Boukelmoune]

On Fri, Apr 12, 2019 at 9:24 PM Poul-Henning Kamp <phk at phk.freebsd.dk> wrote:
>
> --------
> In message <CABoVN9DAYQYd7OiaH-MJg-A425H7OGccZ39aCk0xE_nbqHB9mQ at mail.gmail.com>, Dridi Boukelmoune writes:
>
> >Interesting, for textual logs the effect of vsl_reclen is
> >straightforward but how should we deal with binary records truncation?
>
> Maybe redefine the limit to apply to individual strings (ie: header
> values) rather than the full record ?
>
> >> Some other lessons learned:
> >>
> >> The binary records are not necessarily shorter than the ASCII-formatted
> >> records.
>
> It is incredible how often people overlook this.  I've been ranting
> about it for almost two decades now, in particular in context of the
> userland/kernel API.
>
> Imagine if in addition to errno there also were a const char *errstr
> which the kernel could fill out, that would do *wonders*, in particular
> for EINVAL.
>
> My favourite example was a Nx64 card where the driver spent more
> than 35kloc of source code on defining a ton of structs for ioctl(2)
> usage but no matter what the kernel found deficient, the only thing
> it told you was return(EINVAL).
>
> I threw out the entire thing and added a single ioctl which passed a
> struct with an input string, containing a command like
>         "set chan 4 ts 4-12"
> and another string out with an error message like:
>         "channel 11 already allocated to chan 5"
> didn't even spend 1kloc on it...

I have given a bit more thought to this and going with structured
binary VSL records will have more consequences than a blank-separated
list of fields.

First we lose the ability to treat the whole record as a single sting
out of the box. The client will need to reconstruct the string from
the various field types that may constitute a record. Think string and
regex operators.

Then we further the disconnect between today's VSL parsing and VSL
query parsing. Query parsing of strings does already support quoted
strings, and they are not mandatory. We can even query a prefix
containing blanks by quoting the prefix:

    -q 'VCL_Log:"just because" eq "we can"'

My attempt at solving this (#2872) was bringing back consistency by
allowing fields to be quoted. I'm glad with the turbo-encabulator-friendly
outcome for Backend_health, but we need to consider the std.log()
escape hatch over which we have no input control, and for which we
can't currently provide a VCC interface to describe the fields, pass
them as a vararg and still ensure at "compile time" that arguments
match the spec. Sure, VCC could learn to do that but it sounds a tiny
bit complicated.

Finally, regarding the VSL file header that we'd hypothetically dump
with varnishlog -w in the future (which I really hope we do), we could
consider making this a VSL record itself that is not subject to
vsl_reclen, querying or filtering, for the needs of dynamic VSLs, so
that whenever something changes VSL clients see an update. It would
then be varnishlog -w's job to build such a synthetic record in the
VSL file before dumping actual logs.


Dridi