feat: ensure that log batches don't exceed 1MiB in logSender #12048
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
agent/logs.go
Outdated
| const ( | ||
| flushInterval = time.Second | ||
| logOutputMaxBytes = 1 << 20 // 1MiB | ||
| overheadPerLog = 21 // found by testing |
There was a problem hiding this comment.
We should probably also have an upper bound of buffered logs since these will take up memory, or alternatively, store logs on disk until they can be sent.
If something goes wrong with a command, there's really no telling how much it will spit out, it could be KiB's or it could be GiB's (in extreme cases). We also have limits in place in the API that limit the output to 1 MiB, although I can't recall the details if we store both the head and tail end of logs or simply cutoff at 1 MiB.
There was a problem hiding this comment.
Not a bad idea. Can address in a follow on PR.
4bcbd7e to
0f99947
Compare
181b699 to
27b55aa
Compare
agent/logs.go
Outdated
| break | ||
| } | ||
| req.Logs = append(req.Logs, log) | ||
| n++ |
There was a problem hiding this comment.
I think this change here demonstrates a downside of using PR stacking too much. I don't think it's a good idea to split up an implementation like this, especially a new one (I might be of a different opinion for something much bigger).
Here is a change that introduces more advanced logic in a place where I left a simple suggestion in the previous PR. I think that comment applies here too (as in we don't need a clone of the slice, we can utilize the backing array/append semantics). But based on the comment from my previous PR, this may be easily overlooked due to the change in this one.
agent/logs.go
Outdated
| if len(log.Output) > logOutputMaxBytes { | ||
| logger.Warn(ctx, "dropping log line that exceeds our limit") | ||
| n++ | ||
| continue |
There was a problem hiding this comment.
How about truncating instead of dropping entirely?
0f99947 to
15855f4
Compare
27b55aa to
e941fbc
Compare
89ce3e7 to
c8e7282
Compare
e941fbc to
ede6f2f
Compare
c8e7282 to
0515169
Compare
ede6f2f to
743f5f3
Compare
Protobuf messages are designed to be about 1MiB or less, so this PR ensures that even if we get a lot of logs or logs with huge payloads we don't send more than 1MiB at a time.
We straight up drop logs that themselves exceed 1MiB, and batch up logs so that each request is less than 1MiB if we have a lot of small logs.