Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

Event Streaming (take 2) #320

Merged
merged 7 commits into from
Feb 4, 2020
Merged

Event Streaming (take 2) #320

merged 7 commits into from
Feb 4, 2020

Conversation

nyarly
Copy link
Collaborator

@nyarly nyarly commented Feb 1, 2020

Closes #145 #78 #79.

This PR should replace #183 - if this is acceptable, please close that PR.

Overview

Adds a lorri internal__stream_events subcommand that returns a continuous stream json formatted representations of events. Each event is its own \n separated line. The command takes an option to toggle current state reporting vs. live events (or both), which is useful for the motivating use cases.

Checklist

  • Updated the documentation (code documentation, command help, ...)
  • Tested the change (unit or integration tests)
  • Amended the changelog in release.nix (see release.nix for instructions)

Reporting events to clients isn't useful without context. To provide
that context, we add the NixFile for the shell.nix that represents the
project to the Events.
We need to communicate within the daemon process when a new listener
joins, so that we can start fanning out build events to them. That
message needs to follow the same channels as the existing
build_loop::Events do, once they get to the daemon.

The solution is a wrapping enum (LoopHandlerEvent) that carries the
build_loop::Events as well as NewListener messages.
The stream_events message will require a new method in the varlink
interface to request a stream of build events, and then to receive that
stream.

Adds that method, implements it on the daemon varlink server, and builds
conversion traits between the varlink types and their lorri analogues.
Streamed events are designed to be consumed by machine parsers of JSON
(e.g. jq).  Therefore the events that are to be emitted need to be
serializable as JSON. This also includes serializing type names (for
instance) in snake case, and changing a couple of types to be easier to
serialize. Notably, error::BuildError::Exit carries an Option<i32>
instead of a process::ExitError.
Finally, with the machinery in place, the UI to deliver the streamed
event feature.
@curiousleo
Copy link
Collaborator

I left a little review (#183 (review)) and a couple of comments (#183 (comment), #183 (comment)) on #183 before seeing this new PR. I was going to suggest you rebase on top of master at some point before the PR gets merged, and that's what you seem to have done - great! :)

Just as a note: I have no issue at all with you force-pushing to the branch of an existing PR like #183, IMO there's no need from the reviewer's point of view to create a new PR. But I'm happy to follow along.

@curiousleo
Copy link
Collaborator

curiousleo commented Feb 3, 2020

I had another think about this and spoke to @grahamc. Here's what I suggest we do:

  1. We merge this PR (Event Streaming (take 2) #320), as it is right now. It adds a useful feature and people can start experimenting. It's also explicitly labelled as "internal" so people don't expect the output format to be set in stone.
  2. We address Event streaming #183 (comment) by separating out the representation of the output format from internal structs and enums.
  3. We address the two remaining items listed here: Event streaming #183 (comment)

I'm happy to help as much or as little as you like with each of those steps.

@nyarly how does that sound to you?

@curiousleo curiousleo mentioned this pull request Feb 3, 2020
@nyarly
Copy link
Collaborator Author

nyarly commented Feb 3, 2020

I had another think about this and spoke to @grahamc. Here's what I suggest we do:
...
@nyarly how does that sound to you?

@curiousleo That plan sounds great to me. I would really like to get a nicer version of the example scripts into contrib. The comments regarding an explicit interface make complete sense, and I'm happy to make inroads there as well. Both changes strike me as nicely scoped and straightforward.

@grahamc grahamc merged commit 7ab1e29 into target:master Feb 4, 2020
@curiousleo curiousleo mentioned this pull request Feb 4, 2020
4 tasks
@curiousleo
Copy link
Collaborator

curiousleo commented Feb 4, 2020

@nyarly wrote:

@curiousleo That plan sounds great to me. I would really like to get a nicer version of the example scripts into contrib. The comments regarding an explicit interface make complete sense, and I'm happy to make inroads there as well. Both changes strike me as nicely scoped and straightforward.

Excellent - congratulations, and thank you for your contributions so far!

I've created #324 to keep track of the follow-up items we've discussed.

@symphorien
Copy link

Is this command supposed to return constant output ?
I have a shell.nix failing to evaluate, and running lorri internal__stream_events --kind snapshot repeatedly sometimes includes a like about the failed evaluation, and sometimes does not.

@nyarly
Copy link
Collaborator Author

nyarly commented Jul 10, 2020

--kind snapshot reports the current understanding of the server of your builds - it should change over time as your builds do.

@symphorien
Copy link

I mean:
I have 1 shell.nix failing to evaluate, and 3 shells already built.
If I continuously run lorri internal__stream_events --kind snapshot without changing anything on the file system, the failing shell is only reported about half the time. That does not look right to me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicate that Lorri is done building for a project
4 participants