Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logs are lost while request is pending. #95

Open
kristians-salna opened this issue Mar 27, 2020 · 10 comments
Open

Logs are lost while request is pending. #95

kristians-salna opened this issue Mar 27, 2020 · 10 comments
Projects

Comments

@kristians-salna
Copy link

kristians-salna commented Mar 27, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

What is your configInterval setting?

@kristians-salna
Copy link
Author

Default. 0 I assume in such case.

@billsaysthis
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@billsaysthis
Copy link
Contributor

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
Copy link

ldsenow commented Jul 13, 2020

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.

@safaiyeh safaiyeh added this to To do in Bugs Jun 12, 2021
@jamesottaway
Copy link

jamesottaway commented Sep 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Bugs
To do
Development

No branches or pull requests

4 participants