Subject: Re: [RFC PATCH 3/3] trace2: add a schema validator for trace2 events

From: Josh Steadmon <[email-masked]>
Date: Wed, 12 Jun 2019 09:23:41 -0700

On 2019.06.12 15:28, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 12 2019, Josh Steadmon wrote:
> 
> > trace_schema_validator can be used to verify that trace2 event output
> > conforms to the expectations set by the API documentation and codified
> > in event_schema.json (or strict_schema.json). This allows us to build a
> > regression test to verify that trace2 output does not change
> > unexpectedly.
> 
> Does this actually work for you? As seen in my code at
> https://public-inbox.org/git/[email-masked]/ our
> test suite emits various lines of JSON that aren't even validly encoded,
> so I can't imagine we're passing any sort of proper parser validatior,
> let alone a schema validator.

Yes, it seems that gojsonschema (and its dependencies) are not very strict about
encoding. I also had an alternate Python implementation, and it failed to parse
lines that were not properly encoded. I just had that version print out a
warning with the number of failed decodings. I believe it was ~20 out of 1.7M
events.

> In terms of implementation I think it would make sense to have a *.sh
> wrapper for this already, then we could test via prereqs if we have some
> of the existing validators (seems there's a list at
> https://json-schema.org/implementations.html) and e.g. run a dummy test
> against some small list of git commands, and then you could also pass it
> an env variable with "here's the trace file" so you could do:
> 
>     GIT_TRACE2_EVENT=/tmp/git.events prove <all testss> && VALIDATE_THIS=/tmp/git.events ./<that new test>.sh
> 
> And it would validate that file, if set.

The problem with the existing validators is that they expect each file to be a
complete JSON entity, whereas the trace output is one object per line. You can
of course loop over the lines in a shell script, but in my testing this approach
took multiple hours on the full test suite trace output, vs. 15 minutes for the
implementation in this patch.