journal: add StderrIsJournalStream function#410
Conversation
ee7ac9c to
b259d42
Compare
|
Thanks for the PR! At the high level, I think this API should be renamed to avoid mentioning 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 |
|
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. |
|
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. |
b259d42 to
6b5ddca
Compare
|
I had an out-of-band chat with systemd upstream folks and I'm coming back with two main clarifications:
|
|
Given the above, the Regarding protocol upgrade detection, this is effectively the same logic as the stderr check. |
| // is present, but malformed, fstat syscall fails, etc. | ||
| // | ||
| // [Journal Native Protocol]: https://systemd.io/JOURNAL_NATIVE_PROTOCOL/#automatic-protocol-upgrading | ||
| func StderrIsJournalStream() (res bool, err error) { |
There was a problem hiding this comment.
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!
6b5ddca to
e87ae12
Compare
WGH-
left a comment
There was a problem hiding this comment.
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.
So you want me to add it then?
And if we introduce |
Yes, if you are up for it. Otherwise I'll handle that in a subsequent PR.
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. |
See discussion in coreos#410
e87ae12 to
78aa072
Compare
See discussion in coreos#410
bbbf9b7 to
1e01118
Compare
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
See discussion in coreos#410
3e5dce4 to
ea42749
Compare
|
So I added Additionally, I've written a somewhat complicated integration test for Unfortunately, it doesn't work as is on Ubuntu 16.04 due to |
ea42749 to
2a4f72f
Compare
That's totally fine. The old version there is just because we hadn't recently touched the CI setup. |
See discussion in coreos#410
2a4f72f to
b6075d1
Compare
See discussion in coreos#410
A test that will be added in a latter commit requires systemd-run --wait, which is not available on Ubuntu 16.04. DEBIAN_FRONTEND=noninteractive prevents hang on "Setting up tzdata" that became a problem on Ubuntu 20.04 (though something similar could happen on any Debian/Ubuntu version).
To ensure that StderrIsJournalStream properly works in real conditions, this test re-executes itself with systemd-run, and observes exit code and logged entries.
b6075d1 to
4ca6222
Compare
lucab
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot for your contribution!
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