Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: only check stderr for protocol upgrading #121

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

lucab
Copy link
Owner

@lucab lucab commented Nov 4, 2022

This changes connected_to_journal() behavior to only check stderr
against $JOURNAL_STREAM for protocol upgrading.

Protocol upgrading assumes that all the logging happens on stderr.
For that purpose, it is not relevant whether stdout is connected
to journal.

@lucab
Copy link
Owner Author

lucab commented Nov 4, 2022

/cc @lunaryorn

This is the result of a recent conversation I had with systemd folks. There is a summary of that in coreos/go-systemd#410 (comment).

In short, my understanding of "native protocol upgrading" was wrong: in reality the systemd approach assumes that all logging happens on stderr, so it shouldn't be concerned with stdout. Instead, the stdout/stderr checks are two separate lower level primitives which can be used independently.

Copy link
Collaborator

@swsnr swsnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks. I agree that changing the connected_to_journal function is a good idea, but I'm less sure about these helpers; do we really need those? Would someone really like to check stdout so frequently that they need an explicit helper? Shouldn't we just expose JournalStream directly, so everyone can check whatever stream they like if they really have to?

@lucab
Copy link
Owner Author

lucab commented Nov 4, 2022

Yes, I don't think there is going to be much usage for the stdout helper, it is mostly there for symmetry with stderr.
If you prefer we can drop them both and consider adding them later if anybody has a need for those.

JournalStream is already public: https://docs.rs/libsystemd/0.5.0/libsystemd/logging/struct.JournalStream.html
So in theory the same stdour/stderr helpers can be already implemented directly in consumers.

Overall, it sounds like I can just only change connected_to_journal() in this PR, agreed?

Speaking of public interfaces, do you have a need for public JournalStream::parse() and JournalStream::from_env()? It looks like those can be turned into private methods, only leaving from_fd() and from_env_default() as public methods, right?

@swsnr
Copy link
Collaborator

swsnr commented Nov 4, 2022

@lucab Yes, I'd just change connected_to_journal() 🙂 If someone needs to access the plumbing here they can open a ticket (or even just copy the code, it's pretty simple after all 🙂 )

This changes `connected_to_journal()` behavior to only check stderr
against `$JOURNAL_STREAM` for protocol upgrading.

Protocol upgrading assumes that all the logging happens on stderr.
For that purpose, it is not relevant whether stdout is connected
to journal.
@lucab lucab changed the title logging: split journal stream checks from protocol upgrading logging: only check stderr for protocol upgrading Nov 4, 2022
swsnr added a commit to swsnr/gnome-search-providers-jetbrains that referenced this pull request Nov 4, 2022
That's how systemd wants us to do this, see lucab/libsystemd-rs#121
swsnr added a commit to swsnr/gnome-search-providers-jetbrains that referenced this pull request Nov 4, 2022
That's how systemd wants us to do this, see lucab/libsystemd-rs#121
@lucab lucab merged commit df596db into master Nov 4, 2022
@lucab lucab deleted the ups/logging-journal-stream branch November 4, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants