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: add function to check for journal connection #80

Merged
merged 2 commits into from
Oct 16, 2021
Merged

logging: add function to check for journal connection #80

merged 2 commits into from
Oct 16, 2021

Conversation

swsnr
Copy link
Collaborator

@swsnr swsnr commented Sep 3, 2021

connected_to_journal implements systemd's recommendation for processes
to check for a direct connection to the journal.

This allows services to automatically upgrade to the native journald
protocol for logging instead of relying on stdout being piped to
journald.

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 5, 2021

@lucab Would you mind taking a look?

If you need help maintaining this crate I could lend a hand 🙂

src/logging.rs Outdated Show resolved Hide resolved
src/logging.rs Outdated Show resolved Hide resolved
src/logging.rs Outdated Show resolved Hide resolved
@lucab
Copy link
Owner

lucab commented Oct 6, 2021

@lunaryorn thanks for the PR! I've left some comments in review.

I'm a bit ashamed to admit that I totally missed the initial notification for this. I recently had a look at this logging upgrade protocol and was starting to implement it myself. Thus I was positively surprised when I saw the ping this morning :)

In general yes, I'd be happy to have a co-maintainer here. Feel free to add yourself to Cargo.toml authors list, and I'll figure out the ACLs later on.

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 6, 2021

@lucab Please take another look. I've kept the simple top-level function for convenience, but re-implemented it on top of a new JournalStream struct which encapsulates the device/inode number pair. Let me know whether you like it better.

The change in activation.rs was a drive-by fix for a warning from Rustdoc. I hope that's okay 🙂

I'm happy to help. I'll make a separate MR for the Cargo.toml change which we can use for the whole onboarding process.

connected_to_journal implements systemd's recommendation for processes
to check for a direct connection to the journal.

This allows services to automatically upgrade to the native journald
protocol for logging instead of relying on stdout being piped to
journald.
src/errors.rs Outdated Show resolved Hide resolved
/// [systemd.exec(5)][1] for more information.
///
/// [1]: https://www.freedesktop.org/software/systemd/man/systemd.exec.html#Environment%20Variables%20Set%20or%20Propagated%20by%20the%20Service%20Manager
pub fn connected_to_journal() -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

As I mentioned, this API makes little sense for consumers.

A typical logging framework will use stdout OR stderr, and will need to know if the specific one they are using (or maybe both) is attached to journald. If so, that specific stream can be upgraded to journald native protocol (e.g. for structured logging).

This detection helper doesn't really work if logging is going to stderr, but only stdout is attached to journald. We should instead separate helper for checking stdout and stderr status separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This detection helper doesn't really work if logging is going to stderr, but only stdout is attached to journald.

I'm not sure I follow… I don't understand why it matters which stream the logging framework uses when not connected to the journal 🤔 and I don't understand why a program would only upgrade to journal logging if the journal stream happens to be bound to whatever standard stream it normally uses.

In my understanding it wouldn't matter whether stdout or stderr of my program is connected to the journal. All that matters is that the program is directly connected to the journal, one way or the other, because in this case it'd simply stop to log to stdout/stderr and instead write directly to the journal using the API of this crate. stdout/stderr would simply be unused then.

From this perspective it doesn't make any sense to check stdout/stderr independently because in either case you'd do the same: Setup a logger which writes directly to the journal.

Only if no stream is connected to the journal the program would use stdout/stderr to log.

Copy link
Owner

Choose a reason for hiding this comment

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

We may still be missing some bits in other parts of this library for the whole thing to work, and I may be misunderstanding some pieces of this. But as far as I understand the intended usage of this would be:

  1. logging framework is journald-aware and configured to log to stderr (for example)
  2. on startup it checks whether the stderr FD is connected to the journald socket:
    • NO: log messages will be written as plaintext lines to the FD, as usual
    • YES: log messages will be sent as binary datagrams to the FD, using the native protocol to support structured logging, large blobs, etc.
  3. same applies to stdout FD, but it will need to be checked separately as the two streams may be configured in an asymmetric way

I haven't played with this in practice yet, so I may have misunderstood some parts of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is different.

I don't think that stdout/stderr of processes started through systemd are directly connected to the journald socket, and I don't think you can actually send journald datagrams over these streams. Given that the journald socket is a datagram socket which uses a very specific data format how could that possibly work? You'd also not need $JOURNAL_STREAM in this case: You could just stat the journald socket path and compare its device/inode against the result of fstat(stdout) and fstat(stderr).

But on my system stat /run/systemd/journal/socket gives me a different device/inode than what I get in $JOURNAL_STREAM.

I guess what systemd gives you in $JOURNAL_STREAM is the device/inode pair of the anonymous pipe systemd created for the stdout/stderr of the process; this pipe ends somewhere inside systemd where the data gets split into lines, which get turned into native journal entries, and forwarded to the journald socket.

Now, I haven't looked this up in the systemd sources, and I haven't found any place where this is explicitly documented, but Stream Logging implies that there's some conversion going on, and also there's an explicit transport flag (see TRANSPORT field) for stdout which wouldn't make sense if stdout was just the journald socket, would it?

So, as I understand it, a program would check whether either stdout or stderr are connected to the journal. In this case it'd not use stdout/stderr at all, for logging at least. It could even close both of these FDs.

Instead it directly writes to journald, either with sd_journal_print and friends, or through the Unix socket API as implemented in this crate and as used by sd_journal_print. It wouldn't really matter which stream the program checks, because it's not relevant which stream is connected to the journal; it's just that some stream is connected to the journal, which tells you that logs likely end up in journald anyway, just unstructured, so you can just as well write to the journal yourself, in a structured way.


That said I can also add dedicated helpers to check stdout & stderr independently but I'd very much like to keep the simple function, because in my understanding of the whole setup it's the only function a program's likely to use.

And in any case, in the current version of this MR you could already do JournalStream::from_env_default() == JournalStream::from_fd(std::io::stdout()) if you're really interested in checking only stdout 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PS: Also note that there doesn't seem to be any sd_journal_print-like API which takes a file descriptor, so libsystemd won't let you write journald datagrams to stdout/stderr.

And sd_journal_stream_fd() seems to be the function that creates these special stdout/stderr file descriptors.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, very clear followup, indeed I now believe you are right and I misunderstood some part of this.
Thanks for your patience and sorry for the extra roundtrips!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're welcome 🙏

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 11, 2021

@lucab I removed the excessive error type, but I haven't changed the API yet, because I'd still discuss that.

@lucab lucab changed the title Add function to check for journal connection logging: add function to check for journal connection Oct 14, 2021
@lucab
Copy link
Owner

lucab commented Oct 14, 2021

@lunaryorn LGTM! I did tweak the permission on the repo, you should be able to merge this.

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 16, 2021

@lucab Great. I've pressed the button; do I need to update some changelog of sorts?

@swsnr swsnr merged commit 2a5ccda into lucab:master Oct 16, 2021
@lucab
Copy link
Owner

lucab commented Oct 16, 2021

No need to, I usually gather the list of changes at release time (e.g. #73).

@swsnr swsnr deleted the connected-to-journal branch October 16, 2021 20:25
@swsnr
Copy link
Collaborator Author

swsnr commented Oct 17, 2021

Cool thanks! Would you make another release with this merge request?

@lucab
Copy link
Owner

lucab commented Oct 18, 2021

@lunaryorn I'd use this occasion to make sure you have access everywhere.
Without any hurry, whenever you are comfortable feel free to grab https://github.com/crate-ci/cargo-release and send a release PR like the one above.

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 18, 2021

@lucab I'll do that; will take a bit though, as I'm not quite familiar with GPG, so I'll need to spent some time to get a key and setup the whole commit/tag signing infra, and I'm quite busy over the next two weeks. I'll make a mental note and check this out in November 🙂

@swsnr
Copy link
Collaborator Author

swsnr commented Oct 26, 2021

@lucab I did #86; does it look good?

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