feat(agent): output buffer persistence #15221
Conversation
telegraf-tiger
bot
added
the
feat
There was a problem hiding this comment.
Thanks @DStrand1 for this draft. It looks quite nice overall and I left some suggestions in the code.
What concerns me a bit is the underlying WAL implementation. Looking through their code I discovered some fundamental flaws.
- The WAL file is not opened with SYNC, so any issue on disk or with Telegraf will loose more metrics than expected (in the worst case with a lot of RAM) all metrics are lost...
- The filenames are saved with the pattern
<user defined name>.<index>.<offset>and are string-sorted, so if theindexoroffsetcross the order-boundary (i.e. a digit is added9 -> 10) the order is messed up aslala.10.xyzfollowslala.1.xyzinstead oflala.9.xyz! This will in turn mess up the metric order. - The WAL implementation does not care about removing files, so if you got WAL file(s) and restart Telegraf multiple time the metrics in the WAL will be written multiple times without further handling. Doing this handling outside of the WAL implementation is hard as you cannot know which file was processed.
- In the current form the WAL implementation is not capable of truncate the files front-to-back, so metrics are prone to be sent multiple times if the file was not completely flushed...
There are more (smaller) issues, but looking at the issues above I think you should go for another WAL library that is more mature. I looked at https://github.com/tidwall/wal in the past and think it can do what we need. Not a must, just a suggestion. ;-)
|
Thanks for the PR, super excided to see this. To capture what we talked about in pairs; specifically, to have you address some of the initial comments and switch to the proposed WAL library. Let's plan to talk through where you are at or any issues with the new library in Monday's pairs. Thanks! |
There was a problem hiding this comment.
Thanks @DStrand1 for the nice update! My only concern (despite test-coverage) is the error-handling. IMO we should pass errors up to be able to log them e.g. if we are running out-of-disk. For unrecoverable errors (e.g. corrupt WAL files, non-serializable metrics, etc) we probably should panic, but everything else should propagate up to the plugin level (at least).
models/buffer_disk.go
Outdated
| func (b *DiskBuffer) addSingle(metric telegraf.Metric) bool { | ||
| err := b.walFile.Write(b.writeIndex(), b.metricToBytes(metric)) | ||
| metric.Accept() | ||
| if err == nil { | ||
| b.metricAdded() | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is it really worth the code duplication or should we use the batch-interface instead?
| return index | ||
| } | ||
|
|
||
| func (b *DiskBuffer) Add(metrics ...telegraf.Metric) int { |
There was a problem hiding this comment.
I believe we should return an error here to log it or do other mitigation...
| return written | ||
| } | ||
|
|
||
| func (b *DiskBuffer) Batch(batchSize int) []telegraf.Metric { |
Summary
Implements the
write-throughbuffer persistence strategy detailed in the spec added in #14928.Currently this PR is mostly a draft of separating the output buffer into multiple implementations, as well as experimentation with a WAL file library. Largest outstanding issue is around metric serialization to
[]byte.encoding/gob, which is what the PR is currently doing. However this has issues with un-exported fields, so I need to look more into how to work around thisChecklist
Related issues
Related to #802, #14805