L-1147 Add batchSizeKiB option, add default size for Browser #102
Conversation
| @@ -21,6 +21,9 @@ const defaultOptions: ILogtailOptions = { | |||
| // Maximum number of logs to sync in a single request to Better Stack | |||
| batchSize: 1000, | |||
|
|
|||
| // Size of logs (in KiB) to trigger sync to Better Stack (0 to disable) | |||
| batchSizeKiB: 0, | |||
There was a problem hiding this comment.
Any reason why this should be disabled by default?
Could you use default that prevents issues with running into memory limits? 🙏
There was a problem hiding this comment.
@curusarn My idea here was that the limit only makes sense for keep alive requests. I don't really see any issue in sending even a few MiB in a single request - do you?
There is a 48KiB limit for Browser that makes keepalive requests.
Other scenarios use Msgpack instead of JSON for serialization, so we'd need different size calculation. The way it's implemented at this moment, we're still not preventing the buffer from getting filled by logs when requests start to fail and are being retried (eg. when user's machine gets disconnected from the internet for a while).
Honestly, this PR aims just to solve the underlying issue of the keepalive request body limit. Keeping it disabled by default has the added benefit of not serializing data twice.
The default batch size is 1000 records or 1 second of runtime - that's not going to reach any memory limit that's set reasonably high. And if a user want's to keep memory use low, they now have a better tool for achieving that.
WDYT? Maybe I'm not seeing something, and there's a good reason for a specific default value 🤔
There was a problem hiding this comment.
Makes sense. Let's keep it off by default. Thanks a lot!
Resolves #100
Adds a new option
batchSizeKiBand configures Browser to automatically flush logs after exceeding 48 KiB in total to avoid reaching maximum amount of queued data for keepalive requests.