search

robur-coop / builder

Scheduling build jobs on regular intervals, collecting artifacts
ISC License
13 stars 1 forks source link

[WIP] Add timestamp to builder-client observe output #20

Closed reynir closed 2 years ago

reynir commented 2 years ago

I experienced a builder job that timed out, but the client observe output said it had finished. Because there were no timestamps I couldn't tell the timings of the messages I had received in client observe when I came back to it later.

TODO:

hannesm commented 2 years ago

This looks good to me.

I don't think that "A flag in the client command line interface to enable/disable timestamps" is actually needed - the usefulness of timestamps is there (and these timestamps are also preserved for later already.

"Have backwards compatibility between server and client - perhaps with a new Observe_with_timestamp client command ?" I'd phrase this differently: an old client should continue to receive and be able to print Output. A new client (talking to a new server) should display the timestamp. Which also means: an old server talking to a new client will fail (since n = Builder.client_cmds is not true). A new server should be able to talk to new clients (indeed, Output_timestamped is constructed and send over the wire). A new server should be able to talk to an old client (here, the server needs to be lax about client_cmds (if 9 it should be fine) by emitting Output (no timestampts), so the old client has the possibility to decode them successfully.

hannesm commented 2 years ago

Thanks, from reading the code, the ASN grammar is indeed compatible -- since it is a sequence, now with an added optional element that does not need to be present. From the worker-server communication, the sequence without optional timestamp is being used.

reynir commented 2 years ago

I rebased on main (since #16 was merged) and added a commit to support older clients. This requires keeping track of client (and worker) version numbers. The code doesn't enforce client/worker protocol distinction. Ideally, I think it should be split up to enforce this.

hannesm commented 2 years ago

The code doesn't enforce client/worker protocol distinction. Ideally, I think it should be split up to enforce this.

Maybe we should open an issue about that, and leave it for future work? :) From a security perspective, it should not matter. But from a code complexity perspective it would be nice to distinguish the different state machines.