Re: [PATCH 1/2] Support publishing build events

[Clément Lassieur]

Hi Christopher,

This is a small review :)

Christopher Baines <address@hidden> writes:

> Add a table to store events, which have a type and a JSON blob. These can be
> used to record changes, this commit inserts events when new builds are
> created, and when the status of builds change.

Why is it a JSON blob?  I mean, why do we need it to be JSON if we use
JSON-STRING->SCM everytime we need to access it?  If it's just to
normalize the scheme object, I think it's already done by %SQLITE-EXEC.
It converts the scheme object to a string.  I can't see how a JSON
object is better than a string.

> The EventsOutbox table is then used to track when events have been sent
> out. This is done through the new cuirass-send-events script.

So the Events table will grow a lot.  And once its content has been sent
out, we don't use it anymore if I understand well.  Would it be possible
to just use the Events table, that would be emptied upon successful
send?  That way, the whole mechanism would be simpler, and that table
wouldn't grow.

Unless there is a need to fetch events that have already been sent out?
But in that case, why not fetch them from the Guix Data Service?

Now that I think about it, I don't understand the point the HTTP API
("build-events", "evaluation-events").  If events are sent from Cuirass
to the Guix Data Service, why would anyone need to fetch those events
from Cuirass?  Wouldn't it make more sense to fetch them from the Guix
Data Service?

Another thing: Cuirass instances that aren't bound to the Guix Data
Service will still have their Events table grow.  So it would be great
if DB-ADD-EVENT can be enabled through a Cuirass parameter.  And by
default it would be disabled.

> * Makefile.am (bin_SCRIPTS): Add bin/cuirass-send-events.
                                             ^
Could you please add it to .gitignore?

> +;;;
> +;;; Entry point.
> +;;;
> +
> +(define* (main #:optional (args (command-line)))
> +
> +  ;; Always have stdout/stderr line-buffered.
> +  (setvbuf (current-output-port) 'line)
> +  (setvbuf (current-error-port) 'line)
> +
> +  (let ((opts (getopt-long args %options)))
> +    (parameterize
> +        ((%program-name     (car args))
> +         (%package-database (option-ref opts 'database (%package-database)))
> +         (%package-cachedir
> +          (option-ref opts 'cache-directory (%package-cachedir))))
> +      (cond
> +       ((option-ref opts 'help #f)
> +        (show-help)
> +        (exit 0))
> +       (else
> +        (run-fibers
               ^
Why do we need to use a fiber, if there is only one?  A fiber is a
lightweight thread.  They are useful if we need to do several things in
the same time, but if there is only one thing to do, I don't understand
the point.

> diff --git a/src/cuirass/base.scm b/src/cuirass/base.scm
> index 143bc2e..e7c2597 100644
> --- a/src/cuirass/base.scm
> +++ b/src/cuirass/base.scm
> @@ -670,7 +670,14 @@ started)."
>                       (#:timestamp . ,cur-time)
>                       (#:starttime . 0)
>                       (#:stoptime . 0))))
> -        (db-add-build build))))
> +        (if (db-add-build build)
> +            (begin
> +              (db-add-event 'build
> +                            cur-time
> +                            `((#:derivation . ,drv)
> +                              (#:event       . scheduled)))
> +              drv)
> +            #f))))

Could this be moved into the DB-ADD-BUILD procedure, in database.scm?
This way everyting would be done at the same (lower) level.  The higher
level base.scm module doesn't need to know about it.  And it'd adds
consistency.

>    (define derivations
>      (filter-map register jobs))
> diff --git a/src/cuirass/database.scm b/src/cuirass/database.scm
> index 523165d..8cb7465 100644
> --- a/src/cuirass/database.scm
> +++ b/src/cuirass/database.scm

[...]

> +          (unless (eq? (changes-count db) 0)
                           ^
             (when (positive? (changes-count db))

> diff --git a/src/cuirass/send-events.scm b/src/cuirass/send-events.scm
> new file mode 100644
> index 0000000..2b7dd9c
> --- /dev/null
> +++ b/src/cuirass/send-events.scm
> @@ -0,0 +1,91 @@

[...]

> +(define* (send-events target-url
> +                      #:key (batch-limit 100))
> +  "Send up to BATCH-LIMIT events to TARGET-URL"
> +  (with-exponential-backoff-upon-error
> +   (lambda ()
> +     (let ((events-to-send
> +            (db-get-events-in-outbox batch-limit)))
> +       (unless (null? events-to-send)
> +         (let* ((body
              ^
             let
> +                 (object->json-string
> +                  `((items
> +                     . ,(list->vector
> +                         (map (lambda (event)
> +                                (let ((event-json
> +                                       (json-string->scm

[...]

> +(define* (with-exponential-backoff-upon-error f #:key (retry-number 1))
> +  "Run F and catch exceptions, retrying after a number of seconds that
> +increases exponentially."

Here, could you use common Guile conventions to write the docstring?
There are nice examples in ports.scm[1].  F would be a THUNK for
example.  And "call" would be used instead of "run".

Also, a small wrapper macro (as it is done with (@@ (guix gnu machine
ssh) with-roll-back)) would allow to avoid typing (lambda () ...) and
remove an indentation level.  You could prefix the procedure with
'call-', and add something like:

(define-syntax-rule (with-exponential-backoff-upon-error body ...)
  (call-with-exponential-backoff-upon-error
   (lambda ()
     body ...)))

(untested)

Thank you!
Clément

[1]: https://github.com/ilovezfs/guile/blob/master/module/ice-9/ports.scm