-
Notifications
You must be signed in to change notification settings - Fork 699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Remaining and Size to perf/ring Record #1167
Conversation
52c47f9
to
3b522ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, can you add test coverage please?
perf/reader.go
Outdated
Remaining int | ||
|
||
// The total size of the per-CPU perf buffer in bytes. | ||
Size int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Size is fixed for the reader, why not expose this as Reader.Size() int
instead? You can move the call to perfBufferSize
from newPerfEventRing
to NewReaderWithOptions
. Same idea applies to ringbuf.Reader
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size we are interested in, is the data portion, without the metadata page. Moving the call to perfBufferSize
would've required changes to a bunch of other places that expect newPerfEventRing
to handle the size manipulation. I did something different, let me know what you think.
433565f
to
515ea9b
Compare
Added. For the backward/overwriteable buffer it isn't possible to get a precise remaining count once it has been overwritten. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for adding the tests! Final question: seems like the semantics for overwritable aren't clear. Should we just set remaining to -1
for now in that case and document that this means the data remaining can't be determined?
We could. It does work correctly until any kind of overwrite happens, and then it becomes inaccurate by reporting more data remaining than their actually is (because of the partial leftover records). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a doc fix. Please squash then we are good to go.
Signed-off-by: Bryce Kahle <bryce.kahle@datadoghq.com> Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
220e1e7
to
5a0992d
Compare
squashed |
@lmb ready to go. |
This allows tracking utilization of the underlying buffers. Processing the values to make sense in a user's monitoring system is left to the user.
Using the post-read remaining bytes allows the value to reach zero, whereas a pre-read value would never reach zero.