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

Optionally add EOL (fix #1) #3

Merged
merged 5 commits into from
Apr 27, 2017
Merged

Conversation

cmwd
Copy link
Contributor

@cmwd cmwd commented Apr 26, 2017

No description provided.

lib/rfc3164.js Outdated
@@ -3,6 +3,7 @@
const moment = require('moment-timezone')
const stringify = require('fast-safe-stringify')
const through2 = require('through2')
const EOL = require('os').EOL
Copy link
Member

Choose a reason for hiding this comment

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

Should this be configurable, e.g. { newline: '\n' }? Or can we safely assume that \n will always be an acceptable terminator? I think so since syslog is a Unix tool and \n is the standard EOL for Unix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's safe to agree on \n

Copy link
Member

Choose a reason for hiding this comment

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

Then let's just have { newline: false } and { newline: true }. When true, just append a '\n'. I'm not sure what would happen if a carriage return or carriage return + line feed were added, as using the OS EOL would do in certain circumstances.

@cmwd
Copy link
Contributor Author

cmwd commented Apr 26, 2017

After testing the latest release, I found I was wrong about newline. Logs are sent to the Papertail and they are parsed well (probably somehow I missed it during e2e tests or Papertail fixed it). The only remaining thing is that after sending a syslog through pino-socket they are swallowed. With a new line, they are sent back to stdout. I'm not sure if it's an issue of this library or pino-socket.

This works for me, but we can still add this feature because it's increasing readability. What do you think?

@jsumners
Copy link
Member

The only remaining thing is that after sending a syslog through pino-socket they are swallowed. With a new line, they are sent back to stdout. I'm not sure if it's an issue of this library or pino-socket.

What do you mean "sending a syslog"? pino-socket is meant to accept Pino logs and transform them into syslog lines. Commit d2dba06 prevents any logs that are not processed from being printed to stdout. If that is not desired behavior, then a separate issue should be filed.

@cmwd
Copy link
Contributor Author

cmwd commented Apr 26, 2017

I'm talking about the following scenario: node ./index.js | pino-syslog | pino-socket. AFAIK it should do three things: transform pino log to syslog format, send it to the receiver and to the stdout. Currently, the log is sent to the receiver but pino-socket is not passing it to the stdout. However, with appended newline in pino-syslog it works as expected.

Anyway, it sounds more like a problem of pino-socket, so I'm going to open an issue in its repository.

@jsumners jsumners merged commit 2a2723b into pinojs:master Apr 27, 2017
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.

2 participants