search

SumoLogic / js-sumo-logger

Sumo Logic JavaScript SDK for Logging
Apache License 2.0
45 stars 26 forks source link

Logs are lost while request is pending. #95

Open kristians-salna opened 4 years ago

kristians-salna commented 4 years ago

this.sendLogs() method has a flaw that the assurance of data being successfully sent to the given endpoint is depending on combination of:

  1. Particular internet connection.
  2. sendLogs() execution timing from the client.

This particular flag - this.logSending is being reset within the this._postSuccess() method which is only being executed when the http promise is being resolved.

Which means that all logs which will be sent during the pending request time, will be dropped and forever lost here: if (this.logSending || this.pendingLogs.length === 0) { return false; }

This should work and no logs must be silently dropped without an interval/batch/flushLogs involvement.

billsaysthis commented 4 years ago

Are you suggesting that logs are only dropped given the above plus configInterval is set to zero or if flushLogs is called when logSending is true?

kristians-salna commented 4 years ago

Our use-case is that our app is generating sequentially two errors in a such interval that second is fired at the time first one is still pending. In such case, since our app is down and no more errors are going to be generated, the second error (which is being held in this.pendingLogs) is never fired, cause we won't trigger this,sendLogs() anymore.

As I mentioned in the original comment, using the flushLogs might help us here, but I have a feeling this should be working without calling flushLogs. Sumo should intercept all the incoming logs whilst the http promise is still pending and process them after promise resolves.

billsaysthis commented 4 years ago

What is your configInterval setting?

kristians-salna commented 4 years ago

Default. 0 I assume in such case.

billsaysthis commented 4 years ago

If sendLogs is called when logSending is true then the unsent log(s) would remain in pendingLogs and sent on the next call to sendLogs. Unless there is no next call because your app is shutting down, which is why we included flushLogs. Are you saying your app flow is different than this?

kristians-salna commented 4 years ago

The thing is that "app shutting down" is not that reliable. You might lose the logs if the browser crashes due to a null pointer exception, which is when things like onbeforeunload might not work. It's obviously a pretty rare edge case, but it could happen. To cover that we have to call flushLogs every time HTTP promise resolves. Not that big of a deal, but "automatic flushLogs" is the thing which I was originally suggesting should have been in place.

billsaysthis commented 4 years ago

I was on PTO but now considering options, will reply soon.

billsaysthis commented 4 years ago

I've been thinking about your last message for a few days now but I'm still a bit unsure about how I would implement automatic flushLogs especially since (a) not all users will set configInterval to zero and (b) the library cannot know the app crashed. I'm definitely open to suggestions or a PR but for now my only thought is that when sendLogs finishes checking if there's anything in pendingLogs and then if configInterval is zero, recursively calling sendLogs.

ldsenow commented 4 years ago

I guess if it provides different storage options (memory, localStoarge etc.) would solve the problem. By default is memory and when the app crashes it will lost the pending logs however, it depends how important these logs are. If critical, then relatively speaking, the logs could be synced up from a persistent local storage when the app launches again.

jamesottaway commented 2 years ago

Just wanted to stop by and say that this just bit me hard, and that this is an absolute show-stopper. This library cannot be trusted to actually send the logs that make it into the pendingLogs queue.

I cannot stress enough how broken this library is in it's current state.

In order to be a little more helpful, I wrote a small repro case: https://gist.github.com/jamesottaway/8865eb7aa8808efb9af9316e02fe8cac

As for a constructive suggestion, please just lean into promises and let the consumer manage them.

Two changes I'd suggest are:

  1. At a minimum, flushLogs should return a promise that only resolves when the items in the queue have been sent
  2. Deprecate the returnPromise option and make log always return a promise