journald: add support for specifying syslog facility #2425

oherrala · 2022-12-23T13:32:40Z

Motivation

Syslog facility is supported in JournalD this allows using that mechanism. This way JournalD can provide same fields as syslog messages: Syslog identifier and facility.

Solution

The Syslog facility is optional and if it is not specified, it is not included in JournalD message. The Syslog facility can be set with Subscriber::with_syslog_facility()method using e.g.libc crate's LOG_* constants: Subscriber::with_syslog_facility(libc::LOG_DAEMON).

hawkw · 2022-12-29T19:31:13Z

It looks like rustfmt needs to be run on this PR: https://github.com/tokio-rs/tracing/actions/runs/3766015080/jobs/6443426140

Would you mind running cargo fmt and committing the results? Thanks!

hawkw

overall, this looks correct. i had a few minor suggestions, but i didn't notice any significant issues with the implementation.

it would be nice to get a review from @Ralith (tracing-journald's original author) as well.

tracing-journald/src/lib.rs

hawkw · 2022-12-29T19:38:35Z

tracing-journald/src/lib.rs

+/// message. This lets the configuration file specify that messages from
+/// different facilities will be handled differently.
+#[derive(Copy, Clone)]
+pub enum SyslogFacility {


do we know that the list of syslog facilities will always be the same across various operating systems and libcs? should this maybe have a #[non_exhaustive] on it just in case?

journald so far is Linux-only I think? And I expect Linux's definitions to stay stable.

hawkw · 2022-12-29T19:39:57Z

tracing-journald/src/lib.rs

+    /// security/authorization messages
+    Auth,
+    /// security/authorization messages (private)
+    Authpriv,
+    /// clock daemon (cron and at)
+    Cron,
+    /// system daemons without separate facility value
+    Daemon,
+    /// ftp daemon
+    Ftp,
+    /// kernel messages (these can't be generated from user
+    /// processes)
+    Kern,
+    ///reserved for local use
+    Local0,
+    ///reserved for local use
+    Local1,
+    ///reserved for local use
+    Local2,
+    ///reserved for local use
+    Local3,
+    ///reserved for local use
+    Local4,
+    ///reserved for local use
+    Local5,
+    ///reserved for local use
+    Local6,
+    ///reserved for local use
+    Local7,
+    /// line printer subsystem
+    Lpr,
+    /// mail subsystem
+    Mail,
+    /// USENET news subsystem
+    News,
+    /// messages generated internally by syslogd(8)
+    Syslog,
+    /// generic user-level messages
+    User,
+    /// UUCP subsystem


nit: generally, the documentation comments for enum variants are capitalized:

Suggested change

/// security/authorization messages

Auth,

/// security/authorization messages (private)

Authpriv,

/// clock daemon (cron and at)

Cron,

/// system daemons without separate facility value

Daemon,

/// ftp daemon

Ftp,

/// kernel messages (these can't be generated from user

/// processes)

Kern,

///reserved for local use

Local0,

///reserved for local use

Local1,

///reserved for local use

Local2,

///reserved for local use

Local3,

///reserved for local use

Local4,

///reserved for local use

Local5,

///reserved for local use

Local6,

///reserved for local use

Local7,

/// line printer subsystem

Lpr,

/// mail subsystem

Mail,

/// USENET news subsystem

News,

/// messages generated internally by syslogd(8)

Syslog,

/// generic user-level messages

User,

/// UUCP subsystem

/// Security/authorization messages

Auth,

/// Security/authorization messages (private)

Authpriv,

/// Clock daemon (`cron` and `at`)

Cron,

/// System daemons without separate facility value

Daemon,

/// FTP daemon

Ftp,

/// Kernel messages (these can't be generated from user

/// processes)

Kern,

/// Reserved for local use

Local0,

/// Reserved for local use

Local1,

/// Reserved for local use

Local2,

/// Reserved for local use

Local3,

/// Reserved for local use

Local4,

/// Reserved for local use

Local5,

/// Reserved for local use

Local6,

/// Reserved for local use

Local7,

/// Line printer subsystem

Lpr,

/// Mail subsystem

Mail,

/// USENET news subsystem

News,

/// Messages generated internally by `syslogd(8)`

Syslog,

/// Generic user-level messages

User,

/// UUCP subsystem

tracing-journald/src/lib.rs

hawkw · 2022-12-29T19:48:01Z

tracing-journald/src/lib.rs

+pub enum SyslogFacility {
+    /// security/authorization messages
+    Auth,
+    /// security/authorization messages (private)
+    Authpriv,
+    /// clock daemon (cron and at)
+    Cron,
+    /// system daemons without separate facility value
+    Daemon,
+    /// ftp daemon
+    Ftp,
+    /// kernel messages (these can't be generated from user
+    /// processes)
+    Kern,
+    ///reserved for local use
+    Local0,
+    ///reserved for local use
+    Local1,
+    ///reserved for local use
+    Local2,
+    ///reserved for local use
+    Local3,
+    ///reserved for local use
+    Local4,
+    ///reserved for local use
+    Local5,
+    ///reserved for local use
+    Local6,
+    ///reserved for local use
+    Local7,
+    /// line printer subsystem
+    Lpr,
+    /// mail subsystem
+    Mail,
+    /// USENET news subsystem
+    News,
+    /// messages generated internally by syslogd(8)
+    Syslog,
+    /// generic user-level messages
+    User,
+    /// UUCP subsystem
+    Uucp,


i believe that an enum variant's value can be assigned to a constant of the enum's repr type (i had to check, see this playground). i think we should be able to write something like this:

Suggested change

pub enum SyslogFacility {

/// security/authorization messages

Auth,

/// security/authorization messages (private)

Authpriv,

/// clock daemon (cron and at)

Cron,

/// system daemons without separate facility value

Daemon,

/// ftp daemon

Ftp,

/// kernel messages (these can't be generated from user

/// processes)

Kern,

///reserved for local use

Local0,

///reserved for local use

Local1,

///reserved for local use

Local2,

///reserved for local use

Local3,

///reserved for local use

Local4,

///reserved for local use

Local5,

///reserved for local use

Local6,

///reserved for local use

Local7,

/// line printer subsystem

Lpr,

/// mail subsystem

Mail,

/// USENET news subsystem

News,

/// messages generated internally by syslogd(8)

Syslog,

/// generic user-level messages

User,

/// UUCP subsystem

Uucp,

#[repr(i32)]

pub enum SyslogFacility {

/// security/authorization messages

Auth = libc::LOG_AUTH as i32,

/// security/authorization messages (private)

Authpriv = libc::LOG_AUTHPRIV as i32,,

/// clock daemon (cron and at)

Cron = libc::LOG_CRON as i32,

/// system daemons without separate facility value

Daemon = libc::LOG_DAEMON as i32,

/// ftp daemon

Ftp = libc::LOG_FTP as i32,

/// kernel messages (these can't be generated from user

/// processes)

Kern = libc::LOG_KERN as i32,

///reserved for local use

Local0 = libc::LOG_LOCAL0 as i32,

///reserved for local use

Local1 = libc::LOG_LOCAL1 as i32,

///reserved for local use

Local2 = libc::LOG_LOCAL2 as i32,

///reserved for local use

Local3 = libc::LOG_LOCAL3 as i32,

///reserved for local use

Local4 = libc::LOG_LOCAL4 as i32,

///reserved for local use

Local5 = libc::LOG_LOCAL5 as i32,

///reserved for local use

Local6 = libc::LOG_LOCAL6 as i32,

///reserved for local use

Local7 = libc::LOG_LOCAL7 as i32,

/// line printer subsystem

Lpr = libc::LOG_LPR as i32,

/// mail subsystem

Mail = libc::LOG_MAIL as i32,

/// USENET news subsystem

News = libc::LOG_NEWS as i32,

/// messages generated internally by syslogd(8)

Syslog = libc::LOG_SYSLOG as i32,

/// generic user-level messages

User = libc::LOG_USER as i32,

/// UUCP subsystem

Uucp = libc::LOG_UUCP as i32,

if we did this, we could remove the match in put_facility and replace it with just let value = facility as i32;, which would be nice. however, this does rely on these values being a c_int (castable to an i32) on all platforms' libcs, so we should make sure it wouldn't cause any problems to do this.

tracing-journald/src/lib.rs

mguender · 2023-06-05T09:15:50Z

Is this pull request actively worked on? I would be very happy to use SYSLOG_FACILTY for filtering journald logs...

oherrala · 2023-06-05T16:25:55Z

Is this pull request actively worked on? I would be very happy to use SYSLOG_FACILTY for filtering journald logs...

I'd be happy to work towards getting this merged.

Ralith · 2023-06-05T17:57:31Z

tracing-journald/src/lib.rs

+            // libc definitions are bitshifted left by 3, but journald uses
+            // values without bitshift.
+            writeln!(buf, "SYSLOG_FACILITY={}", facility >> 3).unwrap();


Hmm, this seems like a potential footgun. If you Google around for facility definitions and pass one manually rather than via libc, it would be silently mangled, and passing a value different than what you'd use to filter for it is confusing.

@Ralith Do you have any advice how we could go forward?

No, I'm wondering if anyone else has ideas before we set this in stone. Both obvious options have significant drawbacks. How do other journald bindings handle it?

No much luck with my Googling of various implementations:

Python has "enum": https://github.com/mosquito/logging-journald/blob/3cae0d0fb637a8a6873e21a35ce3bf164f61642a/logging_journald.py#L18-L43

Go's zerolog doesn't support facility: https://github.com/rs/zerolog/blob/master/journald/journald.go

Go ssreg's journald doesn't support facility but one can probably just inject SYSLOG_FACILITY as a string to journald message: https://github.com/ssgreg/journald/blob/acf944d4c13d9c8751bd5f7931bbb8dc1b80ace4/README.md?plain=1#L107-L112

PHP journald one can probably just add SYSLOG_FACILITY string to be sent: https://github.com/systemd/php-systemd/blob/master/README.md?plain=1#L51

Node node-systemd-journald doesn't support facility: https://github.com/jue89/node-systemd-journald

Node sd-journald probably supports setting SYSLOG_FACILITY as a string: https://github.com/sargun/sd-journald

Hmm, would it make more sense to punt on abstracting this entirely, and let people supply arbitrary key/value pairs to be appended globally? Particularly as this seems to be a really niche/archaic parameter.

The syslog facility is optional and if it is not specified, it is not included in JournalD message. The SyslogFacility enum contains all the fields specified in syslog(3) for facility and numbers are mapped using libc's definition.

Finomnis · 2023-08-31T11:51:49Z

Any reason the current change code doesn't contain the initial enum approach?

I'd like to take over this merge request, but I'm not sure if I understand its current design decisions.

Ralith · 2023-08-31T17:33:09Z

My current feeling is that this sort of niche (I think?) case is best addressed by allowing the user to specify arbitrary key-value pairs. That saves us from needing complex/opinionated APIs.

Finomnis · 2023-08-31T17:45:16Z

My current feeling is that this sort of niche (I think?) case is best addressed by allowing the user to specify arbitrary key-value pairs. That saves us from needing complex/opinionated APIs.

Is there an open PR for it already? Or should we start one?

Ralith · 2023-09-01T04:33:51Z

I'm not aware of any attempt. It should be straightforward enough; if you're motivated, please feel welcome!

oherrala · 2023-09-01T07:18:58Z

Is this PR still needed? I've not been given any actionable directions to go towards so this has been left hanging without any decisions.

Finomnis · 2023-09-01T13:23:11Z

I opened #2708, which should replace this PR, based on what was discussed above.

EDIT: After thinking about it some more, I'm actually opposed to closing this PR and would instead prefer to rework+merge it.

Finomnis · 2023-09-01T14:08:46Z

One problem I can see with allowing arbitrary values is that users can specify values that already get set by other options, like SYSLOG_IDENTIFIER and .with_syslog_identifier(). There aren't many settings left - should we simply add them all manually? That would avoid that aliasing.

I personally think that the best way forward is to proceed with adding with_syslog_facility (as this change originally intended), but with a u32/u16/u8 parameter that does not get >>3'd. This would allow compatibility with definitions like https://wiki.archlinux.org/title/Systemd/Journal#Facility and the official specification.

The fact that the values are <<3 is very specific to the journal C API, where the log level and the facility are combined in a single value. This isn't the case for us, so I oppose using the libc constants.

Finomnis · 2023-09-01T14:25:33Z

@oherrala I opened a merge request to your branch with how I would continue this PR.

Also, you should at some point merge master into your change, otherwise clippy will issue some warnings.

What's your opinion, @Ralith? Also including my previous comment about aliasing, and about continuing this PR.

I kind of need this feature for work, so I'm very interested in getting this merged. Otherwise this crate is not viable for our usecase. (we use local0/local1 to redirect logs onto specific log locations)

## Motivation It's currently not possible to customize how messages will get send to journald. This became apparent in #2425, where first a specific API got designed, but then it was decided that users should not get restricted in only a subset of fields, but should be able to simply choose by themselves what fields get set with what values. So in a sense, this is the successor/rework of #2425. ## Solution Allow custom fields to be set in tracing-journald. ## Open Questions - [x] How should we deal with fields that also get supplied by other options? For example, setting `SYSLOG_IDENTIFIER` here and also setting `.with_syslog_identifier()` will send said field twice, potentially with differing values. Is that a problem? - Answer: No, this is not a problem. Closes #2425

It's currently not possible to customize how messages will get send to journald. This became apparent in #2425, where first a specific API got designed, but then it was decided that users should not get restricted in only a subset of fields, but should be able to simply choose by themselves what fields get set with what values. So in a sense, this is the successor/rework of #2425. Allow custom fields to be set in tracing-journald. - [x] How should we deal with fields that also get supplied by other options? For example, setting `SYSLOG_IDENTIFIER` here and also setting `.with_syslog_identifier()` will send said field twice, potentially with differing values. Is that a problem? - Answer: No, this is not a problem. Closes #2425

oherrala requested review from hawkw, davidbarsky and a team as code owners December 23, 2022 13:32

hawkw reviewed Dec 29, 2022

View reviewed changes

hawkw requested a review from Ralith December 29, 2022 19:53

Ralith requested changes Dec 29, 2022

View reviewed changes

tracing-journald/src/lib.rs Outdated Show resolved Hide resolved

tracing-journald/src/lib.rs Outdated Show resolved Hide resolved

oherrala force-pushed the syslog-facility branch 2 times, most recently from 3419e27 to cc402e6 Compare December 29, 2022 21:23

oherrala force-pushed the syslog-facility branch 2 times, most recently from 5f5be01 to 2ea1cd6 Compare January 12, 2023 14:02

Ralith reviewed Jun 5, 2023

View reviewed changes

journald: add support for specifying syslog facility

50bade0

The syslog facility is optional and if it is not specified, it is not included in JournalD message. The SyslogFacility enum contains all the fields specified in syslog(3) for facility and numbers are mapped using libc's definition.

oherrala force-pushed the syslog-facility branch from 2ea1cd6 to 50bade0 Compare July 22, 2023 12:24

Finomnis mentioned this pull request Sep 1, 2023

tracing-journald: allow custom journal fields #2708

Merged

1 task

hawkw closed this in 672dfdf Sep 5, 2023

oherrala deleted the syslog-facility branch September 6, 2023 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

journald: add support for specifying syslog facility #2425

journald: add support for specifying syslog facility #2425

oherrala commented Dec 23, 2022 •

edited

Loading

hawkw commented Dec 29, 2022

hawkw left a comment

hawkw Dec 29, 2022

oherrala Dec 29, 2022

hawkw Dec 29, 2022

hawkw Dec 29, 2022

mguender commented Jun 5, 2023

oherrala commented Jun 5, 2023

Ralith Jun 5, 2023

oherrala Jun 5, 2023

Ralith Jun 6, 2023

oherrala Jun 7, 2023

Ralith Jun 8, 2023

Finomnis commented Aug 31, 2023

Ralith commented Aug 31, 2023

Finomnis commented Aug 31, 2023

Ralith commented Sep 1, 2023

oherrala commented Sep 1, 2023

Finomnis commented Sep 1, 2023 •

edited

Loading

Finomnis commented Sep 1, 2023 •

edited

Loading

Finomnis commented Sep 1, 2023 •

edited

Loading

journald: add support for specifying syslog facility #2425

journald: add support for specifying syslog facility #2425

Conversation

oherrala commented Dec 23, 2022 • edited Loading

Motivation

Solution

hawkw commented Dec 29, 2022

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mguender commented Jun 5, 2023

oherrala commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Finomnis commented Aug 31, 2023

Ralith commented Aug 31, 2023

Finomnis commented Aug 31, 2023

Ralith commented Sep 1, 2023

oherrala commented Sep 1, 2023

Finomnis commented Sep 1, 2023 • edited Loading

Finomnis commented Sep 1, 2023 • edited Loading

Finomnis commented Sep 1, 2023 • edited Loading

oherrala commented Dec 23, 2022 •

edited

Loading

Finomnis commented Sep 1, 2023 •

edited

Loading

Finomnis commented Sep 1, 2023 •

edited

Loading

Finomnis commented Sep 1, 2023 •

edited

Loading