Allow for long log lines #265
Conversation
|
|
|
@tomhjp Excuse me, Can you help me check this pr? Do you have any suggestions? |
|
Hey @luoxiaohei - my plate is pretty full atm, but just wanted to say this looks like something we should be able to land and I'll try to revisit it soon. Thanks for putting this together, I can see it's been very thoughtfully constructed. |
Thanks for your positive feedback on my pr, I look forward to seeing it merged soon. |
|
Thinking out loud a bit, an alternative solution would be to allow lines that cross the buffer threshold to build up in memory and deserialise them once Instead, this PR allows clients to opt into behaviour that increases memory consumption, and still keeps a hard cap on it at the increased level. It's more fiddly because now clients have to be aware of the maximum log size they expect from a plugin, but also safer so I think the correct decision. Does that reasoning track with you @luoxiaohei, or am I missing an opportunity to simplify here? |
|
@tomhjp Yes, your reasoning tracks with me. I think the configurable option is a good balance between flexibility and safety. |
…ing test - Renamed `LogBufferSize` to `PluginLogBufferSize` to better reflect its purpose. - Updated all references of `LogBufferSize` to `PluginLogBufferSize`. - Added a new test `TestClient_logStderrParseJSON` to verify the parsing of JSON formatted logs.
|
@tomhjp I've refining the naming and enhancing the unit tests to explicitly cover the intent of the changes. Please take another look when you have a moment. Thanks! |
This pull request introduces a new feature to the
ClientConfigin theclient.go. The main change involves the addition of aLogBufferSizefield, which will serve as the buffer size for reading stderr logs.Currently, the
stdErrBufferSizeis set to a default value of 64 KB. When a log line exceeds this size, the client will not perform json unmarshal and subsequent processing for that line. This behavior can be unfriendly for certain use cases that involve lengthy logs.To address this limitation, the proposed solution is to add the
LogBufferSizefield to theClientConfig, allowing users to customize their own buffer size when creating a client. If the user doesn't specify a value, the default of 64 KB will be used to maintain compatibility with the current implementation.An alternative approach would be to handle oversized logs by creating a new buffer to retain truncated data, concatenating it with historical data on the next read, and then proceeding with the complete log unmarshal. However, this method introduces added complexity and could lead to performance overhead in scenarios where logs consistently exceed the buffer size.
Considering simplicity and ease of implementation, the configuration-based solution is preferred. Users can now effortlessly fine-tune the buffer size to suit their specific logging needs.