goodeggs / librato-node

A Node.js client for Librato (http://librato.com/)
MIT License
37 stars 26 forks source link

Add a failing test that a Librato failure does not crash the client #32

Closed kevinburkeshyp closed 7 years ago

kevinburkeshyp commented 9 years ago

If Librato returns a server error, the background worker crashes. This is probably not desired behavior, but I'm not sure what it should do instead. I added a failing test to expose this.

I'm a little confused about the mechanism as well, since librato.flush() with zero arguments makes the callback an empty function. Continuing to dig.

bobzoller commented 9 years ago

this just bit us too. it's a node-ism -- if an EventEmitter emits an error and nobody's listening, it throws an uncaught exception. @adborden thoughts?

adborden commented 9 years ago

Thanks for the test cases @kevinburkeshyp

Been thinking about this a bit. I don't think under any circumstance would the consumer want their app to crash in the event of a Librato error, so I think it makes sense to have a default error handler. I suppose logging out the error via console.log or console.error would be the right thing to do in the default case.

I also think it's a little weird to be using EventEmitter to expose the error handling[#33]. I would prefer if error handling is more explicit. One idea would be to add a addErrorHandler method which accepts an error callback and replaces librato-node's default error handler. That would be explicit, avoid the EventEmitter, and allow us to have a default error handler in case the consumer doesn't set one.

kevinburkeshyp commented 9 years ago

@adborden Sounds good to me

joshperry commented 9 years ago

Should be able to take advantage of node domains to easily fix this.