Rework for syslog version 5 #5

argv-minus-one · 2020-05-26T10:40:55Z

This PR reworks slog-syslog for version 5 of the syslog crate. It is a breaking change.

The syslog crate underwent extensive API refactoring between versions 3 and 5. This PR rewrites much of slog-syslog to fit the new API. It is partially compatible with previous versions of slog-syslog (examples/syslog-unix.rs still works without modification), but the shape of Streamer3164 in particular is quite different.

There are also some new features:

All SyslogBuilder settings are now optional and have sensible defaults. Simply running SyslogBuilder::new().start() will yield a usable Drain (if there is a syslog daemon on the local machine).
SyslogBuilder no longer requires a hostname to be supplied when choosing a transport (TCP, UDP, Unix socket). Instead, the hostname is a separate setting (SyslogBuilder::hostname), and if none is provided, it's discovered (if possible) using the hostname crate.
Streamer3164 no longer contains a mutex, and is not thread-safe (that is, it is no longer Sync). This improves performance when used with slog-async, which doesn't need the underlying Drain to be thread-safe. slog::Logger does require the underlying Drain to be thread-safe, so SyslogBuilder::start now returns a Streamer3164 wrapped in a Mutex, and there's now a SyslogBuilder::start_single_threaded method that returns an unwrapped Streamer3164 instead.
Key-value pairs attached to logging records are now, by default, formatted in a style that resembles RFC 5424 STRUCTURED-DATA. Their formatting can be customized by implementing the MsgFormat3164 trait.

The minimum supported Rust version of syslog version 5, and therefore of slog-syslog with this PR, is 1.31. This is newer than Slog's MSRV of 1.26.

dpc · 2020-05-26T19:48:59Z

Cargo.toml

@@ -14,6 +14,6 @@ readme = "README.md"
 path = "lib.rs"

 [dependencies]
+hostname = "0.3"


Maybe already bump the version of this crate if this is a breaking change?

Ok. I pushed another commit to a bump the version.

argv-minus-one · 2020-05-27T01:24:17Z

I've made several changes and squashed them together. For complete history, see my syslog-v5-dev branch.

The `syslog` crate underwent extensive API refactoring between versions 3 and 5. This commit rewrites much of `slog-syslog` to fit the new API. It is partially compatible with previous versions of `slog-syslog` (`examples/syslog-unix.rs` still works without modification), but the shape of `Streamer3164` in particular is quite different. There are also some new features: * All `SyslogBuilder` settings are now optional and have sensible defaults. Simply running `SyslogBuilder::new().start()` will yield a usable `Drain` (if there is a syslog daemon on the local machine). * `SyslogBuilder` no longer requires a hostname to be supplied when choosing a transport (TCP, UDP, Unix socket). Instead, the hostname is a separate setting (`SyslogBuilder::hostname`), and if none is provided, it's discovered (if possible) using the `hostname` crate. * `Streamer3164` no longer contains a mutex, and is not thread-safe (that is, it is no longer `Sync`). This improves performance when used with slog-async, which doesn't need the underlying `Drain` to be thread-safe. `slog::Logger` does require the underlying `Drain` to be thread-safe, so `SyslogBuilder::start` now returns a `Streamer3164` wrapped in a `Mutex`, and there's now a `SyslogBuilder::start_single_threaded` method that returns an unwrapped `Streamer3164` instead. * Key-value pairs attached to logging records are now, by default, formatted in a style that resembles RFC 5424 STRUCTURED-DATA. Their formatting can be customized by implementing the `MsgFormat3164` trait.

dpc · 2020-06-03T20:14:11Z

lib.rs

+        let mut guard = self.io.borrow_mut();
+        let logger = &mut *guard;
+
+        // We need to smuggle errors out of the `Display` implementation for `msg`, below. The `Display::fmt` method takes `&self` rather than `&mut self`, so the obvious way of accomplishing this (`let mut msg_format_result: syslog::Result<()>`) will not work because the closure cannot write to any captured variable (including `msg_format_result`).


Is what's going here better than thread_local reusable buffer that was here bofore?

No. Your thread-local reusable buffer was faster than this implementation.

But there's no way to avoid it and still use syslog version 5. It now uses std::fmt::format on the input, which allocates a new buffer and writes the whole message into it. If we tried to use a reusable buffer like before, syslog would just copy the contents of that reusable buffer into the newly-allocated one anyway, negating the performance benefit.

A thread-local reusable buffer would be a good idea, but it would need to be done in the syslog crate rather than here. Unfortunately, that crate seems abandoned, judging from all the old, unanswered PRs.

So, maybe it would be best to reject this PR and stick with the old implementation. I don't know.

To be honest, several days after I submitted this PR, I gave up on using the syslog crate entirely. It turns out that it uses the wrong protocol for local syslog over a Unix-domain socket, and it's pretty much impossible to implement correctly because the details of that protocol vary greatly between systems. Some (macOS) use a completely different protocol instead of the BSD local syslog protocol. Some (OpenBSD) don't even use a socket! So, I wrote a syslog logger based on libc calls for the sloggers crate instead.

I don't know if you care, but if you're not already a cargo-crev user you should probably fill a negative package review with unmaintained: true, and your findings in it.

From the rest of it, it seems that the correct way to go is to take over / have a new syslog-like crate that is libc based and re-use it both in sloggers and here? There could be more users that would benefit from it? Or not?

We can leave this PR hanging for now, I guess.

I don't know if you care, but if you're not already a cargo-crev user you should probably fill a negative package review with unmaintained: true, and your findings in it.

I'm sorry, but I don't feel comfortable interfering with someone else's project like that.

From the rest of it, it seems that the correct way to go is to take over / have a new syslog-like crate that is libc based and re-use it both in sloggers and here? There could be more users that would benefit from it? Or not?

Probably not. The POSIX syslog API is pretty simple and easy to use from Rust. Most of the code I contributed to sloggers was just an adapter for Slog's different log record format (it has key-value pairs, whereas plain syslog doesn't) and different logger lifecycle (there is only one syslog logger per process, whereas Slog allows there to be many Drains at the same time).

But it might be useful to Slog users if they could construct a syslog-based Drain without all the decoration that sloggers stacks on top (it automatically applies slog-async, etc). So, I could extract the code I contributed to sloggers and use that as a new slog-syslog. Should I do that?

But it might be useful to Slog users if they could construct a syslog-based Drain without all the decoration that sloggers stacks on top (it automatically applies slog-async, etc). So, I could extract the code I contributed to sloggers and use that as a new slog-syslog. Should I do that?

Exactly. Please do.

I'm sorry, but I don't feel comfortable interfering with someone else's project like that.

If you can't get a response from the maintainer in reasonable time, it's a clear sign the package is not maintained, especially if there are problem with it. Rather than thinking about the author, think about all the people that could lose their time and maybe even more than time, building on top of stuff that has problems. Anyway ... I don't even fully understand the issues, so it's all up to you. I just think it's worthwhile sharing opinions and finding to help the community.

dpc

LGTM. Good work.

As discussed in PR slog-rs#5, this is based on the [syslog backend that I wrote for sloggers][1] and does not use the `syslog` crate. It is now very different from both slog-syslog 0.12 and PR slog-rs#5. Differences from slog-rs#5 include: * Uses the POSIX syslog API through the `libc` crate. (See “design and rationale” in src/lib.rs, starting at line 52, for details.) This means: * Only works on Unix-like platforms. * Only sends log messages to the local syslog daemon, not to a remote server. * Formats log messages into a reusable thread-local buffer, as in slog-syslog 0.12. * The `Drain` implementation is now called `SyslogDrain` instead of `Streamer3164`, since local syslog doesn't use the RFC3164 protocol. * If the `serde` feature is enabled, logging settings can be loaded from a configuration file. Minimum supported Rust version is 1.27.2, or if the `serde` feature is enabled, 1.31.0. Doc-tests require 1.28.0. Works (except doc-tests) on 1.26.2 if you run the following commands first: ``` cargo generate-lockfile cargo update --package lazy_static --precise 1.3.0 cargo update --package serde --precise 1.0.58 cargo update --package serde_derive --precise 1.0.58 ``` [1]: sile/sloggers#34

eun-ice · 2020-07-13T09:38:29Z

Thank you for this, I use this on production Linux machines without any problems.

It does not work on macOS however, but this is also true for the original slog-syslog.

thread '<unnamed>' panicked at 'slog::Fuse Drain: Error(Format, State { next_error: Some(Os { code: 55, kind: Other, message: "No buffer space available" }), backtrace: InternalBacktrace })', /Users/XXXX/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/src/libstd/macros.rs:16:9

I spent a bit of time trying to solve it, but since we use macOS only on dev machines I stopped spending time on this.

It works like a charm on Linux though

argv-minus-one · 2020-07-13T09:59:00Z

@eun-ice As discussed above, this code (i.e. PR #5) doesn't actually work correctly even on Linux. It probably depends on the particular syslogd you're using, but if you use systemd-journald, you'll get malformed log entries if you use this code.

Did you try the code in PR #6? I'm not sure whether it'll work any better for you (I don't have a Mac on hand right now to test it with), but it's a completely different and hopefully more compatible implementation.

I apparently forgot to close this PR, so I'll go ahead and do that now.

eun-ice · 2020-07-13T16:29:48Z

@argv-minus-one thanks for the heads up, I actually started using and testing this branch before the discussion above happened.

dpc reviewed May 26, 2020

View reviewed changes

argv-minus-one force-pushed the syslog-v5 branch from 284003e to 0e9656a Compare May 27, 2020 01:22

argv-minus-one force-pushed the syslog-v5 branch from 0e9656a to f41b594 Compare May 27, 2020 03:23

dpc reviewed Jun 3, 2020

View reviewed changes

dpc approved these changes Jun 3, 2020

View reviewed changes

argv-minus-one mentioned this pull request Jun 19, 2020

Rework using the POSIX syslog API #6

Open

argv-minus-one closed this Jul 13, 2020

argv-minus-one deleted the syslog-v5 branch July 13, 2020 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework for syslog version 5 #5

Rework for syslog version 5 #5

argv-minus-one commented May 26, 2020

dpc May 26, 2020 •

edited

Loading

argv-minus-one May 26, 2020

argv-minus-one commented May 27, 2020

dpc Jun 3, 2020

argv-minus-one Jun 4, 2020

dpc Jun 4, 2020

argv-minus-one Jun 9, 2020

dpc Jun 9, 2020 •

edited

Loading

dpc left a comment

eun-ice commented Jul 13, 2020 •

edited

Loading

argv-minus-one commented Jul 13, 2020

eun-ice commented Jul 13, 2020

Rework for syslog version 5 #5

Rework for syslog version 5 #5

Conversation

argv-minus-one commented May 26, 2020

dpc May 26, 2020 • edited Loading

Choose a reason for hiding this comment

argv-minus-one May 26, 2020

Choose a reason for hiding this comment

argv-minus-one commented May 27, 2020

dpc Jun 3, 2020

Choose a reason for hiding this comment

argv-minus-one Jun 4, 2020

Choose a reason for hiding this comment

dpc Jun 4, 2020

Choose a reason for hiding this comment

argv-minus-one Jun 9, 2020

Choose a reason for hiding this comment

dpc Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

dpc left a comment

Choose a reason for hiding this comment

eun-ice commented Jul 13, 2020 • edited Loading

argv-minus-one commented Jul 13, 2020

eun-ice commented Jul 13, 2020

dpc May 26, 2020 •

edited

Loading

dpc Jun 9, 2020 •

edited

Loading

eun-ice commented Jul 13, 2020 •

edited

Loading