kenperkins / winston-papertrail

Papertrail transport for Winston
MIT License
203 stars 94 forks source link

Emitting 'error'-event #56

Open johnnycrab opened 8 years ago

johnnycrab commented 8 years ago

There is this line 228 in the main file in onErrored:

self.emit('error', err);

As the error is emitted, this causes the whole process to end, if the error isn't listened on elsewhere. This can lead to server crashes if Papertrail cannot be connected to. I thought maybe this should be mentioned in the docs. :)

matteocontrini commented 8 years ago

Actually this is how Node.js works. Refenrece.

troy commented 8 years ago

@johnnycrab: independent of the emit question, if you encountered a case where not being able to connect to the remote syslog server affected the app, it might be worth trying the current git master commit. Although Papertrail didn't write this library, and obviously can't test or vouch for every client library's connection handling, master includes fixes to some, maybe all, cases where being unable to connect affects the app.

johnnycrab commented 8 years ago

I'm on current master, but thanks for the note. :)

@matteocontrini I know that this is how node works, but I still thought it might be worth mentioning in the docs – as it is a pretty self-contained library, people like me don't think about listening to the socket error events (especially as it is emitted before the reconnection retries). I would at least add a line to the examples or something like that. I think it wouldn't hurt (–it would have helped people like me and some others on stackoverflow), but if not, nevermind and close.

freezy commented 8 years ago

So I've come across this as well.

This is the worst to debug. Without longjohn Node just crashes with a connection error, without any hint at all where the problem could be (stacktrace is a threeliner). This took me hours to find.

@matteocontrini: Are you seriously suggesting to keep your "Usage" example? This will crash the whole Node process in case something goes wrong.

For the record, there is nothing wrong with emitting your errors on your own stream. Or publish a library with a streaming API. But this is no streaming API. It's a drop-in replacement for a logging transport. This needs at least documentation. Also examples that don't crash the process would probably be a good idea.

dmiddlecamp commented 8 years ago

I submitted a PR that tries to be a bit more forgiving and clear with connection errors ( https://github.com/kenperkins/winston-papertrail/pull/61 ). Would this partially address this issue?

troy commented 8 years ago

FYI, @dmiddlecamp's #61 is now in master and will be part of a 1.0.4 release, probably in the next week. If anyone wants to pull master and see whether they can repro this, feel free.

troy commented 8 years ago

@dmiddlecamp's #61 has been released to NPM in 1.0.4. If anyone experiences this or believes they have, please make sure you're running >= 1.0.4; it may mitigate this issue to the point of eliminating it.