Event streaming #183

nyarly · 2019-10-21T23:56:13Z

Adds a lorri stream_events_ subcommand that receives 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.

Included are example jq|xargs style scripts to make use of the event streams.

t184256 · 2019-10-27T15:13:35Z

Just a user report: I've tried that and it worked fine for me.

nyarly · 2019-11-08T16:51:50Z

Closes #145 #78 #79 (I believe)

src/build_loop.rs

Profpatsch

Can you rebase on master and make sure all commits build by themselves? I tried to rebase, but most of the commits seem to have build errors.

You have to change all ::module::Foo forms to crate::module::Foo, because we updated to Rust 2018.

nyarly · 2019-11-15T22:56:03Z

I'll rebase and squash to a single commit when I get a chance.

nyarly · 2019-11-28T19:21:28Z

Merged with master, removed Heartbeats.

As each new listener joins, the daemon sends a SectionEnd event to the existing ones. This results in a BrokenPipe for clients that have exited, and keeps the thread and connection consumption under control.

nyarly · 2020-01-28T20:12:13Z

As a workaround, I've implemented a server deadline in lorri stream_events_ and echo NewListener to all existing clients.

Profpatsch · 2020-01-28T20:13:43Z

@Profpatsch I'm starting more an more to think that the only way to manage this is periodic heartbeats over the socket to trigger varlink noticing that the socket closed.

yeah, if that’s the case go for it. My request for removal came from before the decision was made to throw the socket code away and switch to varlink. Sorry for the overhead.

curiousleo

I think this is solid code, and the feature is useful. Thanks for putting in the effort.

First review round done. +866 -48 line changes makes me nervous, but (1) you can just straight delete 50 lines (see comments) and (2) most of that seems to be in TryFrom declarations, which is benign code.

src/build_loop.rs

src/daemon.rs

src/build_loop.rs

src/com.target.lorri.varlink

src/ops/stream_events.rs

curiousleo · 2020-01-31T09:29:27Z

src/ops/stream_events.rs

+
+    let mut snapshot_done = false;
+    let (tx, rx) = unbounded();
+    let recycle = tx.clone();


tx_clone perhaps? recycle as a name doesn't carry much intuition, at least for me.

I was hoping to indicate the reason for this channel - to feed a first event back into our local channel that's tee'd from the daemon monitor.

That's fine. I find naming channels particularly difficult - I find myself looking at their uses a lot to figure out what they're actually meant to do. You could document this channel's purpose in a comment if you think that would be useful and possible to do succinctly. If not, don't - your call :)

src/ops/stream_events.rs

curiousleo · 2020-01-31T09:35:27Z

src/project/roots.rs

@@ -19,7 +19,7 @@ pub struct Roots {

 /// A path to a gc root.
 #[derive(Hash, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
-pub struct RootPath(PathBuf);
+pub struct RootPath(pub PathBuf);


Not super elegant IMO, but I think RootPath should just go away at some point, so I don't mind breaking the abstraction here.

example/notify.sh

src/cli.rs

curiousleo · 2020-01-31T14:34:04Z

src/com.target.lorri.varlink

+# indicating that the stream of events is now "live."
+method Monitor() -> (event: Event)
+
+type Event (


Varlink does not provide proper enums or gRPC-like oneof. But I think that events can be modelled in a slightly more "type-safe" way. What do you think about this sort of structure:

type Event { kind: (section_end, started), section_end: SectionEnd?, # set iff kind == section_end started: Started?, # set iff kind == started # ... }

where SectionEnd, Started, etc. have only non-optional fields?

I think this might be interesting to experiment, but I think I need to put it into a followup PR.

nyarly · 2020-01-31T17:51:08Z

@curiousleo Thank you for the comprehensive and thoughtful review.

Some of your suggestions are already addressed in my local branch. I'll be pushing those changes presently.

Of the remainder, I like all of them and intend to implement.

I am concern is that the pace of development outstrips my available time. This week has been merging current master back to this branch. If we could merge a sub-optimal version, I'm happy to commit to followup PRs.

curiousleo · 2020-02-03T07:54:25Z

src/cli.rs

+    StreamEvents_(StreamEvents_),
+
+    /// Upgrade Lorri
+    #[structopt(name = "self-upgrade", alias = "self-update")]


The alias "self-update" was removed in a recent PR. If I had to guess, I'd say that re-introducing it here was not intentional?

curiousleo · 2020-02-03T07:54:57Z

src/cli.rs

-    #[structopt(name = "self-upgrade")]
+    /// (plumbing) Ask the lorri daemon to report build events as they occur
+    #[structopt(
+        name = "internal__stream_events",


Nice. I appreciate your adopting the new conventions of the surrounding code here!

curiousleo · 2020-02-03T07:57:59Z

src/ops/stream_events.rs

+    Compat(String),
+}
+
+/// See the documentation for lorri::cli::Command::Shell for more


Sorry, that wasn't a helpful comment! I just wanted to point out that

See the documentation for lorri::cli::Command::Shell for more details.

is perhaps not the best documentation for the stream_events main function.

curiousleo · 2020-02-03T08:01:52Z

src/ops/stream_events.rs

+
+    let mut snapshot_done = false;
+    let (tx, rx) = unbounded();
+    let recycle = tx.clone();


That's fine. I find naming channels particularly difficult - I find myself looking at their uses a lot to figure out what they're actually meant to do. You could document this channel's purpose in a comment if you think that would be useful and possible to do succinctly. If not, don't - your call :)

curiousleo · 2020-02-03T08:03:05Z

src/ops/stream_events.rs

+    }
+
+    for event in rx.iter() {
+        debug!("Received"; "event" => format!("{:#?}", &event));


One last format! that can be replaced by slog's ?&event syntax.

curiousleo · 2020-02-03T08:03:58Z

tests/daemon/main.rs

@@ -17,7 +17,7 @@ use std::time::{Duration, Instant};
 pub fn start_job_with_ping() -> std::io::Result<()> {
    // TODO: this code is a mess because Daeomon is not
    // nicely abstracted yet.
-
+    //


Super-nit: this change looks superfluous.

curiousleo · 2020-02-03T08:46:23Z

@nyarly thanks for the work you put in here, and for being very responsive to code review.

I am concern is that the pace of development outstrips my available time. This week has been merging current master back to this branch.

That's totally fair. The last couple of weeks were particularly frenetic. Expect fewer sweeping changes from now on.

If we could merge a sub-optimal version, I'm happy to commit to followup PRs.

I'm on board with that idea, with one big caveat I'll expand on in my next comment.

The two items of I'd like to see follow-up on are:

Event streaming #183 (comment): a PR that brings back notify.sh and prompt.sh and explains how users can leverage event streams
Event streaming #183 (comment): a PR that makes it harder to use the event streams varlink protocol the wrong way by making it more type-safe (note that the varlink protocol is considered an implementation details, so changes to it are not considered breaking changes even after a 1.0 release)

I think that you have the most context on this and are probably the best person to do these things, but if you'd like help or would like to work on them together, we can definitely talk about that.

curiousleo · 2020-02-03T09:45:55Z

Problem: implementation details exposed in the output

I think there is an issue with re-using existing structs the way it is done right now in this PR. Here's an example:

$ lorri internal__stream_events | jq
{
  "started": {
    "nix_file": {
      "shell": "/home/leo/Code/lorri/shell.nix"
    },
    "reason": {
      "project_added": {
        "shell": "/home/leo/Code/lorri/shell.nix"
      }
    }
  }
}
{
  "completed": {
    "nix_file": {
      "shell": "/home/leo/Code/lorri/shell.nix"
    },
    "result": {
      "output_paths": {
        "shell_gc_root": "/home/leo/.cache/lorri/gc_roots/2043910c8519bf079838568f54a73839/gc_root/shell_gc_root"
      }
    }
  }
}

What is clear here is that this exposes

lorri/src/lib.rs

Lines 50 to 55 in c38e0f0

    
           pub enum NixFile { 
        
               /// A .nix file which describes a shell environment 
        
               Shell(PathBuf), 
        
               /// A .nix file which describes a list of services 
        
               Services(PathBuf), 
        
           }

lorri/src/watch.rs

Lines 34 to 44 in c38e0f0

    
           pub enum Reason { 
        
               /// When a project is presented to Lorri to track, it's built for this reason. 
        
               ProjectAdded(NixFile), 
        
               /// When a ping is received. 
        
               PingReceived, 
        
               /// When there is a filesystem change, the first changed file is recorded, 
        
               /// along with a count of other filesystem events. 
        
               FilesChanged(Vec<PathBuf>), 
        
               /// When the underlying notifier reports something strange. 
        
               UnknownEvent(DebugMessage), 
        
           }

lorri/src/builder.rs

Lines 284 to 287 in c38e0f0

    
           pub struct OutputPaths<T> { 
        
               /// Shell path modified to work as a gc root 
        
               pub shell_gc_root: T, 
        
           }

and other structs and enums to users of this command - basically all those that now have Serialize derives.

This means that any change to any of those structs or enums is likely to change the output format of event_stream. I think it is likely that we'll want the freedom to change those structs in the future. I also think that the way things work at the moment in this PR, it is easy to make a change to any of them and not realise that this changes the format of event_stream's output.

Context: v1.0 and backwards compatibility

We are working towards a lorri v1.0 release: #306. From that point on, the versioning scheme will be used to determine the next version.

I believe that @Profpatsch may have planned for event_stream to be a non-internal command.

If event_stream is to be labelled non-internal, then after we release v1.0, any change to any of the structs or enums that can appear in event_stream's output will be a "change to the machine-readable output of a non-internal command", which means it's a breaking change, which means it requires a major version bump. That's not the end of the world, but the way things stand right now, I think that any internal change is likely to touch at least one of the enums and structs exposed via event_stream, so my fear is that we'd race through lots of major versions very quickly.

Proposal: expose an explicit interface

My proposal is thus to separate the internal representation of things like build failure reasons from how they are represented in the output of the event_stream command. This allows us to make changes to the internal representations without changing the output format. It also makes it harder to accidentally change the output format of event_stream.

I see two options for how the explicit interface can be defined:

We can write a set of Rust structs and enums that serve the particular purpose of being serialised into the output format for event_stream.
We can use Varlink type definitions to describe the output format. Describing a JSON schema is something Varlink is pretty good at, in my opinion. People who want to build tooling for lorri using event_stream may find it easier to read the Varlink schema than Rust structs and enums. Finally, changes to this definition would be very obvious, this would pretty much rule out making output format changes by accident. The downside is that we would now have two sets of Varlink definitions: an internal one for the daemon <> lorri clients communication, and this new one describing the event_stream output. They should probably go in separate files and be documented appropriately to make sure future collaborators understand their different purposes.

I hope this all makes sense. To summarise: event_stream exposes types that I think of as implementation details. As a result, we'll either change the output of event_stream a lot (perhaps unwittingly), or we'll slow down development because we try not to change the output of event_stream, which means preserving many types as they are. Neither is a good place to be, in my opinion, even ignoring the versioning issue. I think the way to get around this problem is to separate the representation of the user-facing output from the internal representation.

curiousleo · 2020-02-03T13:49:07Z

#320 (comment) describes my proposal for our next steps with this work.

Profpatsch requested review from Profpatsch and grahamc October 22, 2019 13:00

nyarly added 11 commits November 13, 2019 17:43

Adding 'Reason' data for Started events

45cf22e

Message passing skeleton for streaming events to clients

50e7812

Removing listener channels as they expire

9fcb3b3

Added stream_events_ command

59ef6bf

Live events and example notify script

63641a5

Snapshot output and example prompt script

3621c19

Readable serialization for log lines

98b8d54

Resolving rebase conflicts

f3f6936

Finishing

9acce20

Nicer error when daemon not available

dc603c9

Rebase cleanup

7bee0f0

Profpatsch reviewed Nov 14, 2019

View reviewed changes

src/build_loop.rs Outdated Show resolved Hide resolved

Profpatsch suggested changes Nov 14, 2019

View reviewed changes

Profpatsch mentioned this pull request Nov 18, 2019

Show journal logs on a single line #224

Closed

nyarly added 3 commits November 28, 2019 09:42

Moving from Heartbeat to infinite client timeout

f260176

Merge remote-tracking branch 'upstream/master' into stream_events

5f7d4fe

Cleaning up

109ec1c

nyarly requested a review from Profpatsch November 28, 2019 19:21

fmt

a227d9a

curiousleo mentioned this pull request Dec 10, 2019

Use varlink for internal RPCs #253

Merged

5 tasks

nyarly mentioned this pull request Jan 4, 2020

Command to block until lorri is guaranteed to provide the newest environment #203

Open

Merge remote-tracking branch 'upstream/master' into stream_events

f859e04

Fix for thread exhaustion

6b57420

As each new listener joins, the daemon sends a SectionEnd event to the existing ones. This results in a BrokenPipe for clients that have exited, and keeps the thread and connection consumption under control.

nyarly added 3 commits January 31, 2020 00:07

Merge remote-tracking branch 'upstream/master' into stream_events

fbb15ab

Transition to using BuildError

f1ebd02

Downcasing NixFile JSON

767a016

curiousleo reviewed Jan 31, 2020

View reviewed changes

example/notify.sh Outdated Show resolved Hide resolved

curiousleo reviewed Jan 31, 2020

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

curiousleo reviewed Jan 31, 2020

View reviewed changes

nyarly added 2 commits January 31, 2020 08:57

Merge branch 'master' into stream_events

5bcfcf1

Simplifying log line handling

e49bf56

nyarly added 7 commits January 31, 2020 09:58

Removing example scripts for time being

b116a15

Review response

4bc7006

Merge remote-tracking branch 'upstream/master' into stream_events

98f136e

Post merge cleanup

e464081

Format and linting

5f4bac2

Nicer names for varlink kinds

398b9ed

Improving comments on varlink interface

5563ab2

nyarly mentioned this pull request Feb 1, 2020

Event Streaming (take 2) #320

Merged

3 tasks

curiousleo reviewed Feb 3, 2020

View reviewed changes

curiousleo mentioned this pull request Feb 4, 2020

Event stream: follow-up #324

Open

4 tasks

grahamc closed this Feb 4, 2020

curiousleo mentioned this pull request Mar 9, 2020

Conversions for JSON DTO #346

Closed

3 tasks

nyarly mentioned this pull request Mar 23, 2020

[RFC] Stabilize the Event fields as they are #360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event streaming #183

Event streaming #183

nyarly commented Oct 21, 2019

t184256 commented Oct 27, 2019

nyarly commented Nov 8, 2019

Profpatsch left a comment

nyarly commented Nov 15, 2019

nyarly commented Nov 28, 2019

nyarly commented Jan 28, 2020

Profpatsch commented Jan 28, 2020

curiousleo left a comment

curiousleo Jan 31, 2020

nyarly Jan 31, 2020

curiousleo Feb 3, 2020

curiousleo Jan 31, 2020

curiousleo Jan 31, 2020

nyarly Jan 31, 2020

nyarly commented Jan 31, 2020

curiousleo Feb 3, 2020

curiousleo Feb 3, 2020

curiousleo Feb 3, 2020

curiousleo Feb 3, 2020

curiousleo Feb 3, 2020

curiousleo Feb 3, 2020 •

edited

Loading

curiousleo commented Feb 3, 2020

curiousleo commented Feb 3, 2020

curiousleo commented Feb 3, 2020

Event streaming #183

Event streaming #183

Conversation

nyarly commented Oct 21, 2019

t184256 commented Oct 27, 2019

nyarly commented Nov 8, 2019

Profpatsch left a comment

Choose a reason for hiding this comment

nyarly commented Nov 15, 2019

nyarly commented Nov 28, 2019

nyarly commented Jan 28, 2020

Profpatsch commented Jan 28, 2020

curiousleo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousleo Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

curiousleo commented Feb 3, 2020

curiousleo commented Feb 3, 2020

Problem: implementation details exposed in the output

Context: v1.0 and backwards compatibility

Proposal: expose an explicit interface

curiousleo commented Feb 3, 2020

curiousleo Feb 3, 2020 •

edited

Loading