tracing: fix corruption of structured recording buffer #75099
Conversation
|
This change is |
andreimatei
force-pushed
the
tracing.fix-structured-buf
branch
from
4dc5b50 to
fb7f04e
Compare
pkg/util/tracing/crdbspan.go
Outdated
| for _, c := range children { | ||
| for _, e := range c.StructuredRecords { | ||
| s.recordInternalLocked(&e, &s.mu.recording.structured) | ||
| for c := range children { |
There was a problem hiding this comment.
[mega nit] c looks like it'd be of type tracingpb.RecordedSpan but it's an index.
Not really. The symptoms that the failed tests report (trying to read the first element of an empty buffer) are caused by the fact that the computed size of the buffer is not kept in sync with reality, which is a problem when a span overflows its byte limit for the structured recordings. So one thing that would trigger it is a change that makes some span accumulate more structured recordings than before. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @irfansharif)
There was a problem hiding this comment.
bors r+
Reviewable status:
This patch fixes a bug leading to corruption of one of the Span's
recording buffers.
We had broken code like the following:
```
for _, e := range c.StructuredRecords {
s.recordInternalLocked(&e, &s.mu.recording.structured)
}
```
`recordInternalLocked(ptr)` takes ownership of ptr, so the `&e` is
broken because `e` keep being reassigned.
The bug was causing buffer elements to essentially be overwritten. In
turn, this was leading to inconsistencies in the accounting of the size
of the buffer's elements - we were maintaining a size sum that was
diverging from the overwritten reality.
Fixes cockroachdb#74994
Fixes cockroachdb#75045
Release note: None
andreimatei
force-pushed
the
tracing.fix-structured-buf
branch
from
fb7f04e to
5a1adf7
Compare
|
Canceled. |
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This patch fixes a bug leading to corruption of one of the Span's
recording buffers.
We had broken code like the following:
recordInternalLocked(ptr)takes ownership of ptr, so the&eisbroken because
ekeep being reassigned.The bug was causing buffer elements to essentially be overwritten. In
turn, this was leading to inconsistencies in the accounting of the size
of the buffer's elements - we were maintaining a size sum that was
diverging from the overwritten reality.
Fixes #74994
Fixes #75045
Release note: None