journal: add StderrIsJournalStream function

[WGH-]

This function can be used for automatic protocol upgrade described in [1].

Both unit tests and runnable example are included. Only the latter requires systemd, as unit tests are self-sufficient, and only test that JOURNAL_STREAM environment variable is checked properly.

[1] https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading

[lucab]

Thanks for the PR!

At the high level, I think this API should be renamed to avoid mentioning stderr. In fact, the upgrade dance works the same way for stderr as well as for stdout, it's just that the systemd documentation is slightly deceiving on that front. See systemd/systemd#6800 for a concrete example of what I'm talking about.

In the Rust land this logic is implemented by https://docs.rs/libsystemd/0.5.0/libsystemd/logging/fn.connected_to_journal.html, so I think similarly here it could be called IsConnectedToJournal(). Also, it should check device and inode matching against stdout too (the final result is the OR of the two checks).

[WGH-]

As a counter-example, systemd components check only stderr when deciding whether to upgrade to the native journal protocol or not: https://github.com/systemd/systemd/blob/40c05a34595ed769ce676206f3c5de874f9a9234/src/basic/log.c#L217-L241 (this is not a part of e.g. public libsystemd, it's used internally by various systemd binaries)

In theory, stdout and stderr might be configured differently. StandardOutput=/ StandardError= can be changed explicitly, and shell-outs/shell pipelines are likely to make them different. It would be surprising if, for example, if stdout is configured to go to a file or a pipe, and application would redirect output (that it used to print to stdout) to the journal anyway (because stderr is still connected to the journal).

[lucab]

The upgrade protocol is a bit subpar from the point of view of an application, as it uses a single env-variable and allow mixing two different concerns into that.
But I think you have a good point about mixed-use scenarios, and the docs also highlight the same case.
I'll reach out privately to the systemd folks for some customary feedback, but I guess it would be good to expose checks for stdout/stderr separately. That will give more flexibility to applications/consumers.

[lucab]

I had an out-of-band chat with systemd upstream folks and I'm coming back with two main clarifications:

• probing logic should be performed separately for stdout and stderr. That is, it makes sense to have two helpers to check is *stdout* a journal stream? and is *stderr* a journal stream? separately.
• the protocol upgrading logic described in https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading is only concerned with stderr. That is, if and only if stderr is a journal stream then automatic protocol upgrading is feasible.

[lucab]

Given the above, the StderrIsJournalStream() method here is fine to stay.
Ideally it would be good to have a StdoutIsJournalStream() next to it.

Regarding protocol upgrade detection, this is effectively the same logic as the stderr check.
We can decide to either leave it out, or add something dedicated like IsConnectedToJournal() with a docstring to explain the protocol upgrade dance.
I have a slightly preference for the latter, as it helps capturing the nuances of this ticket discussion directly in the code.

[lucab]

I'd be happier if we could avoid named return values, as they can become the source of subtle bugs when capturing. Plain explicit return statements are better, thanks!

[WGH-]

Fixed

[WGH-]

I suppose I could try to write an integration tests that would run on CI. That would take some time, though. I'll try for a couple of days, and if I don't reply here, consider I've given up.

[WGH-]

Given the above, the StderrIsJournalStream() method here is fine to stay. Ideally it would be good to have a StdoutIsJournalStream() next to it.

So you want me to add it then?

Regarding protocol upgrade detection, this is effectively the same logic as the stderr check. We can decide to either leave it out, or add something dedicated like IsConnectedToJournal() with a docstring to explain the protocol upgrade dance. I have a slightly preference for the latter, as it helps capturing the nuances of this ticket discussion directly in the code.

StderrIsJournalStream()'s docstring already refers to https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading, and there's an example of said dance that gets rendered in godoc, isn't it enough?

And if we introduce IsConnectedToJournal(), is it going to be simply an alias for StderrIsJournalStream()?

[lucab]

So you want me to add it then?

Yes, if you are up for it. Otherwise I'll handle that in a subsequent PR.

there's an example of said dance that gets rendered in godoc, isn't it enough?
And if we introduce IsConnectedToJournal(), is it going to be simply an alias for StderrIsJournalStream()?

Yes, it would be a plain alias. Leaving this one up to you. If it already feels clear enough, I'm ok with. We can always add new helpers later if we change idea in the future.

[WGH-]

So I added StdoutIsJournalStream, and written some documentation as I saw fit. Please correct me if my understanding of its usefulness and limitations is wrong.

Additionally, I've written a somewhat complicated integration test for StderrIsJournalStream that executes itself as a transient systemd service to ensure it picks up JOURNAL_STREAM as it's initialized by systemd.

Unfortunately, it doesn't work as is on Ubuntu 16.04 due to systemd-run not supporting --wait. Instead of adding more workarounds to an already complicated test, I bumped distro versions instead. Ubuntu 16.04 is old and not supported anyway (except for commercial ESM). If that's inappropriate, let's discuss other options.

[lucab]

I bumped distro versions instead. Ubuntu 16.04 is old and not supported anyway (except for commercial ESM). If that's inappropriate, let's discuss other options.

That's totally fine. The old version there is just because we hadn't recently touched the CI setup.

stale

[lucab]

LGTM, thanks a lot for your contribution!