search

Rcomian / bunyan-rotating-file-stream

Is an endpoint for bunyan that lets you control how much disk space logs take.
Other
29 stars 15 forks source link

JSON parser error when using non-JSON log-format #6

Closed chris281 closed 8 years ago

chris281 commented 8 years ago

When using non-JSON log-format like with bunyan-format module the JSON.parse call in 'thresholdtrigger.js' method 'logWrite' throws an exception. It would be better to use something like: var log = {time: new Date().getTime()}; instead. By this the loss of information wouldn't be big (delay should be some ms) and the implementation is more robust.

Rcomian commented 8 years ago

Interesting, I didn't realise there was a non-json option for logging with bunyan. This could cause extra issues with the log writer - I'm pretty sure the json assumption is built-in. Thanks for raising this, I'll take a look this weekend.

chris281 commented 8 years ago

Thanks for your fast response. The bunyan-format modue creates a stream which gets the RotatingStream handed over, so it writes to the RotatingStream after reformating the log-entry. We didn't face issues regarding the writing of the log-entries yet, but only when the log-file rotates to a new one.

Rcomian commented 8 years ago

Yes, there's options like re-ordering the order that the json gets written as well, although they're weakly supported it'd probably crash in this situation - would be better to gracefully warn and not crash.

Rcomian commented 8 years ago

@chris281

I've wrapped the JSON parse in a try catch for the moment. There are a few weird situations I want to check out before removing support completely - basically I had situation where time could go backwards if you got the right combination of period triggers and threshold triggers going at the same time - that's why I put the date in there in the first place.

But it is a major smell having that JSON.parse there at all.

It'll be a rare issue if it's even still possible. This version (1.6) should work for your situation.

Could you check this works for me?

Cheers.

chris281 commented 8 years ago

Thanks for the fix, it works fine for our current needs.