eonpatapon / mpDris2

MPRIS V2.1 support for mpd
GNU General Public License v3.0
214 stars 46 forks source link

Don't timestamp messages when logging to systemd journal #122

Closed ferdnyc closed 5 years ago

ferdnyc commented 5 years ago

The systemd journal already includes timestamps, making them redundant in the message content

grawity commented 5 years ago

Does this remove timestamps also from stdout (in debug mode)?

ferdnyc commented 5 years ago

Does this remove timestamps also from stdout (in debug mode)?

That's a good question. Yes, it appears that it does. Hrm.

Since the journal capture is basically just the stderr output, that's not easy to solve. ... I could add a command-line switch to turn off timestamps, and update the unit file to include that argument?

ferdnyc commented 5 years ago

@grawity PR updated to add a new -n / --no-timestamps flag, and to use it when launching from the systemd unit file.

grawity commented 5 years ago

Looks good, although it still feels like logging should take care of such things somehow (timestamp for stdout, <prio> tag for systemd)... I guess it does not have built-in journald support?

ferdnyc commented 5 years ago

Looks good, although it still feels like logging should take care of such things somehow

I had that same feeling, TBH.

I guess it does not have built-in journald support?

Not in the default set of logging handlers, at least. Closest thing is the Syslog handler, which could leverage journald's syslog compatibility.

Initially it felt sloppy to me, writing new code to take advantage of legacy compatibility. But now I'm wondering if journald really has new logging protocols of it's own, or if it's just a new storage method? Maybe syslog is actually the right way to communicate with it. I'll look into that.

ferdnyc commented 5 years ago

Hrm. It seems there is a native protocol. (It even supports additional fields that correspond to other things logging captures, like current-function / -method context.) And SystemD even ships with default Python bindings for systemd.journal. ...They just aren't tied in to the logging module.

Well, then I just don't even know what to do with that.

grawity commented 5 years ago

How about replacing it with a mode to write messages via the Syslog handler, then? It would also work with journald, and would at least preserve the message priorities (which is possible with systemd stdout but a bit clumsy).

ferdnyc commented 5 years ago

Actually, I realized syslog won't work. We're running as a user unit, so we want our logs to go to the user journal — not the system journal.

ferdnyc commented 5 years ago

(Unless maybe there's a user syslog socket in /run/ that we could target, rather than /dev/log.)

grawity commented 5 years ago

Actually, I realized syslog won't work. We're running as a user unit, so we want our logs to go to the user journal — not the system journal.

Doesn't matter; there is only one journald in either case, and it sorts out logs based on which UID sent messages to the socket, not based on which socket was used.

ferdnyc commented 5 years ago

it sorts out logs based on which UID sent messages to the socket, not based on which socket was used.

Oh, that's smart. Neat. I'll play around with that a bit, then.

We'll probably still need a command-line flag, which would direct logging to syslog instead of stderr.

ferdnyc commented 5 years ago

...They just aren't tied in to the logging module.

I was wrong. There is (today) a systemd.journal.JournalHandler, which is a subclass of logging.handler. So, yay.