Possible concurrency issue and possible lost logs issue

logtail / logback-logtail

Better Stack Java Logback appender

https://betterstack.com/logs

MIT License

17 stars 4 forks source link

Possible concurrency issue and possible lost logs issue #6

Closed romanbsd closed 1 year ago

romanbsd commented 2 years ago

We are considering to use logtail in production, and I was reviewing the code of the logback-logtail appender. There're a few issues that I noticed from reading the code:

The batch instance member is not a thread safe collection, thus it can lead to thread safety issues.
The batch is accessed here https://github.com/logtail/logback-logtail/blob/main/src/main/java/com/logtail/logback/LogtailAppender.java#L127 and cleared in line 136, leading to possible loss of data if events were appended in between.
The batch is flushed every 1000 events, so if the batch has 990 events and the system is stopped or crashed, these events are lost. I suggest an auto-flush every 1 second.

P.S. IMHO using javax.ws.rs.client.Client here is an overkill and it impedes performance.

adikus commented 2 years ago

Hi @romanbsd, thanks for taking the time to review our code!

All three points you mentioned make total sense to me. I'll try to take some time to review them and come up with some changes. That said I can't promise it will be immediately, sorry about that!

For your P.S., what would you use instead? I'm just curious.

romanbsd commented 2 years ago

I'd just use a plain HttpClient available from Java8 and execute the calls asynchronously. The heavy lifting of JSON serialization is performed by Jackson anyway...

cowwoc commented 1 year ago

I agree. Fixing batch is a critical issue.

And with respect to JAX-RS, I agree that ideally you should use the built-in HTTP client. If this were a more elaborate app then I would understand the JAX-RS dependency but for a logger it seems overkill.

PetrHeinz commented 1 year ago

Thanks @romanbsd for opening the issue and pinpointing the issues.

In 0.3.0 (see #13), several improvement are implemented:

thread-safe Vector collection is used
only those logs, that were sent to Better Stack are cleared during flush
there is a configurable max waiting time for logs to be sent (batchInterval), currently defaulting to 3 seconds
trimmed down dependencies significantly

I'll do some further testing and should it be merged and released in no time.

If you have any more concerns about concurrency, feel free to reopen the issue after it closes, I'll appreciate it 🙏