Follow logs

[itowlson]

Fixes #380.

For discussion and feedback.

The current UI is:

• spin up ... --follow comp1 --follow comp2 to follow specifically those components
• spin up ... --follow-all to follow all components

The current behaviour is the followed component stdout gets redirected to Spin stdout and component stderr to Spin stderr, unless the executor chooses to do things differently. (WAGI, for example, follows only stderr.)

I feel like there should be scope for simplification, and I'm not quite sure whether I'm passing the follow selection around in an appropriate way. Also there is a clippy lint that I'm not sure how to address. Input eagerly requested.

Note: This needs a rebase but I was thinking I'd hold off on that until I've had feedback on whether this is along the right lines - no point ploughing through a rebase if we decide to redesign it!

[lann]

I wonder if it would make sense to push all this logging through tracing, then implement "following" as a custom tracing subscriber. I'm thinking forward to other things we might want to do with these component logs like exposing them through hippo; it seems likely that tracing's ecosystem could simplify that for us.

[lann]

The docs suggest using any:

ids.any(|id| id == component_id)

[itowlson]

@lann re using the tracing crate - yes, I had been wondering whether to use tracing or to dump straight to the output streams. My initial feeling was that the goal here was specifically "follow" so you wouldn't necessarily want an alternate subscriber, you want to see it in your UI; but it's definitely an open question and I should have flagged it. Thanks for bringing it up!

[lann]

you wouldn't necessarily want an alternate subscriber, you want to see it in your UI

I think seeing things in your UI is a perfect example of a subscriber!

[radu-matei]

Potential follow-up.

How would we feel about prepending the component name in the followed logs?

For example, following the logs of the Spin docs from this repo, it is unclear in which component a log line originated.

Not found: /favicon.ico

Cannot read file: cannot open /image/nonexistent.png

Caused by:
    No such file or directory (os error 44)

For example, Docker Compose's output (source):

#docker-compose -f docker-compose-all.yml logs -t|head
Attaching to oauth_web_1, oauth_core_1, oauth_core-ok_1, oauth_core-fb_1, oauth_oauth_1, oauth_fill-sqls-web_1, oauth_fill-sqls-oauth_1, oauth_web-redis_1, oauth_core-redis_1, oauth_core-mysql_1, oauth_oauth-mysql_1, oauth_web-mysql_1
core_1 | 2016-10-07T06:22:10.603699134Z [nodemon] 1.10.2
web_1 | 2016-10-07T06:23:26.013419543Z [06:23:26] Using gulpfile /usr/src/app/gulpfile.js

(definitely not blocking this PR)

[radu-matei]

This looks great, thank you!

LGTM

[itowlson]

This, like Lann's tracing question, is interesting. At the moment we avoid having to do anything with the output byte stream. Using tracing or prepending component IDs would need us to parse the stream and deal with it on a line-by-line basis. The PR as it stands already uses a LineWriter to buffer and emit as lines (so that concurrent output doesn't get mixed up on the same line): we could certainly see whether we can wrap that further to modify the line as it's written, or to explicitly detect things as lines and emit them to trace.

It's certainly attractive to be able to distinguish logs in this way. But I can also see a case for not messing with the output of code that doesn't belong to us and we have no idea what it does!

This would be good to resolve before we merge this, because it might mean changes at the structural level. We'd probably also want to run it past the commenters on #438 to make sure it wouldn't affect the way they want to do things (I think they plan to inject their own streams so it shouldn't be an issue but).

[radu-matei]

Ah, good point, I will reference the comment in #438.

[lann]

I'm fine punting on tracing integration for now. This PR addresses the problem well.

ref #449

[lann]

We ran into this before and punted by...removing an unnecessary argument. 😐

I'm fine leaving this for now since these HttpExecutors are ripe for a broader refactoring. Should probably reword the comment though.