Implement verifier stats parser

[aibor]

With ebpf.LogLevelStats the verifier outputs stats about the verification process. It would be nice to have a parser that provides the metrics as a dedicated data structure.

Up to now the format was quite stable. Although it was introduced with the note that "the format is expected to change in the future", it hasn't changed since it was added. Even if it changes in the future, it would be possible to support multiple formats.

The kernel bpf selftests have a parser for those stats. It would be easy to adapt it: a separate helper function that takes a verifier log string and outputs a dedicated stats data structure.

Would you be open to accept a PR for such a verifier stats parser? If so, where would be the best place for it? Should it be added to prog.go?

[ti-mo]

I'd be open to it, we just need to figure out a good API for both successful (Program.VerifierLog) and unsuccessful (VerifierError) cases. A VerifierStats struct could be embedded into VerifierError.

For Program, I'm not sure what to do. The VerifierLog field is kind of a one-off; all other functionality like prog info is behind methods. I'd probably choose the same approach today, e.g. an unexported Program.verifierStats pointer with a Program.VerifierStats() accessor.

Alternatively, we might want to opt for Program.VerifierStats() (*VerifierStats, error) that returns any errors encountered during parsing of the last line, since we shouldn't fail loading the Program if we fail to parse the log. Now I think of it, the same should probably apply to VerifierError, especially given the last line will be missing or incomplete if the log buffer is too small, and we don't want to interfere with the kernel's errno.

Also, I'd take a more defensive and granular approach to the parsing itself, for example: processed %d insns (limit %d) max_states_per_insn %d total_states %d peak_states %d mark_read %d should be parsed using individual scanfs, like:

fmt.Sscanf(log, "processed %d insns (limit %d)", processed, limit)
fmt.Sscanf(log, "max_states_per_insn %d", max_states)

.. etc., so that when things do change (likely adding things at the end or in the middle), we degrade gracefully and still provide some useful output.

[aibor]

Splitting all the individual chunks into separate scans sounds like a great idea. A custom error that indicates incomplete information that is passed along with the parsed data might be sensible, so the caller can still consume the incomplete information in this case.

As for the general API: My first idea was to provide the parser as a separate function. Since the user must load the program with the correct log level to get verifier stats, it seems reasonable to me that the user uses the stats parser explicitly as well. Also, with a separate helper function, it can be called with the value of either Program.VerifierLog or VerifierError.Log. If a method for Program is desired as well, it could just call the helper directly with the value of VerifierLog.

But thinking about it, another idea would be to have a dedicated VerifierLog type. It is just a string type but provides a additional methods like parsing VerifierStats. This new type can than be used for Program.VerifierLog and VerifierError.Log. The new type may own other formatting helpers that are now integrated in internal.ErrorWithLog as well.

[ti-mo]

Yep, I think the VerifierLog type is a good idea. We could take this as an opportunity to harmonize the APIs, something like:

type VerifierLog string

func (l *VerifierLog) Lines() []string
func (l *VerifierLog) Stats() (VerifierStats, error)

type Program struct {
  VerifierLog VerifierLog
  ...
}

type VerifierError struct {
  // Deprecated: use (*VerifierError.)VerifierLog.Lines() instead.
  Log []string // Populated by VerifierLog.Lines() for backwards-compat.
  VerifierLog VerifierLog
  ...
}

We can remove VerifierError.Log in the next release +1.

[aibor]

Sounds good.

So, what's the best way to implement this? Is it fine to introduce the new type with the stats parser included in a single issue/MR, or should it be split?

[ti-mo]

@aibor Feel free to do this in one PR as long as the commit history is clear.