[jaeger-v2] Implement UTF-8 sanitizer for OTLP #6078
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
| default: | ||
| sanitized = append(sanitized, []byte(v.Str())...) |
There was a problem hiding this comment.
I'd like to get a second set of eyes on this. In v1, we casted the value to a lossy string and converted it to its byte representation. Is just doing v.str() safe here?
There was a problem hiding this comment.
Write unit tests for different types and we will found out.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6078 +/- ##
==========================================
+ Coverage 96.92% 96.94% +0.02%
==========================================
Files 351 352 +1
Lines 16675 16724 +49
==========================================
+ Hits 16162 16213 +51
+ Misses 329 328 -1
+ Partials 184 183 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| const ( | ||
| invalidOperation = "InvalidOperationName" |
There was a problem hiding this comment.
Values don't match kebub-case from the other sanitizer
| ) | ||
|
|
||
| const ( | ||
| invalidOperation = "InvalidOperationName" |
There was a problem hiding this comment.
Also let's use "span name" term, not operation
| span.SetName(invalidOperation) | ||
| } | ||
|
|
||
| serviceName, ok := span.Attributes().Get(string(otelsemconv.ServiceNameKey)) |
There was a problem hiding this comment.
Why do we need to special case service.name? Wouldn't it be handled generically as all other attributes?
There was a problem hiding this comment.
Also, service name is supposed to be defined at the resource level, not at individual spans.
|
|
||
| if v.Type() == pcommon.ValueTypeStr && !utf8.ValidString(v.Str()) { | ||
| sanitized := []byte(v.Str()) | ||
| newVal := attributes.PutEmptyBytes(k) |
There was a problem hiding this comment.
This contradicts the comment above about modifying the attributes while iterating over them.
There was a problem hiding this comment.
@yurishkuro the comment was for modifying the keys of the map. is there anything wrong with modifying the value associated with a key?
There was a problem hiding this comment.
I don't know, the command is put(), it may depend on the implementation.
| default: | ||
| sanitized = append(sanitized, []byte(v.Str())...) |
There was a problem hiding this comment.
Write unit tests for different types and we will found out.
| var utf8EncodingTests = []struct { | ||
| name string | ||
| key string | ||
| value string |
There was a problem hiding this comment.
It seems if your tests only allow string values you cannot test other attribute types.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test