Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Client should emit errors rather than logging #55

Closed
mbrevoort opened this issue Mar 31, 2017 · 11 comments
Closed

Client should emit errors rather than logging #55

mbrevoort opened this issue Mar 31, 2017 · 11 comments

Comments

@mbrevoort
Copy link

When there are errors the client is spamming my logs with multiline stack trace logs like this:

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)

This is a problem is you are using a structured logger, for example a JSON line delimited format. Instead should emit errors instead. This would be trivial since the LD client is already an EventEmitter.

This would allow the consumer to control how logs are output. For example:

  ldClient.on('error', err => {
    let message = err
    if (err.stack) {
      let firstStackLine = (err.stack.split('\n')[1] || '').trim()
      message = `LaunchDarkly error: ${err} ${firstStackLine}`
    }
    logger.error(message)
  })
@dlau
Copy link
Contributor

dlau commented Apr 20, 2017

Hi Mike, thanks for the report.

This sounds like a candidate for something we may want for the next major version release. I will discuss with the team.

In the meantime, can I get a 👍 👎 from everyone with respect to how you want to consume errors? Errors strings, emitted errors, both?

@dlau
Copy link
Contributor

dlau commented Apr 27, 2017

Related PR that addresses the log spam: #59

How about we do this?

  • If we have an error event listener, registered with the client, we emit the error.
  • If we don't have an .on('error') registered, log it like we currently do.

@kaspiCZ
Copy link

kaspiCZ commented Jun 29, 2017

I like the idea. It will not break existing implementation and e.g. in our case will allow us to improve the handling. Removing the need to duplicate timeout watching on our end.

@apucacao
Copy link
Contributor

Hi @mbrevoort and @kaspiCZ ,

Thanks for your feedback and sorry for the delay. We'll get to this very shortly.

Best,
Alexis

@mbrevoort
Copy link
Author

@dlau emit if handler, log if not sounds perfect to me. The goal should be to never trap errors but give client consumer control of how to handle them.

@MartinNuc
Copy link

Is there any progress?

@apucacao
Copy link
Contributor

apucacao commented Nov 7, 2017

Hi @MartinNuc ,

Apologies for the delay, and thanks for inquiring! We're actively working on this.

I will update this issue as soon as we release this change.

Thank you for your patience,
Alexis

@MartinNuc
Copy link

We made a fork and using our fixed version for now.

This caused an outage of our app because server was waiting for LD to finish which failed to connect to LD API but did not emitted any error. Therefore although the connection was closed already our server thought that its still pending.

@apucacao
Copy link
Contributor

apucacao commented Nov 9, 2017

Sorry about that @MartinNuc. The fact that you had to fork to fix it is definitely not what we’re aiming for. We know that installing a 3rd-party library SDK into your own codebase requires a lot of trust, and we take this very seriously.

A couple of snags came up in our testing, but we're getting closer to release.

Thank you again for your patience,
Alexis

@apucacao
Copy link
Contributor

Hi @mbrevoort and @MartinNuc,

I wanted to let you know we finally released version 3.1.0 in which the client now emits errors instead of logging them if and only if an error event handler has been attached. We've also added specific error objects for different scenarios; we'll be adding more where needed in the future.

Let me know if you have any questions or feedback.

Best,
Alexis

@apucacao
Copy link
Contributor

Closing this for now -- please re-open if something comes up. Thanks!

eli-darkly added a commit that referenced this issue Mar 9, 2018
fix bad parameter that caused a redis query for "launchdarkly:undefined"
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants