jue89 / node-systemd-journald

Native bindings to journald
MIT License
28 stars 10 forks source link

Allow the syslog identifier to be passed without setting process.title #7

Closed bryanburgers closed 7 years ago

bryanburgers commented 7 years ago

process.title is limited, according to the node docs.

The process.title property returns the current process title (i.e. returns the current value of ps). Assigning a new value to process.title modifies the current value of ps.

Note: When a new value is assigned, different platforms will impose different maximum length restrictions on the title. Usually such restrictions are quite limited. For instance, on Linux and OS X, process.title is limited to the size of the binary name plus the length of the command line arguments because setting the process.title overwrites the argv memory of the process. Node.js v0.8 allowed for longer process title strings by also overwriting the environ memory but that was potentially insecure and confusing in some (rather obscure) cases.

It would be awesome if there was a way to specify the syslog identifier without mucking with process.title.

One idea would be to allow

const log = require('systemd-journald')
log.identifier = 'my-identifier';
log.debug(...)

I did some very light testing on one machine, and I was able to set the syslog identifier with the following code:

log.debug('My message', { syslog_identifier: 'my-identifier' })

which makes me think it might be as easy as checking if log.identifier is defined and, if it is, adding fields.SYSLOG_IDENTIFIER = log.identifier, but I didn't test that.

Thoughts?

jue89 commented 7 years ago

Hey Bryan!

Thanks for your thoughts and PR! I am delighted that other people are participating to this module :)

I actually discovered this problem before and finally named my node processes in the systemd unit files. But I know this is just a solution for daemons. And not a very good one as well.

I can confirm that your proposed solution works. But (Don't get me wrong!) I don't like the way how the identifier ist set. I think it is not a good habit to hold a state globally in the module. It may interfere if it is used by several other modules independently.

When I first discovered that process.title is not the best solution to set the identifier, I thought about a breaking change to the API (resp. releasing version 2.0.0). This way every module using node-systemd-journald can hold their very own instance. See my example code:

const Jorunald = require( 'systemd-journald' );

// Create an instance of Journald for logging.
// The parameter holds an object containing default fields. They may be overwritten by logging calls.
// Of course it can have other default fields than just the syslog_identifier.
const log = new Journald( { syslog_identifier: 'foobar' } );

log.debug( ... );

What do you think?

bryanburgers commented 7 years ago

Ha! I actually had a very similar example to that in my initial issue, but then removed it because I wasn't sure if you'd like a breaking version change. I like that direction.

Also, I am doing systemd daemons, and would be curious to see how you set it in your systemd file.

jue89 commented 7 years ago

I don't have problems with breaking changes at all. I try do my very best to stick with semantic versioning. If everybody else would do that, we all do not have to concern about breaking changes. But before we do these changes, I'm going to warn the winston-systemd guys - they are requiring the lastest version of this module and ignore semantic versioning ;)

According to the systemd.exec docu just add SyslogIdentifier=foobar to the [Service] section.

jue89 commented 7 years ago

Implementation in PR #8

bryanburgers commented 7 years ago

I'm surprised the SyslogIdentifier works when using this module. https://lists.freedesktop.org/archives/systemd-devel/2015-June/032816.html seems to suggest that SyslogIdentifier only works when outputting to stdout or stderr, not when using sd_journal_(print|send).

I couldn't get it to work on systemd 229 (Ubuntu 16.04.1).

jue89 commented 7 years ago

Sry, wasn't feeling well the last days. Merge will happen today.

Yes, you are right. I was wrong. Tested it and failed, too.