-
Notifications
You must be signed in to change notification settings - Fork 513
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
Expansion of Encoding Interfaces and Addition of V2 #1227
Conversation
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
rpc PushBytes(PushBytesRequest) returns (PushResponse) {}; | ||
// different versions of PushBytes expect the trace data to be pushed in different formats | ||
rpc PushBytes(PushBytesRequest) returns (PushResponse) {}; // ./pkg/model/v1 | ||
rpc PushBytesV2(PushBytesRequest) returns (PushResponse) {}; // ./pkg/model/v2 |
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.
Are you adding both versions here to allow handling of both versions at the same time?
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.
yup. while the distributors transition to the new version the ingesters will need to be able to take both v1 and v2.
@@ -87,6 +89,14 @@ var ( | |||
}, []string{discardReasonLabel, "tenant"}) | |||
) | |||
|
|||
// rebatchedTrace is used to more cleanly pass the set of data | |||
type rebatchedTrace struct { | |||
id []byte |
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.
Is id
used? I see trace.id
used, but maybe missed where id
is used.
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.
Yup, it's used on line 333 where the distributor creates the batches it sends to the ingesters.
|
||
_ = buffer.EncodeFixed32(uint64(start)) // EncodeFixed32 can't return an error | ||
_ = buffer.EncodeFixed32(uint64(end)) | ||
err := buffer.Marshal(pb) |
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.
Is this marshal appending to the buffer? So the two uint64s are put in place, and then the variable-length trace bytes?
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.
technically two uint32s, but yes. for some reason EncodeFixed32
takes a uint64.
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.
Right sorry, uint32
.
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.
Overall this is looking good and should make it easier to support more data encodings in the future. Do you have any numbers on how much the new per-trace start/end improve search times?
I'm not sure I am understanding the difference between ObjectDecoder and SegmentDecoder. In V2 they have the same underlying format (uint32/uint32/[]byte), and similarities in PrepareForRead and Combine/ToObject. Could/should these be combined, or maybe some more comments/clarification why they are not? This might be a terminology/definition difference (see the other comment for more specifics)
Initially I had them combined but found that more confusing. Having the SegmentDecoder handle the relationship between Distributor/Ingester and the ObjectDecoder handle the backend felt more natural to me. For v2 the SegmentDecoder is handling uint32/uint32/tempopb.Trace and the ObjectDecoder is handling uint32/uint32/tempopb.TraceByte which contains byte slices of unmarshalled tempopb.Traces. These things are just askew enough I felt like they should be distinct. I would definitely take recommendations on consolidating if you have some thoughts. |
Signed-off-by: Joe Elliott <[email protected]>
What this PR does:
Expands the encoding interfaces added in #1211 and adds a
v2
interface which is capable of efficiently retrieving its range to improve search speeds. This PR also puts everything into place to fix the start/end time range of blocks referenced in #1175 .Marked as draft b/c this shouldn't be merged in 1.3.
Also it needs some cleanup.UPDATE:
This PR is ready for review but I'm going to leave it draft until 1.3 is cut.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]