Change Span/Trace ID to be byte array #2001
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2001 +/- ##
==========================================
+ Coverage 91.49% 91.55% +0.06%
==========================================
Files 282 282
Lines 16628 16631 +3
==========================================
+ Hits 15214 15227 +13
+ Misses 978 973 -5
+ Partials 436 431 -5
Continue to review full report at Codecov.
|
|
|
||
| // NewSpanID creates a SpanID from a byte slice. | ||
| func NewSpanID(bytes []byte) SpanID { | ||
| func NewSpanID(bytes [8]byte) SpanID { |
There was a problem hiding this comment.
I wonder if it makes sense to define type alias for [8]byte (or [spanIDSize]byte), name it something like SpanIDBytes and use everywhere (and similar for traceid).
There was a problem hiding this comment.
Can be done in a followup PR.
| } | ||
|
|
||
| // Unmarshal inflates this trace ID from binary representation. Called by Protobuf serialization. | ||
| func (t *SpanID) Unmarshal(data []byte) error { | ||
| return (*bytesID)(t).Unmarshal(data) | ||
| func (sid *SpanID) Unmarshal(data []byte) error { |
There was a problem hiding this comment.
We probably need to document somewhere that either 0 length or exactly 8 byte length span id is allowed. Previously any length was allowed. Same for trace id.
There was a problem hiding this comment.
Based on the proto definition:
https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/trace/v1/trace.proto#L58
I think we have that documented there, do you think duplicating the comment is ok?
| } | ||
|
|
||
| // IsValid returns true if id contains at leas one non-zero byte. | ||
| func (sid SpanID) IsValid() bool { |
There was a problem hiding this comment.
Should we call this IsNonZero instead? I am not quite sure "valid" is the right term. What do you think?
There was a problem hiding this comment.
Based on w3c and https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/trace/v1/trace.proto#L58 the all zeros is called invalid, so I wanted to keep it consistent.
| } | ||
|
|
||
| // Compare the ids. See bytes.Compare for expected return values. | ||
| func (t SpanID) Compare(that SpanID) int { |
There was a problem hiding this comment.
Is Compare function unnecessary? I think it was called by Gogoproto marshaling/unmarshaling.
There was a problem hiding this comment.
- All tests are passing.
- I did a grep in the generated files, no
Comparefound, so I expect that it is not used.
| return false | ||
| // Transforms the byte slice trace ID into a [16]byte internal pdata.TraceID. | ||
| // If larger input then it is truncated to 16 bytes. | ||
| func traceIDToInternal(traceID []byte) pdata.TraceID { |
There was a problem hiding this comment.
Perhaps it would be useful to have error handling for the cases when input byte slice is not the expected length. We should probably accept everything in the [0..16] range and reject anything longer. It likely "never" happens but who knows what we see in the input. Do you think it is fine to silently truncate?
There was a problem hiding this comment.
I think it is fine to silently truncate, we have it documented in both protos otlp and opencensus that the size should be 16.
|
This should reduce the number of allocations of slices used for traceid/spanid. Can you run before/after benchmark, I am curious if we can see any performance difference. |
A lot of boilerplate changes, the major ones: * Return error and reject requests with trace/span ids that are not 16/8 byte arrays * For OC copy only the first 8/16 bytes if larger than that, if shorter pad with 0 at the end. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
99ce77e to
ded9159
Compare
Before: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/consumer/pdata BenchmarkTracesFromOtlp BenchmarkTracesFromOtlp-16 622 1885796 ns/op 1105052 B/op 40944 allocs/op PASS ``` After: ``` goos: darwin goarch: amd64 pkg: go.opentelemetry.io/collector/consumer/pdata BenchmarkTracesFromOtlp BenchmarkTracesFromOtlp-16 686 1759476 ns/op 981565 B/op 35113 allocs/op PASS ``` Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
…lemetry#2001) Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
A lot of boilerplate changes, the major ones:
Before:
After: