attribute: add BYTES type support#7948
attribute: add BYTES type support#7948NesterovYehor wants to merge 12 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7948 +/- ##
=====================================
Coverage 81.6% 81.6%
=====================================
Files 304 304
Lines 23446 23488 +42
=====================================
+ Hits 19136 19174 +38
- Misses 3928 3932 +4
Partials 382 382
🚀 New features to boost your workflow:
|
pellared
left a comment
There was a problem hiding this comment.
Overall LGTM. I left a few comments.
Please also add the benchmark results for ``BenchmarkBytes` in the PR description.
87ff3a1 to
95b2ce9
Compare
|
We would also need to comply with SDK attribute value limits https://opentelemetry.io/docs/specs/otel/common/#anyvalue
I propose to do this in a separate PR and also track it in a separate issue. The important thing is that both PRs would need to be shipped in the same release. If you want you can already start working it in a separate branch / draft PR EDIT: I created #7954 |
e155190 to
fc739fd
Compare
d7eb438 to
eda589a
Compare
|
@open-telemetry/go-approvers, PTAL. This can be now merged given the recent releases. |
MrAlias
left a comment
There was a problem hiding this comment.
Overall looks good. The hashing algorithm access needs attention though.
02b54de to
f6fff2d
Compare
| } | ||
| case BYTES: | ||
| h = h.Uint64(bytesID) | ||
| h = h.Bytes(kv.Value.asBytes()) |
There was a problem hiding this comment.
This will allocate a new slice. Is there a way to use the reflected value to avoid this allocation?
There was a problem hiding this comment.
The problem is that we currently have to either allocate a new slice or hash each byte one by one. The xxhash method needs a []byte slice, but we need a fixed-size array in the Value struct to keep it comparable. So, we either allocate memory for a new slice or spend extra time hashing each byte separately.
If i understand it correctly.
There was a problem hiding this comment.
I think this motivates storing []byte not in slice any, but stringly string of the Value using the built in Go syntax to copy user provided []byte into an immutable string. That will also allow for existing string hashing algorithms to be used as well.
Importantly, the vtype will distinguish between an actual string attribute and an byte slice one.
There was a problem hiding this comment.
The problem is that we currently have to either allocate a new slice or hash each byte one by one. The xxhash method needs a []byte slice, but we need a fixed-size array in the Value struct to keep it comparable. So, we either allocate memory for a new slice or spend extra time hashing each byte separately. If i understand it correctly.
You should be able to return the array as slice without introducing a heap allocation. Notice that asBytes copies the whole slice/array. Note that in #7949 UnsafeAsSlice does not cause a heap allocation.
MrAlias
left a comment
There was a problem hiding this comment.
Blocking base on #7948 (comment)
Investigation needs to be undertaken to storing the []byte as a string in value instead as a slice.
Fixes #7933
Add BYTES type to https://pkg.go.dev/go.opentelemetry.io/otel/attribute