launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

More descriptive error on failed connections #56

Closed mbrevoort closed 6 years ago

mbrevoort commented 7 years ago

Given this service interruption this morning:

screenshot 2017-03-31 08 56 04

It would be great if the error produced could better describe the problem (failed connection?) instead of a non-descriptive stacktrace:

error: [LaunchDarkly] Error: Unexpected error:
    at /Users/mike/dev/missions/missions/node_modules/ldclient-node/index.js:71:19
    at EventSource.es.onerror (/Users/mike/dev/missions/missions/node_modules/ldclient-node/streaming.js:19:7)
    at emitOne (events.js:96:13)
    at EventSource.emit (events.js:188:7)
    at _emit (/Users/mike/dev/missions/missions/node_modules/ldclient-node/eventsource.js:182:17)
    at ClientRequest.onConnectionClosed (/Users/mike/dev/missions/missions/node_modules/ldclient-node/eventsource.js:43:5)
    at emitOne (events.js:96:13)
    at ClientRequest.emit (events.js:188:7)
    at TLSSocket.socketErrorListener (_http_client.js:309:9)
    at emitOne (events.js:96:13)
dlau commented 7 years ago

Hi Mike, apologies for the late response and thanks for the report.

Definitely, we could have better error reporting. As you can see, it's just a matter of handling a few more http status codes:

https://github.com/launchdarkly/node-client/blob/cdddf8f716812cb5bc920fd3429c9f22db660990/index.js#L68

I'll file an issue on our end

Chris911 commented 7 years ago

@dlau, there also seems to be an error on this line: https://github.com/launchdarkly/node-client/blob/b501fbcac11df0c3c082f47c3923901568ae2e54/index.js#L73

I don't think that's a valid constructor call for Node Error. If err is somehow a string that line should probably be: new Error("Unexpected error: " + err);

We're hitting this line in production and can't figure out what the underlying error is.

apucacao commented 7 years ago

Hi @Chris911 ,

Thanks for bringing this up. We're currently making some improvements around error reporting in the SDK so we'll take a look at this right now too.

Alexis

Chris911 commented 7 years ago

Sounds good. Would appreciate if you could ping me in this PR when it's fixed.

apucacao commented 7 years ago

Will do

apucacao commented 6 years ago

Hi @mbrevoort and @Chris911 ,

I apologize for the delay on this. We just released a new version of our SDK which should make this better. For instance, if the SDK fails to connect to the streaming API as indicated by that status update, you would get a LDStreamingError with a more informative message.

Hope that helps. Please let us know if you have any other questions or feedback!

Cheers, Alexis