Batch API calls

[cosmin-marginean]

This is a first implementation for batching API calls as per discussion here https://github.com/logtail/logback-logtail/issues/4.

Some notes:

• Batch size is configures using <batchSize>200</batchSize>, see logback-batch-test.xml
• Some tests require an explicit flush() in order to force the sending.
• There's a need for a Thread.sleep(2000) in LogtailAppenderBatchConfigSizeTest
• We recommend not suggesting queueSize in the documentation as to not create confusion with batchSize

[gyfis]

@cosmin-marginean Hi Cos, thanks a lot for the PR! There seem to be tests failing - do you want to tackle this or should the team here take a look? 🙌

[cosmin-marginean]

It's ok, I can have a look this afternoon, quite likely to do with me coding against jdk11 or something

[cosmin-marginean]

@gyfis I couldn't get this to break, but I see some 401 - Unauthorized. in the tests - are these tests using any "LOGTAIL_INGEST_KEY" env setting? https://github.com/logtail/logback-logtail/runs/5510436756?check_suite_focus=true

[gyfis]

@cosmin-marginean Ah, sorry, we do have a secret token on the repo which I think is not accessible by PRs. Let me test this locally real quick...

[gyfis]

@cosmin-marginean Hi again Cos, sorry, the tests pass fine locally :tada:

The PR looks good, too. I wonder if there's a way to register an exit event to flush any logs that are in the queue - this is so that this library doesn't discard logs. I think with this you could then drop the Thread.sleep and flush. This property seems like a good start: logging.register-shutdown-hook. Do you know if it's possible to register this somehow in our appender, to clean up after ourselves?

[cosmin-marginean]

That's a very good point @gyfis. Leave this with me, I'll do some experiments these days, as this will indeed be a problem beyond the tests - we don't want messages lost on app shutdown in production.

[gyfis]

Appreciate it! Tag me here or at hello@logtail.com if you'd like any help :pray: Thanks a lot!

[cosmin-marginean]

@gyfis I've added a flush() on stop() which seems to do the job: https://github.com/logtail/logback-logtail/pull/5/commits/04088535760ad2a0e0e46cfb3f6bf4364c886e27

I couldn't quite figure out how to write a test for this without actually querying the logs in the remote system, so the only way I could validate this was to send 5 messages (for a batch size of 10), and test with and without flush in stop() and look in the console/dashboard. The messages appear in Logtail when this is on.

We do test in LogtailAppenderShutdownTest that the API is not being called after only adding 5 messages though.

We can't really use a shared variable either as a second test (checking if the messages were flushed) would probably be run in a separate instance, so checking the number of API calls (like we do in other tests) won't help much.

Let me know if you think there's a better way to test this.

[gyfis]

@cosmin-marginean Hi Cos, thanks for the update!

Ideally (acknowledging I'm out of depth here), I'd like to see a mocked server-appender pair that we could start, stop, and inspect mid-run. Logzio seems to be doing something similar with their MockLogzioBulkListener used e.g. in the serverCrashTest, but I understand that's quite a lot of code to write.

Let me know if you want to tackle this too - otherwise I'll make one more review, test locally, and merge after!

[cosmin-marginean]

@gyfis That would be ideal, yes, I agree. I personally don't have the time at the moment to go into the depths of this unfortunately, but I'd be happy to review some work on this at some point if required. Thanks!

[gyfis]

The code looks good - thanks again Cos!